RE: [PATCH v6 7/7] PCI: hv: New paravirtual PCI front-end for Hyper-V VMs

[Jake Oshins]

> -----Original Message-----
> From: Jiang Liu [mailto:jiang.liu@xxxxxxxxxxxxxxx]
> Sent: Wednesday, December 2, 2015 7:12 PM
> To: Jake Oshins <jakeo@xxxxxxxxxxxxx>; gregkh@xxxxxxxxxxxxxxxxxxx; KY
> Srinivasan <kys@xxxxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx;
> devel@xxxxxxxxxxxxxxxxxxxxxx; olaf@xxxxxxxxx; apw@xxxxxxxxxxxxx;
> vkuznets@xxxxxxxxxx; tglx@xxxxxxxxxx; Haiyang Zhang
> <haiyangz@xxxxxxxxxxxxx>; marc.zyngier@xxxxxxx;
> bhelgaas@xxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v6 7/7] PCI: hv: New paravirtual PCI front-end for Hyper-
> V VMs
> 
> On 2015/11/3 5:33, jakeo@xxxxxxxxxxxxx wrote:
> > From: Jake Oshins <jakeo@xxxxxxxxxxxxx>
> >
[...]
> > +
> > +/**
> > + * hv_irq_unmask() - "Unmask" the IRQ by setting its current
> > + * affinity.
> > + * @data:	Describes the IRQ
> > + *
> > + * Build new a destination for the MSI and make a hypercall to
> > + * update the Interrupt Redirection Table. "Device Logical ID"
> > + * is built out of this PCI bus's instance GUID and the function
> > + * number of the device.
> > + */
> > +void hv_irq_unmask(struct irq_data *data)
> > +{
> > +	struct msi_desc *msi_desc = irq_data_get_msi_desc(data);
> > +	struct irq_cfg *cfg = irqd_cfg(data);
> > +	struct retarget_msi_interrupt params;
> > +	struct hv_pcibus_device *hbus;
> > +	struct cpumask *dest;
> > +	struct pci_bus *pbus;
> > +	struct pci_dev *pdev;
> > +	int cpu;
> > +
> > +	dest = irq_data_get_affinity_mask(data);
> > +	pdev = msi_desc_to_pci_dev(msi_desc);
> > +	pbus = pdev->bus;
> > +	hbus = container_of(pbus->sysdata, struct hv_pcibus_device,
> sysdata);
> > +
> > +	memset(&params, 0, sizeof(params));
> > +	params.partition_id = HV_PARTITION_ID_SELF;
> > +	params.source = 1; /* MSI(-X) */
> > +	params.address = msi_desc->msg.address_lo;
> > +	params.data = msi_desc->msg.data;
> > +	params.device_id = (hbus->hdev->dev_instance.b[5] << 24) |
> > +			   (hbus->hdev->dev_instance.b[4] << 16) |
> > +			   (hbus->hdev->dev_instance.b[7] << 8) |
> > +			   (hbus->hdev->dev_instance.b[6] & 0xf8) |
> > +			   PCI_FUNC(pdev->devfn);
> > +	params.vector = cfg->vector;
> > +
> > +	for_each_cpu_and(cpu, dest, cpu_online_mask)
> > +		params.vp_mask |= (1 <<
> vmbus_cpu_number_to_vp_number(cpu));
> No knowledge about the HV implementation details, but feel some chances
> of race here between hv_irq_unmask(), hv_set_affinity() and
> cpu_up()/cpu_down() when accessing 'dest' and cpu_online_mask.
> 
 
Thanks.  Is there any architectural contract here?  I tried implementing this by doing this work in the set_affinity() callback, but the vector was often wrong when that callback was invoked.  (It seems to get changed just after set_affinity().)  Can you suggest a durable strategy?

I'll respond to all the other comments you sent (and this one, once I understand the right response) and resend.

Thanks for your review,
Jake Oshins

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/