Thank you for your replay.
I will submit new patch following your comments.
On Wed, 12 Nov 2008 11:51:04 -0800
Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx> wrote:
> On Tuesday, November 11, 2008 9:37 pm Yuji Shimada wrote:
> > As I have not received a reply to my mail of November 6, 2008, I am
> > resending it herewith.
> >
> > This patch adds the function that reassigns page-aligned memory
> > resources to device, to linux.
> >
> > I created this patch for xen's dom0 linux. It have already been
> > included in xen's dom0 linux. It is useful when we assign I/O device
> > to HVM domain using pci passthrough, because page-aligned memory
> > resource is required for pci passthrough. It is also useful for
> > KVM. So I submit it to linux-pci ML.
>
> Ok, so the fundamental requirement here is to assign PCI devices to guests,
> right? That means PCI resources be aligned to a page boundary and take up at
> least a page so that no other resources will fall into the same page, right?
Yes, that's right.
> So you add a quirk to unassign the resources of the device(s) in question and
> let the core reassign them according to your new constraints. So far, so
> good.
>
> Like Matthew said, reassigning at runtime should be possible, but can be a
> little trickier since your configuration has to disallow driver binding (or
> unbind everything) until after you've done your reassignments. For v11n
> setups that doesn't seem wholly unreasonable, but probably isn't as convenient
> as simply doing it at startup time.
Reassigning at runtime needs suspending device driver. Device driver
should hide the fact of suspending from upper layer. To achieve this,
device driver should queue requests from upper layer during
suspending.
Currently linux does not have such mechanism. So reassigning at
runtime is big challenge.
> Anyway, comments on your patch below.
>
> > To reassign page-aligned memory resources to device, please add boot
> > parameter of linux as follows.
> >
> > reassigndev=00:1d.7,01:00.0
> >
> > reassigndev= Specifies device to reassign page-aligned
> > memory resources. PCI-PCI bridge can be
> > specified, if resource windows need to be
> > expanded.
>
> I'd call this pagealignbars= or something instead, since you're not just
> reassigning things (that happens anyway for conflicting resources and for
> quirked devices), you're making sure that resources really are page aligned.
> It should probably also be part of the pci= parameter; besides your code for
> parsing out PCI device specifiers might be handy in the future (I was
> surprised my quick look didn't find some other boot option that already did
> it).
I will change "reassigndev=" to "pci=pagealignmem=", because only
memory resource is page-aligned.
> > diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
> > index e1ca425..e85896e 100644
> > --- a/drivers/pci/Kconfig
> > +++ b/drivers/pci/Kconfig
> > @@ -32,6 +32,20 @@ config PCI_LEGACY
> > option serves to include/exclude only a few drivers that are
> > still using this API.
> >
> > +config PCI_REASSIGN
> > + bool "Enable reassign page-aligned memory resources to device"
> > + depends on PCI
> > + default y
> > + help
> > + Say Y here if you want to reassign page-aligned memory resources to
> > + the device. And add boot parameter of linux as follows.
> > +
> > + reassigndev=00:1d.7,01:00.0
> > +
> > + "reassigndev=" specifies devices to reassign page-aligned memory
> > + resources. PCI-PCI bridge can be specified, if resource windows need
> > + to be expanded.
>
> Since you're adding this as a quirk, you don't need a separate config option
> (we have one for quirks as a whole already), so long as you include in in the
> #ifdef CONFIG_PCI_QUIRKS section.
I will remove PCI_REASSIGN, and include my code in the #ifdef
#CONFIG_PCI_QUIRKS section.
> > +/*
> > + * This quirk function disables the device and releases resources
> > + * which is specified by kernel's boot parameter 'reassigndev'.
> > + * Later on, kernel will assign page-aligned memory resource back
> > + * to that device.
> > + */
> > +static void __devinit quirk_release_resources(struct pci_dev *dev)
> > +{
> > + int i;
> > + struct resource *r;
> > +
> > + if (is_reassigndev(dev)) {
>
> If you make this "if (!is_reassign...) return;" you don't need to indent the
> rest of this function.
I will use "if (!is_reassign...) return;" and remove indent.
> > + if (dev->hdr_type == PCI_HEADER_TYPE_NORMAL &&
> > + (dev->class >> 8) == PCI_CLASS_BRIDGE_HOST) {
> > + /* PCI Host Bridge isn't a target device */
> > + return;
> > + }
>
> Maybe print a message in this case warning the user that they tried to do
> something invalid? (Nit: { } around one line conditionals aren't necessary,
> there are a few of these.)
I will print a message.
> > + printk(KERN_INFO
> > + "PCI: Disable device and release resources [%s].\n",
> > + pci_name(dev));
> > + pci_disable_device(dev);
>
> I suppose this is safer to disable the device, but fwiw most of the other
> reassignment quirks don't bother to disable the device first.
I will keep this, because I'd like to prevent device from decoding
address 0.
> > +
> > + for (i=0; i < PCI_NUM_RESOURCES; i++) {
> > + r = &dev->resource[i];
> > + if (!(r->flags & IORESOURCE_MEM))
> > + continue;
> > +
> > + r->end = r->end - r->start;
> > + r->start = 0;
>
> Do you also want to make sure the size is at least a page here? Otherwise the
> page you assign to the guest might contain another device too, right? Also,
> increasing the size to at least a page here should make the other alignment
> checks in setup-bus.c and setup-res.c unnecessary, since size alignment is the
> default...
That's right.
But, if I make sure the size is at least a page (make sure "r->end -
r->start >= PAGE_SIZE"), then /proc/iomem and
/sys/bus/pci/devices/dddd:bb:dd.f/resource will not show real size.
I can make sure no other resources fall into the same page,
specifying device which have small resource, with "pci=pagealignmem=".
So I'd like to keep this.
>
> > #ifdef CONFIG_PCI_QUIRKS
> > /* The Mellanox Tavor device gives false positive parity errors
> > * Mark this device with a broken_parity_status, to allow
> > diff --git a/drivers/pci/reassigndev.c b/drivers/pci/reassigndev.c
> > new file mode 100644
> > index 0000000..ea19481
> > --- /dev/null
> > +++ b/drivers/pci/reassigndev.c
>
> You can just put this code into pci.c. And see above about including as part
> of pci= etc.
I will move this code into pci.c.
Thanks,
--
Yuji Shimada
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
