[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



Jeremy Fitzhardinge wrote:
> 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.
> 

Ok.

> 
>> +            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)
> 

Ok. will use the variable declaration to initialize it.

>> +            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?
> 

register_xen_pci_notifier must be executed after PCI subsystem. I think 
fs_initcall is the suitable initcall achieve it. 

Regards,
Weidong

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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.