Anthony Liguori
2013-Jun-20 12:17 UTC
Re: [Qemu-devel] [PATCH] Add Xen platform PCI device version 2.
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
Paul Durrant
2013-Jun-20 12:44 UTC
Re: [Qemu-devel] [PATCH] Add Xen platform PCI device version 2.
> -----Original Message----- > 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. >Anthony, I''m going to take a different approach so please disregard this patch now. Paul