Re: [Qemu-devel] [PATCH] Add Xen platform PCI device version 2.

[Anthony Liguori <anthony@codemonkey.ws>]

Paul Durrant <paul.durrant@citrix.com> writes:

> The XenServer 6.1+ Citrix Windows PV bus driver binds to a new version
> of the Xen platform device (since the newer driver set cannot co-exist
> with previous drivers which bind to the existing "xen-platform" type of
> device). This patch introduces a new "xen-platform-2" device type with
> the appropriate device_id and revision.
>
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> ---
>  hw/xen/xen_platform.c    |   75 ++++++++++++++++++++++++++++++++++++++--------
>  include/hw/pci/pci_ids.h |    1 +
>  2 files changed, 63 insertions(+), 13 deletions(-)
>
> diff --git a/hw/xen/xen_platform.c b/hw/xen/xen_platform.c
> index b6c6793..6edb850 100644
> --- a/hw/xen/xen_platform.c
> +++ b/hw/xen/xen_platform.c
> @@ -48,6 +48,20 @@
>  
>  #define PFFLAG_ROM_LOCK 1 /* Sets whether ROM memory area is RW or RO */
>  
> +typedef struct {
> +    const char *name;
> +    const char *desc;
> +    uint16_t device_id;
> +    uint8_t revision;
> +    uint16_t subsystem_vendor_id;
> +    uint16_t subsystem_id;
> +} PCIXenPlatformDeviceInfo;
> +
> +typedef struct PCIXenPlatformDeviceClass {
> +    PCIDeviceClass              parent_class;
> +    PCIXenPlatformDeviceInfo    info;
> +} PCIXenPlatformDeviceClass;
> +
>  typedef struct PCIXenPlatformState {
>      PCIDevice  pci_dev;
>      MemoryRegion fixed_io;
> @@ -372,8 +386,13 @@ static const VMStateDescription vmstate_xen_platform = {
>  static int xen_platform_initfn(PCIDevice *dev)
>  {
>      PCIXenPlatformState *d = DO_UPCAST(PCIXenPlatformState, pci_dev, dev);
> +    PCIDeviceClass *k = PCI_DEVICE_GET_CLASS(dev);
> +    __attribute__((unused)) PCIXenPlatformDeviceClass *u;
>      uint8_t *pci_conf;
>  
> +    u = container_of(k, PCIXenPlatformDeviceClass, parent_class);
> +    DPRINTF("initializing %s\n", u->info.name);
> +
>      pci_conf = d->pci_dev.config;
>  
>      pci_set_word(pci_conf + PCI_COMMAND, PCI_COMMAND_IO | PCI_COMMAND_MEMORY);
> @@ -402,33 +421,63 @@ static void platform_reset(DeviceState *dev)
>      platform_fixed_ioport_reset(s);
>  }
>  
> +static PCIXenPlatformDeviceInfo platform_devices[] = {
> +    {
> +        .name = "xen-platform",
> +        .desc = "XEN platform pci device (version 1)",
> +        .device_id = PCI_DEVICE_ID_XEN_PLATFORM,
> +        .revision = 1,
> +        .subsystem_vendor_id = PCI_VENDOR_ID_XEN,
> +        .subsystem_id = PCI_DEVICE_ID_XEN_PLATFORM,
> +    }, {
> +        .name = "xen-platform-2",
> +        .desc = "XEN platform pci device (version 2)",
> +        .device_id = PCI_DEVICE_ID_XEN_PLATFORM_V2,
> +        .revision = 2,
> +        .subsystem_vendor_id = PCI_VENDOR_ID_XEN,
> +        .subsystem_id = PCI_DEVICE_ID_XEN_PLATFORM_V2,
> +    }
> +};
> +
>  static void xen_platform_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> +    PCIXenPlatformDeviceInfo *info = data;
> +    PCIXenPlatformDeviceClass *u;
> +
> +    u = container_of(k, PCIXenPlatformDeviceClass, parent_class);
>  
>      k->init = xen_platform_initfn;
>      k->vendor_id = PCI_VENDOR_ID_XEN;
> -    k->device_id = PCI_DEVICE_ID_XEN_PLATFORM;
> +    k->device_id = info->device_id;
>      k->class_id = PCI_CLASS_OTHERS << 8 | 0x80;
> -    k->subsystem_vendor_id = PCI_VENDOR_ID_XEN;
> -    k->subsystem_id = PCI_DEVICE_ID_XEN_PLATFORM;
> -    k->revision = 1;
> -    dc->desc = "XEN platform pci device";
> +    k->subsystem_vendor_id = info->subsystem_vendor_id;
> +    k->subsystem_id = info->subsystem_id;
> +    k->revision = info->revision;
> +    dc->desc = info->desc;
>      dc->reset = platform_reset;
>      dc->vmsd = &vmstate_xen_platform;
> +    u->info = *info;
>  }
>  
> -static const TypeInfo xen_platform_info = {
> -    .name          = "xen-platform",
> -    .parent        = TYPE_PCI_DEVICE,
> -    .instance_size = sizeof(PCIXenPlatformState),
> -    .class_init    = xen_platform_class_init,
> -};
> -
>  static void xen_platform_register_types(void)
>  {
> -    type_register_static(&xen_platform_info);
> +    TypeInfo type_info = {
> +        .parent        = TYPE_PCI_DEVICE,
> +        .instance_size = sizeof(PCIXenPlatformState),
> +        .class_size    = sizeof(PCIXenPlatformDeviceClass),
> +        .class_init    = xen_platform_class_init,
> +    };
> +    int i;
> +    for (i = 0; i < ARRAY_SIZE(platform_devices); i++) {
> +        PCIXenPlatformDeviceInfo *info = &platform_devices[i];
> +
> +        type_info.name = info->name;
> +        type_info.class_data = info;
> +        
> +        type_register(&type_info);
> +    }

I can't tell if this is an RFC or meant a complete patch.  But the
approach you're taking is overly complex.  v2 of the device can just
derive from v1 and in the class_init function change the PCI
information.

Also, if you are going to be adding logic for v2, you should use QOM
cast macros, not container_of.

I don't understand why two devices are required here and the thread
doesn't really answer that either.  Is there a spec for the Xen platform
devices?  Take a look at docs/specs for some examples in the tree.

It certainly helps to have one for discussions like this.

Regards,

Anthony Liguori

>  }
>  
>  type_init(xen_platform_register_types)
> diff --git a/include/hw/pci/pci_ids.h b/include/hw/pci/pci_ids.h
> index d8dc2f1..2039fba 100644
> --- a/include/hw/pci/pci_ids.h
> +++ b/include/hw/pci/pci_ids.h
> @@ -144,6 +144,7 @@
>  
>  #define PCI_VENDOR_ID_XEN               0x5853
>  #define PCI_DEVICE_ID_XEN_PLATFORM      0x0001
> +#define PCI_DEVICE_ID_XEN_PLATFORM_V2   0x0002
>  
>  #define PCI_VENDOR_ID_NEC                0x1033
>  #define PCI_DEVICE_ID_NEC_UPD720200      0x0194
> -- 
> 1.7.10.4