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?
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.
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).
> 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.
> +/*
> + * 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.
> + 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.)
> + 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.
> +
> + 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...
> #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.
Thanks,
Jesse
--
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
