On 07/26/09 18:56, Weidong Han wrote:
> Register the notifier to handle hot-plug devices and SR-IOV devices
> for Xen hypervisor. When a device is hot added or removed, it adds
> or removes it to Xen via hypercalls.
>
Looks pretty good. A few small comments below.
J
> Signed-off-by: Weidong Han <weidong.han@xxxxxxxxx>
> ---
> drivers/xen/Makefile | 5 +-
> drivers/xen/pci.c | 105 +++++++++++++++++++++++++++++++++++++++
> include/xen/interface/physdev.h | 21 ++++++++
> 3 files changed, 129 insertions(+), 2 deletions(-)
> create mode 100644 drivers/xen/pci.c
>
> diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile
> index 007aa99..fd5c88c 100644
> --- a/drivers/xen/Makefile
> +++ b/drivers/xen/Makefile
> @@ -1,12 +1,13 @@
> obj-y += grant-table.o features.o events.o manage.o biomerge.o
> obj-y += xenbus/
>
> +obj-$(CONFIG_PCI) += pci.o
> obj-$(CONFIG_HOTPLUG_CPU) += cpu_hotplug.o
> obj-$(CONFIG_XEN_XENCOMM) += xencomm.o
> obj-$(CONFIG_XEN_BALLOON) += balloon.o
> obj-$(CONFIG_XEN_DEV_EVTCHN) += evtchn.o
> -obj-$(CONFIG_XEN_GNTDEV) += gntdev.o
> +obj-$(CONFIG_XEN_GNTDEV) += gntdev.o
> obj-$(CONFIG_XEN_BLKDEV_BACKEND) += blkback/
> obj-$(CONFIG_XEN_NETDEV_BACKEND) += netback/
> obj-$(CONFIG_XENFS) += xenfs/
> -obj-$(CONFIG_XEN_SYS_HYPERVISOR) += sys-hypervisor.o
> \ No newline at end of file
> +obj-$(CONFIG_XEN_SYS_HYPERVISOR) += sys-hypervisor.o
> diff --git a/drivers/xen/pci.c b/drivers/xen/pci.c
> new file mode 100644
> index 0000000..b1261d4
> --- /dev/null
> +++ b/drivers/xen/pci.c
> @@ -0,0 +1,105 @@
> +/*
> + * Copyright (c) 2009, Intel Corporation.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program; if not, write to the Free Software Foundation, Inc., 59 Temple
> + * Place - Suite 330, Boston, MA 02111-1307 USA.
> + *
> + * Author: Weidong Han <weidong.han@xxxxxxxxx>
> + */
> +
> +#include <linux/pci.h>
> +#include <xen/interface/physdev.h>
> +#include <asm/xen/hypercall.h>
> +#include "../pci/pci.h"
> +
> +static int xen_add_device(struct device *dev)
> +{
> + int r;
> + struct pci_dev *pci_dev = to_pci_dev(dev);
> + struct physdev_manage_pci manage_pci;
> + struct physdev_manage_pci_ext manage_pci_ext;
> +
> +#ifdef CONFIG_PCI_IOV
> + if (pci_dev->is_virtfn) {
>
Perhaps something like:
(earlier in the file)
#ifdef CONFIG_PCI_IOV
#define HANDLE_PCI_IOV 1
#else
#define HANDLE_PCI_IOV 0
#endif
(or is there already something we can use as a compile-time constant for this?)
and then:
if (HANDLE_PCI_IOV && pci_dev->is_virtfn) {
...
} else if (pci_ari_enabled( ... )) {
...
} else {
...
}
That removes the inline #ifdef and the awkward dangling else/#endif
construction.
> + memset(&manage_pci_ext, 0, sizeof(manage_pci_ext));
>
Rather than doing this, move the variable decl into this scope and use
an initializer to assign the elements:
struct physdev_manage_pci_ext manage_pci_ext = {
.bus = pci_dev->bus->number,
.devfn = pci_dev->devfn,
...
};
(ditto for the others)
> + manage_pci_ext.bus = pci_dev->bus->number;
> + manage_pci_ext.devfn = pci_dev->devfn;
> + manage_pci_ext.is_virtfn = 1;
> + manage_pci_ext.physfn.bus = pci_dev->physfn->bus->number;
> + manage_pci_ext.physfn.devfn = pci_dev->physfn->devfn;
> + r = HYPERVISOR_physdev_op(PHYSDEVOP_manage_pci_add_ext,
> + &manage_pci_ext);
> + } else
> +#endif
>
> + if (pci_ari_enabled(pci_dev->bus) && PCI_SLOT(pci_dev->devfn)) {
> + memset(&manage_pci_ext, 0, sizeof(manage_pci_ext));
> + manage_pci_ext.bus = pci_dev->bus->number;
> + manage_pci_ext.devfn = pci_dev->devfn;
> + manage_pci_ext.is_extfn = 1;
> + r = HYPERVISOR_physdev_op(PHYSDEVOP_manage_pci_add_ext,
> + &manage_pci_ext);
> + } else {
> + manage_pci.bus = pci_dev->bus->number;
> + manage_pci.devfn = pci_dev->devfn;
> + r = HYPERVISOR_physdev_op(PHYSDEVOP_manage_pci_add,
> + &manage_pci);
> + }
> +
>
> + return r;
> +}
> +
> +static int xen_remove_device(struct device *dev)
> +{
> + int r;
> + struct pci_dev *pci_dev = to_pci_dev(dev);
> + struct physdev_manage_pci manage_pci;
> +
> + manage_pci.bus = pci_dev->bus->number;
> + manage_pci.devfn = pci_dev->devfn;
> +
> + r = HYPERVISOR_physdev_op(PHYSDEVOP_manage_pci_remove,
> + &manage_pci);
> +
> + return r;
> +}
> +
> +static int xen_pci_notifier(struct notifier_block *nb,
> + unsigned long action, void *data)
> +{
> + struct device *dev = data;
> + int r = 0;
> +
> + switch (action) {
> + case BUS_NOTIFY_ADD_DEVICE:
> + r = xen_add_device(dev);
> + break;
> + case BUS_NOTIFY_DEL_DEVICE:
> + r = xen_remove_device(dev);
> + break;
> + default:
> + break;
> + }
> +
> + return r;
> +}
> +
> +struct notifier_block device_nb = {
> + .notifier_call = xen_pci_notifier,
> +};
> +
> +void __init register_xen_pci_notifier(void)
> +{
> + bus_register_notifier(&pci_bus_type, &device_nb);
> +}
> +
> +fs_initcall(register_xen_pci_notifier);
>
Why fs_initcall? Is that the conventional initcall for registering
these kinds of notifiers?
Thanks,
J
--
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
