WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

[Xen-devel] Re: [PATCH] pv-ops: register xen pci notifier

To: Weidong Han <weidong.han@xxxxxxxxx>
Subject: [Xen-devel] Re: [PATCH] pv-ops: register xen pci notifier
From: Jeremy Fitzhardinge <jeremy@xxxxxxxx>
Date: Tue, 28 Jul 2009 11:11:32 -0700
Cc: xen-devel@xxxxxxxxxxxxxxxxxxx, linux-pci@xxxxxxxxxxxxxxx, yunhong.jiang@xxxxxxxxx, allen.m.kay@xxxxxxxxx, jbarnes@xxxxxxxxxxxxxxxx, Keir.Fraser@xxxxxxxxxxxxx
Delivery-date: Tue, 28 Jul 2009 11:12:00 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <1248659808-22198-1-git-send-email-weidong.han@xxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <1248659808-22198-1-git-send-email-weidong.han@xxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1b3pre) Gecko/20090513 Fedora/3.0-2.3.beta2.fc11 Lightning/1.0pre Thunderbird/3.0b2
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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel