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

[Jake Oshins]

> -----Original Message-----
> From: Bjorn Helgaas [mailto:helgaas@xxxxxxxxxx]
> Sent: Wednesday, February 3, 2016 1:29 PM
> To: Jake Oshins <jakeo@xxxxxxxxxxxxx>
> Cc: gregkh@xxxxxxxxxxxxxxxxxxx; KY Srinivasan <kys@xxxxxxxxxxxxx>; linux-
> kernel@xxxxxxxxxxxxxxx; devel@xxxxxxxxxxxxxxxxxxxxxx; Haiyang Zhang
> <haiyangz@xxxxxxxxxxxxx>; marc.zyngier@xxxxxxx;
> bhelgaas@xxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH RESEND 3/3] PCI: hv: New paravirtual PCI front-end for
> Hyper-V VMs
> 
> Hi Jake,
> 
> On Tue, Feb 02, 2016 at 05:41:43PM +0000, jakeo@xxxxxxxxxxxxx wrote:
> > From: Jake Oshins <jakeo@xxxxxxxxxxxxx>
> >
> > This patch introduces a new driver which exposes a root PCI bus whenever
> a
> > PCI Express device is passed through to a guest VM under Hyper-V. The
> > device can be single- or multi-function. The interrupts for the devices
> > are managed by an IRQ domain, implemented within the driver.

[lots of stuff snipped out]

> > +/**
> > + * hv_pci_free_bridge_windows() - Release memory regions for the
> > + * bus
> > + * @hbus:	Root PCI bus, as understood by this driver
> > + */
> > +static void hv_pci_free_bridge_windows(struct hv_pcibus_device *hbus)
> > +{
> > +	if (hbus->low_mmio_space && hbus->low_mmio_res) {
> > +		hbus->low_mmio_res->flags |= IORESOURCE_BUSY;
> 
> Drivers should not normally set or clear IORESOURCE_BUSY themselves.
> 
> > +		release_mem_region(hbus->low_mmio_res->start,
> > +				   resource_size(hbus->low_mmio_res));
> 
> Usually there's a request_mem_region() to correspond with a
> release_mem_region(), and that takes care of IORESOURCE_BUSY.
> 
> What's unique about this driver, and can you make it less unique?
> 

Thanks for the detailed review.  I'll make all the changes that you're suggesting and resend.

As for the comment above, and all the others related to IORESOURCE_BUSY, this amounts to making the resource claim look like a bridge window, so that callers of request_mem_region() in the drivers for the child devices actually can succeed.  There's no function in the kernel today that amounts to "request_mem_region_for_bridge_window()" or at least none that I understood.  The current plug and play code in Linux is pretty hard coded for various bus types, i.e. ACPI and PCI.  I took a tack at one point where I tried to offer changes to it, so that it understood the concept of "grandchild of ACPI or PCI" which is what would make this code simpler.  Rafael Wysocki basically told me that the pnp layer is deprecated and that new changes to it wouldn't be entertained, and that I should find some way of using what exists.  This was the result.  If you have another suggestion, I'm happy to try it.  In any case, I'll explain what's happening more thoroughly in comments.

Thanks,
Jake Oshins