| Message ID | 20230104144437.27479-7-shentey@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Resolve TYPE_PIIX3_XEN_DEVICE | expand |
+Markus/Thomas On 4/1/23 15:44, Bernhard Beschow wrote: > During the last patches, TYPE_PIIX3_XEN_DEVICE turned into a clone of > TYPE_PIIX3_DEVICE. Remove this redundancy. > > Signed-off-by: Bernhard Beschow <shentey@gmail.com> > --- > hw/i386/pc_piix.c | 4 +--- > hw/isa/piix.c | 20 -------------------- > include/hw/southbridge/piix.h | 1 - > 3 files changed, 1 insertion(+), 24 deletions(-) > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > index 5738d9cdca..6b8de3d59d 100644 > --- a/hw/i386/pc_piix.c > +++ b/hw/i386/pc_piix.c > @@ -235,8 +235,6 @@ static void pc_init1(MachineState *machine, > if (pcmc->pci_enabled) { > DeviceState *dev; > PCIDevice *pci_dev; > - const char *type = xen_enabled() ? TYPE_PIIX3_XEN_DEVICE > - : TYPE_PIIX3_DEVICE; > int i; > > pci_bus = i440fx_init(pci_type, > @@ -250,7 +248,7 @@ static void pc_init1(MachineState *machine, > : pci_slot_get_pirq); > pcms->bus = pci_bus; > > - pci_dev = pci_new_multifunction(-1, true, type); > + pci_dev = pci_new_multifunction(-1, true, TYPE_PIIX3_DEVICE); > object_property_set_bool(OBJECT(pci_dev), "has-usb", > machine_usb(machine), &error_abort); > object_property_set_bool(OBJECT(pci_dev), "has-acpi", > diff --git a/hw/isa/piix.c b/hw/isa/piix.c > index 98e9b12661..e4587352c9 100644 > --- a/hw/isa/piix.c > +++ b/hw/isa/piix.c > @@ -33,7 +33,6 @@ > #include "hw/qdev-properties.h" > #include "hw/ide/piix.h" > #include "hw/isa/isa.h" > -#include "hw/xen/xen.h" > #include "sysemu/runstate.h" > #include "migration/vmstate.h" > #include "hw/acpi/acpi_aml_interface.h" > @@ -465,24 +464,6 @@ static const TypeInfo piix3_info = { > .class_init = piix3_class_init, > }; > > -static void piix3_xen_class_init(ObjectClass *klass, void *data) > -{ > - DeviceClass *dc = DEVICE_CLASS(klass); > - PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); > - > - k->realize = piix3_realize; > - /* 82371SB PIIX3 PCI-to-ISA bridge (Step A1) */ > - k->device_id = PCI_DEVICE_ID_INTEL_82371SB_0; > - dc->vmsd = &vmstate_piix3; IIUC, since this device is user-creatable, we can't simply remove it without going thru the deprecation process. Alternatively we could add a type alias: -- >8 -- diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c index 4b0ef65780..d94f7ea369 100644 --- a/softmmu/qdev-monitor.c +++ b/softmmu/qdev-monitor.c @@ -64,6 +64,7 @@ typedef struct QDevAlias QEMU_ARCH_LOONGARCH) #define QEMU_ARCH_VIRTIO_CCW (QEMU_ARCH_S390X) #define QEMU_ARCH_VIRTIO_MMIO (QEMU_ARCH_M68K) +#define QEMU_ARCH_XEN (QEMU_ARCH_ARM | QEMU_ARCH_I386) /* Please keep this table sorted by typename. */ static const QDevAlias qdev_alias_table[] = { @@ -111,6 +112,7 @@ static const QDevAlias qdev_alias_table[] = { { "virtio-tablet-device", "virtio-tablet", QEMU_ARCH_VIRTIO_MMIO }, { "virtio-tablet-ccw", "virtio-tablet", QEMU_ARCH_VIRTIO_CCW }, { "virtio-tablet-pci", "virtio-tablet", QEMU_ARCH_VIRTIO_PCI }, + { "PIIX3", "PIIX3-xen", QEMU_ARCH_XEN }, { } }; --- But I'm not sure due to this comment from commit ee46d8a503 (2011-12-22 15:24:20 -0600): 47) /* 48) * Aliases were a bad idea from the start. Let's keep them 49) * from spreading further. 50) */ Maybe using qdev_alias_table[] during device deprecation is acceptable? > -} > - > -static const TypeInfo piix3_xen_info = { > - .name = TYPE_PIIX3_XEN_DEVICE, > - .parent = TYPE_PIIX_PCI_DEVICE, > - .instance_init = piix3_init, > - .class_init = piix3_xen_class_init, > -}; > - > static void piix4_realize(PCIDevice *dev, Error **errp) > { > ERRP_GUARD(); > @@ -534,7 +515,6 @@ static void piix3_register_types(void) > { > type_register_static(&piix_pci_type_info); > type_register_static(&piix3_info); > - type_register_static(&piix3_xen_info); > type_register_static(&piix4_info); > } > > diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h > index 65ad8569da..b1fc94a742 100644 > --- a/include/hw/southbridge/piix.h > +++ b/include/hw/southbridge/piix.h > @@ -77,7 +77,6 @@ struct PIIXState { > OBJECT_DECLARE_SIMPLE_TYPE(PIIXState, PIIX_PCI_DEVICE) > > #define TYPE_PIIX3_DEVICE "PIIX3" > -#define TYPE_PIIX3_XEN_DEVICE "PIIX3-xen" > #define TYPE_PIIX4_PCI_DEVICE "piix4-isa" > > #endif
On 1/4/23 9:44 AM, Bernhard Beschow wrote: > During the last patches, TYPE_PIIX3_XEN_DEVICE turned into a clone of > TYPE_PIIX3_DEVICE. Remove this redundancy. > > Signed-off-by: Bernhard Beschow <shentey@gmail.com> > --- > hw/i386/pc_piix.c | 4 +--- > hw/isa/piix.c | 20 -------------------- > include/hw/southbridge/piix.h | 1 - > 3 files changed, 1 insertion(+), 24 deletions(-) > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > index 5738d9cdca..6b8de3d59d 100644 > --- a/hw/i386/pc_piix.c > +++ b/hw/i386/pc_piix.c > @@ -235,8 +235,6 @@ static void pc_init1(MachineState *machine, > if (pcmc->pci_enabled) { > DeviceState *dev; > PCIDevice *pci_dev; > - const char *type = xen_enabled() ? TYPE_PIIX3_XEN_DEVICE > - : TYPE_PIIX3_DEVICE; > int i; > > pci_bus = i440fx_init(pci_type, > @@ -250,7 +248,7 @@ static void pc_init1(MachineState *machine, > : pci_slot_get_pirq); > pcms->bus = pci_bus; > > - pci_dev = pci_new_multifunction(-1, true, type); > + pci_dev = pci_new_multifunction(-1, true, TYPE_PIIX3_DEVICE); > object_property_set_bool(OBJECT(pci_dev), "has-usb", > machine_usb(machine), &error_abort); > object_property_set_bool(OBJECT(pci_dev), "has-acpi", > diff --git a/hw/isa/piix.c b/hw/isa/piix.c > index 98e9b12661..e4587352c9 100644 > --- a/hw/isa/piix.c > +++ b/hw/isa/piix.c > @@ -33,7 +33,6 @@ > #include "hw/qdev-properties.h" > #include "hw/ide/piix.h" > #include "hw/isa/isa.h" > -#include "hw/xen/xen.h" > #include "sysemu/runstate.h" > #include "migration/vmstate.h" > #include "hw/acpi/acpi_aml_interface.h" > @@ -465,24 +464,6 @@ static const TypeInfo piix3_info = { > .class_init = piix3_class_init, > }; > > -static void piix3_xen_class_init(ObjectClass *klass, void *data) > -{ > - DeviceClass *dc = DEVICE_CLASS(klass); > - PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); > - > - k->realize = piix3_realize; > - /* 82371SB PIIX3 PCI-to-ISA bridge (Step A1) */ > - k->device_id = PCI_DEVICE_ID_INTEL_82371SB_0; > - dc->vmsd = &vmstate_piix3; > -} > - > -static const TypeInfo piix3_xen_info = { > - .name = TYPE_PIIX3_XEN_DEVICE, > - .parent = TYPE_PIIX_PCI_DEVICE, > - .instance_init = piix3_init, > - .class_init = piix3_xen_class_init, > -}; > - > static void piix4_realize(PCIDevice *dev, Error **errp) > { > ERRP_GUARD(); > @@ -534,7 +515,6 @@ static void piix3_register_types(void) > { > type_register_static(&piix_pci_type_info); > type_register_static(&piix3_info); > - type_register_static(&piix3_xen_info); > type_register_static(&piix4_info); > } > > diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h > index 65ad8569da..b1fc94a742 100644 > --- a/include/hw/southbridge/piix.h > +++ b/include/hw/southbridge/piix.h > @@ -77,7 +77,6 @@ struct PIIXState { > OBJECT_DECLARE_SIMPLE_TYPE(PIIXState, PIIX_PCI_DEVICE) > > #define TYPE_PIIX3_DEVICE "PIIX3" > -#define TYPE_PIIX3_XEN_DEVICE "PIIX3-xen" > #define TYPE_PIIX4_PCI_DEVICE "piix4-isa" > > #endif This fixes the regression with the emulated usb tablet device that I reported in v1 here: https://lore.kernel.org/qemu-devel/aed4f2c1-83f7-163a-fb44-f284376668dc@aol.com/ I tested this patch again with all the prerequisites and now with v2 there are no regressions. Tested-by: Chuck Zmudzinski <brchuckz@aol.com>
On 1/4/23 10:35 AM, Philippe Mathieu-Daudé wrote: > +Markus/Thomas > > On 4/1/23 15:44, Bernhard Beschow wrote: >> During the last patches, TYPE_PIIX3_XEN_DEVICE turned into a clone of >> TYPE_PIIX3_DEVICE. Remove this redundancy. >> >> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >> --- >> hw/i386/pc_piix.c | 4 +--- >> hw/isa/piix.c | 20 -------------------- >> include/hw/southbridge/piix.h | 1 - >> 3 files changed, 1 insertion(+), 24 deletions(-) >> >> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c >> index 5738d9cdca..6b8de3d59d 100644 >> --- a/hw/i386/pc_piix.c >> +++ b/hw/i386/pc_piix.c >> @@ -235,8 +235,6 @@ static void pc_init1(MachineState *machine, >> if (pcmc->pci_enabled) { >> DeviceState *dev; >> PCIDevice *pci_dev; >> - const char *type = xen_enabled() ? TYPE_PIIX3_XEN_DEVICE >> - : TYPE_PIIX3_DEVICE; >> int i; >> >> pci_bus = i440fx_init(pci_type, >> @@ -250,7 +248,7 @@ static void pc_init1(MachineState *machine, >> : pci_slot_get_pirq); >> pcms->bus = pci_bus; >> >> - pci_dev = pci_new_multifunction(-1, true, type); >> + pci_dev = pci_new_multifunction(-1, true, TYPE_PIIX3_DEVICE); >> object_property_set_bool(OBJECT(pci_dev), "has-usb", >> machine_usb(machine), &error_abort); >> object_property_set_bool(OBJECT(pci_dev), "has-acpi", >> diff --git a/hw/isa/piix.c b/hw/isa/piix.c >> index 98e9b12661..e4587352c9 100644 >> --- a/hw/isa/piix.c >> +++ b/hw/isa/piix.c >> @@ -33,7 +33,6 @@ >> #include "hw/qdev-properties.h" >> #include "hw/ide/piix.h" >> #include "hw/isa/isa.h" >> -#include "hw/xen/xen.h" >> #include "sysemu/runstate.h" >> #include "migration/vmstate.h" >> #include "hw/acpi/acpi_aml_interface.h" >> @@ -465,24 +464,6 @@ static const TypeInfo piix3_info = { >> .class_init = piix3_class_init, >> }; >> >> -static void piix3_xen_class_init(ObjectClass *klass, void *data) >> -{ >> - DeviceClass *dc = DEVICE_CLASS(klass); >> - PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); >> - >> - k->realize = piix3_realize; >> - /* 82371SB PIIX3 PCI-to-ISA bridge (Step A1) */ >> - k->device_id = PCI_DEVICE_ID_INTEL_82371SB_0; >> - dc->vmsd = &vmstate_piix3; > > IIUC, since this device is user-creatable, we can't simply remove it > without going thru the deprecation process. Alternatively we could > add a type alias: > > -- >8 -- > diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c > index 4b0ef65780..d94f7ea369 100644 > --- a/softmmu/qdev-monitor.c > +++ b/softmmu/qdev-monitor.c > @@ -64,6 +64,7 @@ typedef struct QDevAlias > QEMU_ARCH_LOONGARCH) > #define QEMU_ARCH_VIRTIO_CCW (QEMU_ARCH_S390X) > #define QEMU_ARCH_VIRTIO_MMIO (QEMU_ARCH_M68K) > +#define QEMU_ARCH_XEN (QEMU_ARCH_ARM | QEMU_ARCH_I386) > > /* Please keep this table sorted by typename. */ > static const QDevAlias qdev_alias_table[] = { > @@ -111,6 +112,7 @@ static const QDevAlias qdev_alias_table[] = { > { "virtio-tablet-device", "virtio-tablet", QEMU_ARCH_VIRTIO_MMIO }, > { "virtio-tablet-ccw", "virtio-tablet", QEMU_ARCH_VIRTIO_CCW }, > { "virtio-tablet-pci", "virtio-tablet", QEMU_ARCH_VIRTIO_PCI }, > + { "PIIX3", "PIIX3-xen", QEMU_ARCH_XEN }, Hi Bernhard, Can you comment if this should be: + { "PIIX", "PIIX3-xen", QEMU_ARCH_XEN }, instead? IIUC, the patch series also removed PIIX3 and PIIX4 and replaced them with PIIX. Or am I not understanding correctly? Best regards, Chuck > { } > }; > --- > > But I'm not sure due to this comment from commit ee46d8a503 > (2011-12-22 15:24:20 -0600): > > 47) /* > 48) * Aliases were a bad idea from the start. Let's keep them > 49) * from spreading further. > 50) */ > > Maybe using qdev_alias_table[] during device deprecation is > acceptable? > >> -} >> - >> -static const TypeInfo piix3_xen_info = { >> - .name = TYPE_PIIX3_XEN_DEVICE, >> - .parent = TYPE_PIIX_PCI_DEVICE, >> - .instance_init = piix3_init, >> - .class_init = piix3_xen_class_init, >> -}; >> - >> static void piix4_realize(PCIDevice *dev, Error **errp) >> { >> ERRP_GUARD(); >> @@ -534,7 +515,6 @@ static void piix3_register_types(void) >> { >> type_register_static(&piix_pci_type_info); >> type_register_static(&piix3_info); >> - type_register_static(&piix3_xen_info); >> type_register_static(&piix4_info); >> } >> >> diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h >> index 65ad8569da..b1fc94a742 100644 >> --- a/include/hw/southbridge/piix.h >> +++ b/include/hw/southbridge/piix.h >> @@ -77,7 +77,6 @@ struct PIIXState { >> OBJECT_DECLARE_SIMPLE_TYPE(PIIXState, PIIX_PCI_DEVICE) >> >> #define TYPE_PIIX3_DEVICE "PIIX3" >> -#define TYPE_PIIX3_XEN_DEVICE "PIIX3-xen" >> #define TYPE_PIIX4_PCI_DEVICE "piix4-isa" >> >> #endif >
On 4/1/23 18:54, Chuck Zmudzinski wrote: > On 1/4/23 10:35 AM, Philippe Mathieu-Daudé wrote: >> +Markus/Thomas >> >> On 4/1/23 15:44, Bernhard Beschow wrote: >>> During the last patches, TYPE_PIIX3_XEN_DEVICE turned into a clone of >>> TYPE_PIIX3_DEVICE. Remove this redundancy. >>> >>> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >>> --- >>> hw/i386/pc_piix.c | 4 +--- >>> hw/isa/piix.c | 20 -------------------- >>> include/hw/southbridge/piix.h | 1 - >>> 3 files changed, 1 insertion(+), 24 deletions(-) >>> >>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c >>> index 5738d9cdca..6b8de3d59d 100644 >>> --- a/hw/i386/pc_piix.c >>> +++ b/hw/i386/pc_piix.c >>> @@ -235,8 +235,6 @@ static void pc_init1(MachineState *machine, >>> if (pcmc->pci_enabled) { >>> DeviceState *dev; >>> PCIDevice *pci_dev; >>> - const char *type = xen_enabled() ? TYPE_PIIX3_XEN_DEVICE >>> - : TYPE_PIIX3_DEVICE; >>> int i; >>> >>> pci_bus = i440fx_init(pci_type, >>> @@ -250,7 +248,7 @@ static void pc_init1(MachineState *machine, >>> : pci_slot_get_pirq); >>> pcms->bus = pci_bus; >>> >>> - pci_dev = pci_new_multifunction(-1, true, type); >>> + pci_dev = pci_new_multifunction(-1, true, TYPE_PIIX3_DEVICE); >>> object_property_set_bool(OBJECT(pci_dev), "has-usb", >>> machine_usb(machine), &error_abort); >>> object_property_set_bool(OBJECT(pci_dev), "has-acpi", >>> diff --git a/hw/isa/piix.c b/hw/isa/piix.c >>> index 98e9b12661..e4587352c9 100644 >>> --- a/hw/isa/piix.c >>> +++ b/hw/isa/piix.c >>> @@ -33,7 +33,6 @@ >>> #include "hw/qdev-properties.h" >>> #include "hw/ide/piix.h" >>> #include "hw/isa/isa.h" >>> -#include "hw/xen/xen.h" >>> #include "sysemu/runstate.h" >>> #include "migration/vmstate.h" >>> #include "hw/acpi/acpi_aml_interface.h" >>> @@ -465,24 +464,6 @@ static const TypeInfo piix3_info = { >>> .class_init = piix3_class_init, >>> }; >>> >>> -static void piix3_xen_class_init(ObjectClass *klass, void *data) >>> -{ >>> - DeviceClass *dc = DEVICE_CLASS(klass); >>> - PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); >>> - >>> - k->realize = piix3_realize; >>> - /* 82371SB PIIX3 PCI-to-ISA bridge (Step A1) */ >>> - k->device_id = PCI_DEVICE_ID_INTEL_82371SB_0; >>> - dc->vmsd = &vmstate_piix3; >> >> IIUC, since this device is user-creatable, we can't simply remove it >> without going thru the deprecation process. Alternatively we could >> add a type alias: >> >> -- >8 -- >> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c >> index 4b0ef65780..d94f7ea369 100644 >> --- a/softmmu/qdev-monitor.c >> +++ b/softmmu/qdev-monitor.c >> @@ -64,6 +64,7 @@ typedef struct QDevAlias >> QEMU_ARCH_LOONGARCH) >> #define QEMU_ARCH_VIRTIO_CCW (QEMU_ARCH_S390X) >> #define QEMU_ARCH_VIRTIO_MMIO (QEMU_ARCH_M68K) >> +#define QEMU_ARCH_XEN (QEMU_ARCH_ARM | QEMU_ARCH_I386) >> >> /* Please keep this table sorted by typename. */ >> static const QDevAlias qdev_alias_table[] = { >> @@ -111,6 +112,7 @@ static const QDevAlias qdev_alias_table[] = { >> { "virtio-tablet-device", "virtio-tablet", QEMU_ARCH_VIRTIO_MMIO }, >> { "virtio-tablet-ccw", "virtio-tablet", QEMU_ARCH_VIRTIO_CCW }, >> { "virtio-tablet-pci", "virtio-tablet", QEMU_ARCH_VIRTIO_PCI }, >> + { "PIIX3", "PIIX3-xen", QEMU_ARCH_XEN }, > > Hi Bernhard, > > Can you comment if this should be: > > + { "PIIX", "PIIX3-xen", QEMU_ARCH_XEN }, > > instead? IIUC, the patch series also removed PIIX3 and PIIX4 and > replaced them with PIIX. Or am I not understanding correctly? There is a confusion in QEMU between PCI bridges, the first PCI function they implement, and the other PCI functions. Here TYPE_PIIX3_DEVICE means for "PCI function part of the PIIX south bridge chipset, which expose a PCI-to-ISA bridge". A better name could be TYPE_PIIX3_ISA_PCI_DEVICE. Unfortunately this device is named "PIIX3" with no indication of ISA bridge.
On 1/4/23 1:48 PM, Philippe Mathieu-Daudé wrote: > On 4/1/23 18:54, Chuck Zmudzinski wrote: >> On 1/4/23 10:35 AM, Philippe Mathieu-Daudé wrote: >>> +Markus/Thomas >>> >>> On 4/1/23 15:44, Bernhard Beschow wrote: >>>> During the last patches, TYPE_PIIX3_XEN_DEVICE turned into a clone of >>>> TYPE_PIIX3_DEVICE. Remove this redundancy. >>>> >>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >>>> --- >>>> hw/i386/pc_piix.c | 4 +--- >>>> hw/isa/piix.c | 20 -------------------- >>>> include/hw/southbridge/piix.h | 1 - >>>> 3 files changed, 1 insertion(+), 24 deletions(-) >>>> >>>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c >>>> index 5738d9cdca..6b8de3d59d 100644 >>>> --- a/hw/i386/pc_piix.c >>>> +++ b/hw/i386/pc_piix.c >>>> @@ -235,8 +235,6 @@ static void pc_init1(MachineState *machine, >>>> if (pcmc->pci_enabled) { >>>> DeviceState *dev; >>>> PCIDevice *pci_dev; >>>> - const char *type = xen_enabled() ? TYPE_PIIX3_XEN_DEVICE >>>> - : TYPE_PIIX3_DEVICE; >>>> int i; >>>> >>>> pci_bus = i440fx_init(pci_type, >>>> @@ -250,7 +248,7 @@ static void pc_init1(MachineState *machine, >>>> : pci_slot_get_pirq); >>>> pcms->bus = pci_bus; >>>> >>>> - pci_dev = pci_new_multifunction(-1, true, type); >>>> + pci_dev = pci_new_multifunction(-1, true, TYPE_PIIX3_DEVICE); >>>> object_property_set_bool(OBJECT(pci_dev), "has-usb", >>>> machine_usb(machine), &error_abort); >>>> object_property_set_bool(OBJECT(pci_dev), "has-acpi", >>>> diff --git a/hw/isa/piix.c b/hw/isa/piix.c >>>> index 98e9b12661..e4587352c9 100644 >>>> --- a/hw/isa/piix.c >>>> +++ b/hw/isa/piix.c >>>> @@ -33,7 +33,6 @@ >>>> #include "hw/qdev-properties.h" >>>> #include "hw/ide/piix.h" >>>> #include "hw/isa/isa.h" >>>> -#include "hw/xen/xen.h" >>>> #include "sysemu/runstate.h" >>>> #include "migration/vmstate.h" >>>> #include "hw/acpi/acpi_aml_interface.h" >>>> @@ -465,24 +464,6 @@ static const TypeInfo piix3_info = { >>>> .class_init = piix3_class_init, >>>> }; >>>> >>>> -static void piix3_xen_class_init(ObjectClass *klass, void *data) >>>> -{ >>>> - DeviceClass *dc = DEVICE_CLASS(klass); >>>> - PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); >>>> - >>>> - k->realize = piix3_realize; >>>> - /* 82371SB PIIX3 PCI-to-ISA bridge (Step A1) */ >>>> - k->device_id = PCI_DEVICE_ID_INTEL_82371SB_0; >>>> - dc->vmsd = &vmstate_piix3; >>> >>> IIUC, since this device is user-creatable, we can't simply remove it >>> without going thru the deprecation process. Alternatively we could >>> add a type alias: >>> >>> -- >8 -- >>> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c >>> index 4b0ef65780..d94f7ea369 100644 >>> --- a/softmmu/qdev-monitor.c >>> +++ b/softmmu/qdev-monitor.c >>> @@ -64,6 +64,7 @@ typedef struct QDevAlias >>> QEMU_ARCH_LOONGARCH) >>> #define QEMU_ARCH_VIRTIO_CCW (QEMU_ARCH_S390X) >>> #define QEMU_ARCH_VIRTIO_MMIO (QEMU_ARCH_M68K) >>> +#define QEMU_ARCH_XEN (QEMU_ARCH_ARM | QEMU_ARCH_I386) >>> >>> /* Please keep this table sorted by typename. */ >>> static const QDevAlias qdev_alias_table[] = { >>> @@ -111,6 +112,7 @@ static const QDevAlias qdev_alias_table[] = { >>> { "virtio-tablet-device", "virtio-tablet", QEMU_ARCH_VIRTIO_MMIO }, >>> { "virtio-tablet-ccw", "virtio-tablet", QEMU_ARCH_VIRTIO_CCW }, >>> { "virtio-tablet-pci", "virtio-tablet", QEMU_ARCH_VIRTIO_PCI }, >>> + { "PIIX3", "PIIX3-xen", QEMU_ARCH_XEN }, >> >> Hi Bernhard, >> >> Can you comment if this should be: >> >> + { "PIIX", "PIIX3-xen", QEMU_ARCH_XEN }, >> >> instead? IIUC, the patch series also removed PIIX3 and PIIX4 and >> replaced them with PIIX. Or am I not understanding correctly? > > There is a confusion in QEMU between PCI bridges, the first PCI > function they implement, and the other PCI functions. > > Here TYPE_PIIX3_DEVICE means for "PCI function part of the PIIX > south bridge chipset, which expose a PCI-to-ISA bridge". A better > name could be TYPE_PIIX3_ISA_PCI_DEVICE. Unfortunately this > device is named "PIIX3" with no indication of ISA bridge. Thanks, you are right, I see the PIIX3 device still exists after this patch set is applied. chuckz@debian:~/sources-sid/qemu/qemu-7.50+dfsg/hw/i386$ grep -r PIIX3 * pc_piix.c: pci_dev = pci_new_multifunction(-1, true, TYPE_PIIX3_DEVICE); I also understand there is the PCI-to-ISA bridge at 00:01.0 on the PCI bus: chuckz@debian:~$ lspci 00:00.0 Host bridge: Intel Corporation 440FX - 82441FX PMC [Natoma] (rev 02) 00:01.0 ISA bridge: Intel Corporation 82371SB PIIX3 ISA [Natoma/Triton II] 00:01.1 IDE interface: Intel Corporation 82371SB PIIX3 IDE [Natoma/Triton II] 00:01.2 USB controller: Intel Corporation 82371SB PIIX3 USB [Natoma/Triton II] (rev 01) 00:01.3 Bridge: Intel Corporation 82371AB/EB/MB PIIX4 ACPI (rev 03) 00:02.0 Unassigned class [ff80]: XenSource, Inc. Xen Platform Device (rev 01) 00:03.0 VGA compatible controller: Device 1234:1111 (rev 02) I also see with this patch, there is a bridge that is a PIIX4 ACPI at 00:01.3. I get the exact same output from lspci without the patch series, so that gives me confidence it is working as designed.
Am 4. Januar 2023 16:42:43 UTC schrieb Chuck Zmudzinski <brchuckz@aol.com>: >On 1/4/23 9:44 AM, Bernhard Beschow wrote: >> During the last patches, TYPE_PIIX3_XEN_DEVICE turned into a clone of >> TYPE_PIIX3_DEVICE. Remove this redundancy. >> >> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >> --- >> hw/i386/pc_piix.c | 4 +--- >> hw/isa/piix.c | 20 -------------------- >> include/hw/southbridge/piix.h | 1 - >> 3 files changed, 1 insertion(+), 24 deletions(-) >> >> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c >> index 5738d9cdca..6b8de3d59d 100644 >> --- a/hw/i386/pc_piix.c >> +++ b/hw/i386/pc_piix.c >> @@ -235,8 +235,6 @@ static void pc_init1(MachineState *machine, >> if (pcmc->pci_enabled) { >> DeviceState *dev; >> PCIDevice *pci_dev; >> - const char *type = xen_enabled() ? TYPE_PIIX3_XEN_DEVICE >> - : TYPE_PIIX3_DEVICE; >> int i; >> >> pci_bus = i440fx_init(pci_type, >> @@ -250,7 +248,7 @@ static void pc_init1(MachineState *machine, >> : pci_slot_get_pirq); >> pcms->bus = pci_bus; >> >> - pci_dev = pci_new_multifunction(-1, true, type); >> + pci_dev = pci_new_multifunction(-1, true, TYPE_PIIX3_DEVICE); >> object_property_set_bool(OBJECT(pci_dev), "has-usb", >> machine_usb(machine), &error_abort); >> object_property_set_bool(OBJECT(pci_dev), "has-acpi", >> diff --git a/hw/isa/piix.c b/hw/isa/piix.c >> index 98e9b12661..e4587352c9 100644 >> --- a/hw/isa/piix.c >> +++ b/hw/isa/piix.c >> @@ -33,7 +33,6 @@ >> #include "hw/qdev-properties.h" >> #include "hw/ide/piix.h" >> #include "hw/isa/isa.h" >> -#include "hw/xen/xen.h" >> #include "sysemu/runstate.h" >> #include "migration/vmstate.h" >> #include "hw/acpi/acpi_aml_interface.h" >> @@ -465,24 +464,6 @@ static const TypeInfo piix3_info = { >> .class_init = piix3_class_init, >> }; >> >> -static void piix3_xen_class_init(ObjectClass *klass, void *data) >> -{ >> - DeviceClass *dc = DEVICE_CLASS(klass); >> - PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); >> - >> - k->realize = piix3_realize; >> - /* 82371SB PIIX3 PCI-to-ISA bridge (Step A1) */ >> - k->device_id = PCI_DEVICE_ID_INTEL_82371SB_0; >> - dc->vmsd = &vmstate_piix3; >> -} >> - >> -static const TypeInfo piix3_xen_info = { >> - .name = TYPE_PIIX3_XEN_DEVICE, >> - .parent = TYPE_PIIX_PCI_DEVICE, >> - .instance_init = piix3_init, >> - .class_init = piix3_xen_class_init, >> -}; >> - >> static void piix4_realize(PCIDevice *dev, Error **errp) >> { >> ERRP_GUARD(); >> @@ -534,7 +515,6 @@ static void piix3_register_types(void) >> { >> type_register_static(&piix_pci_type_info); >> type_register_static(&piix3_info); >> - type_register_static(&piix3_xen_info); >> type_register_static(&piix4_info); >> } >> >> diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h >> index 65ad8569da..b1fc94a742 100644 >> --- a/include/hw/southbridge/piix.h >> +++ b/include/hw/southbridge/piix.h >> @@ -77,7 +77,6 @@ struct PIIXState { >> OBJECT_DECLARE_SIMPLE_TYPE(PIIXState, PIIX_PCI_DEVICE) >> >> #define TYPE_PIIX3_DEVICE "PIIX3" >> -#define TYPE_PIIX3_XEN_DEVICE "PIIX3-xen" >> #define TYPE_PIIX4_PCI_DEVICE "piix4-isa" >> >> #endif > > >This fixes the regression with the emulated usb tablet device that I reported in v1 here: > >https://lore.kernel.org/qemu-devel/aed4f2c1-83f7-163a-fb44-f284376668dc@aol.com/ > >I tested this patch again with all the prerequisites and now with v2 there are no regressions. Good news! >Tested-by: Chuck Zmudzinski <brchuckz@aol.com> Thanks for the test ride and the Tested-by medal ;) Best regards, Bernhard
Am 4. Januar 2023 19:29:35 UTC schrieb Chuck Zmudzinski <brchuckz@aol.com>: >On 1/4/23 1:48 PM, Philippe Mathieu-Daudé wrote: >> On 4/1/23 18:54, Chuck Zmudzinski wrote: >>> On 1/4/23 10:35 AM, Philippe Mathieu-Daudé wrote: >>>> +Markus/Thomas >>>> >>>> On 4/1/23 15:44, Bernhard Beschow wrote: >>>>> During the last patches, TYPE_PIIX3_XEN_DEVICE turned into a clone of >>>>> TYPE_PIIX3_DEVICE. Remove this redundancy. >>>>> >>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >>>>> --- >>>>> hw/i386/pc_piix.c | 4 +--- >>>>> hw/isa/piix.c | 20 -------------------- >>>>> include/hw/southbridge/piix.h | 1 - >>>>> 3 files changed, 1 insertion(+), 24 deletions(-) >>>>> >>>>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c >>>>> index 5738d9cdca..6b8de3d59d 100644 >>>>> --- a/hw/i386/pc_piix.c >>>>> +++ b/hw/i386/pc_piix.c >>>>> @@ -235,8 +235,6 @@ static void pc_init1(MachineState *machine, >>>>> if (pcmc->pci_enabled) { >>>>> DeviceState *dev; >>>>> PCIDevice *pci_dev; >>>>> - const char *type = xen_enabled() ? TYPE_PIIX3_XEN_DEVICE >>>>> - : TYPE_PIIX3_DEVICE; >>>>> int i; >>>>> >>>>> pci_bus = i440fx_init(pci_type, >>>>> @@ -250,7 +248,7 @@ static void pc_init1(MachineState *machine, >>>>> : pci_slot_get_pirq); >>>>> pcms->bus = pci_bus; >>>>> >>>>> - pci_dev = pci_new_multifunction(-1, true, type); >>>>> + pci_dev = pci_new_multifunction(-1, true, TYPE_PIIX3_DEVICE); >>>>> object_property_set_bool(OBJECT(pci_dev), "has-usb", >>>>> machine_usb(machine), &error_abort); >>>>> object_property_set_bool(OBJECT(pci_dev), "has-acpi", >>>>> diff --git a/hw/isa/piix.c b/hw/isa/piix.c >>>>> index 98e9b12661..e4587352c9 100644 >>>>> --- a/hw/isa/piix.c >>>>> +++ b/hw/isa/piix.c >>>>> @@ -33,7 +33,6 @@ >>>>> #include "hw/qdev-properties.h" >>>>> #include "hw/ide/piix.h" >>>>> #include "hw/isa/isa.h" >>>>> -#include "hw/xen/xen.h" >>>>> #include "sysemu/runstate.h" >>>>> #include "migration/vmstate.h" >>>>> #include "hw/acpi/acpi_aml_interface.h" >>>>> @@ -465,24 +464,6 @@ static const TypeInfo piix3_info = { >>>>> .class_init = piix3_class_init, >>>>> }; >>>>> >>>>> -static void piix3_xen_class_init(ObjectClass *klass, void *data) >>>>> -{ >>>>> - DeviceClass *dc = DEVICE_CLASS(klass); >>>>> - PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); >>>>> - >>>>> - k->realize = piix3_realize; >>>>> - /* 82371SB PIIX3 PCI-to-ISA bridge (Step A1) */ >>>>> - k->device_id = PCI_DEVICE_ID_INTEL_82371SB_0; >>>>> - dc->vmsd = &vmstate_piix3; >>>> >>>> IIUC, since this device is user-creatable, we can't simply remove it >>>> without going thru the deprecation process. Alternatively we could >>>> add a type alias: >>>> >>>> -- >8 -- >>>> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c >>>> index 4b0ef65780..d94f7ea369 100644 >>>> --- a/softmmu/qdev-monitor.c >>>> +++ b/softmmu/qdev-monitor.c >>>> @@ -64,6 +64,7 @@ typedef struct QDevAlias >>>> QEMU_ARCH_LOONGARCH) >>>> #define QEMU_ARCH_VIRTIO_CCW (QEMU_ARCH_S390X) >>>> #define QEMU_ARCH_VIRTIO_MMIO (QEMU_ARCH_M68K) >>>> +#define QEMU_ARCH_XEN (QEMU_ARCH_ARM | QEMU_ARCH_I386) >>>> >>>> /* Please keep this table sorted by typename. */ >>>> static const QDevAlias qdev_alias_table[] = { >>>> @@ -111,6 +112,7 @@ static const QDevAlias qdev_alias_table[] = { >>>> { "virtio-tablet-device", "virtio-tablet", QEMU_ARCH_VIRTIO_MMIO }, >>>> { "virtio-tablet-ccw", "virtio-tablet", QEMU_ARCH_VIRTIO_CCW }, >>>> { "virtio-tablet-pci", "virtio-tablet", QEMU_ARCH_VIRTIO_PCI }, >>>> + { "PIIX3", "PIIX3-xen", QEMU_ARCH_XEN }, >>> >>> Hi Bernhard, >>> >>> Can you comment if this should be: >>> >>> + { "PIIX", "PIIX3-xen", QEMU_ARCH_XEN }, >>> >>> instead? IIUC, the patch series also removed PIIX3 and PIIX4 and >>> replaced them with PIIX. Or am I not understanding correctly? >> >> There is a confusion in QEMU between PCI bridges, the first PCI >> function they implement, and the other PCI functions. >> >> Here TYPE_PIIX3_DEVICE means for "PCI function part of the PIIX >> south bridge chipset, which expose a PCI-to-ISA bridge". A better >> name could be TYPE_PIIX3_ISA_PCI_DEVICE. Unfortunately this >> device is named "PIIX3" with no indication of ISA bridge. > > >Thanks, you are right, I see the PIIX3 device still exists after >this patch set is applied. > >chuckz@debian:~/sources-sid/qemu/qemu-7.50+dfsg/hw/i386$ grep -r PIIX3 * >pc_piix.c: pci_dev = pci_new_multifunction(-1, true, TYPE_PIIX3_DEVICE); > >I also understand there is the PCI-to-ISA bridge at 00:01.0 on the PCI bus: > >chuckz@debian:~$ lspci >00:00.0 Host bridge: Intel Corporation 440FX - 82441FX PMC [Natoma] (rev 02) >00:01.0 ISA bridge: Intel Corporation 82371SB PIIX3 ISA [Natoma/Triton II] >00:01.1 IDE interface: Intel Corporation 82371SB PIIX3 IDE [Natoma/Triton II] >00:01.2 USB controller: Intel Corporation 82371SB PIIX3 USB [Natoma/Triton II] (rev 01) >00:01.3 Bridge: Intel Corporation 82371AB/EB/MB PIIX4 ACPI (rev 03) >00:02.0 Unassigned class [ff80]: XenSource, Inc. Xen Platform Device (rev 01) >00:03.0 VGA compatible controller: Device 1234:1111 (rev 02) > >I also see with this patch, there is a bridge that is a PIIX4 ACPI at 00:01.3. Yeah, this PIIX4 ACPI device is what we consider a "Frankenstein" device here on the list. Indeed my PIIX consolidation series aims for eventually replacing the remaining PIIX3 devices with PIIX4 ones to present a realistic environment to guests. The series you tested makes Xen work with PIIX4. With a couple of more patches you might be able to opt into a realistic PIIX4 emulation in the future! Best regards, Bernhard >I get the exact same output from lspci without the patch series, so that gives >me confidence it is working as designed.
Am 4. Januar 2023 17:54:16 UTC schrieb Chuck Zmudzinski <brchuckz@aol.com>: >On 1/4/23 10:35 AM, Philippe Mathieu-Daudé wrote: >> +Markus/Thomas >> >> On 4/1/23 15:44, Bernhard Beschow wrote: >>> During the last patches, TYPE_PIIX3_XEN_DEVICE turned into a clone of >>> TYPE_PIIX3_DEVICE. Remove this redundancy. >>> >>> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >>> --- >>> hw/i386/pc_piix.c | 4 +--- >>> hw/isa/piix.c | 20 -------------------- >>> include/hw/southbridge/piix.h | 1 - >>> 3 files changed, 1 insertion(+), 24 deletions(-) >>> >>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c >>> index 5738d9cdca..6b8de3d59d 100644 >>> --- a/hw/i386/pc_piix.c >>> +++ b/hw/i386/pc_piix.c >>> @@ -235,8 +235,6 @@ static void pc_init1(MachineState *machine, >>> if (pcmc->pci_enabled) { >>> DeviceState *dev; >>> PCIDevice *pci_dev; >>> - const char *type = xen_enabled() ? TYPE_PIIX3_XEN_DEVICE >>> - : TYPE_PIIX3_DEVICE; >>> int i; >>> >>> pci_bus = i440fx_init(pci_type, >>> @@ -250,7 +248,7 @@ static void pc_init1(MachineState *machine, >>> : pci_slot_get_pirq); >>> pcms->bus = pci_bus; >>> >>> - pci_dev = pci_new_multifunction(-1, true, type); >>> + pci_dev = pci_new_multifunction(-1, true, TYPE_PIIX3_DEVICE); >>> object_property_set_bool(OBJECT(pci_dev), "has-usb", >>> machine_usb(machine), &error_abort); >>> object_property_set_bool(OBJECT(pci_dev), "has-acpi", >>> diff --git a/hw/isa/piix.c b/hw/isa/piix.c >>> index 98e9b12661..e4587352c9 100644 >>> --- a/hw/isa/piix.c >>> +++ b/hw/isa/piix.c >>> @@ -33,7 +33,6 @@ >>> #include "hw/qdev-properties.h" >>> #include "hw/ide/piix.h" >>> #include "hw/isa/isa.h" >>> -#include "hw/xen/xen.h" >>> #include "sysemu/runstate.h" >>> #include "migration/vmstate.h" >>> #include "hw/acpi/acpi_aml_interface.h" >>> @@ -465,24 +464,6 @@ static const TypeInfo piix3_info = { >>> .class_init = piix3_class_init, >>> }; >>> >>> -static void piix3_xen_class_init(ObjectClass *klass, void *data) >>> -{ >>> - DeviceClass *dc = DEVICE_CLASS(klass); >>> - PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); >>> - >>> - k->realize = piix3_realize; >>> - /* 82371SB PIIX3 PCI-to-ISA bridge (Step A1) */ >>> - k->device_id = PCI_DEVICE_ID_INTEL_82371SB_0; >>> - dc->vmsd = &vmstate_piix3; >> >> IIUC, since this device is user-creatable, we can't simply remove it >> without going thru the deprecation process. Alternatively we could >> add a type alias: >> >> -- >8 -- >> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c >> index 4b0ef65780..d94f7ea369 100644 >> --- a/softmmu/qdev-monitor.c >> +++ b/softmmu/qdev-monitor.c >> @@ -64,6 +64,7 @@ typedef struct QDevAlias >> QEMU_ARCH_LOONGARCH) >> #define QEMU_ARCH_VIRTIO_CCW (QEMU_ARCH_S390X) >> #define QEMU_ARCH_VIRTIO_MMIO (QEMU_ARCH_M68K) >> +#define QEMU_ARCH_XEN (QEMU_ARCH_ARM | QEMU_ARCH_I386) >> >> /* Please keep this table sorted by typename. */ >> static const QDevAlias qdev_alias_table[] = { >> @@ -111,6 +112,7 @@ static const QDevAlias qdev_alias_table[] = { >> { "virtio-tablet-device", "virtio-tablet", QEMU_ARCH_VIRTIO_MMIO }, >> { "virtio-tablet-ccw", "virtio-tablet", QEMU_ARCH_VIRTIO_CCW }, >> { "virtio-tablet-pci", "virtio-tablet", QEMU_ARCH_VIRTIO_PCI }, >> + { "PIIX3", "PIIX3-xen", QEMU_ARCH_XEN }, > >Hi Bernhard, > >Can you comment if this should be: > >+ { "PIIX", "PIIX3-xen", QEMU_ARCH_XEN }, > >instead? IIUC, the patch series also removed PIIX3 and PIIX4 and >replaced them with PIIX. Or am I not understanding correctly? PIIX3 is correct. The PIIX consolidation is just about sharing code between the PIIX3 and PIIX4 south bridges and should not cause any user or guest observable differences. Best regards, Bernhard > >Best regards, > >Chuck > > >> { } >> }; >> --- >> >> But I'm not sure due to this comment from commit ee46d8a503 >> (2011-12-22 15:24:20 -0600): >> >> 47) /* >> 48) * Aliases were a bad idea from the start. Let's keep them >> 49) * from spreading further. >> 50) */ >> >> Maybe using qdev_alias_table[] during device deprecation is >> acceptable? >> >>> -} >>> - >>> -static const TypeInfo piix3_xen_info = { >>> - .name = TYPE_PIIX3_XEN_DEVICE, >>> - .parent = TYPE_PIIX_PCI_DEVICE, >>> - .instance_init = piix3_init, >>> - .class_init = piix3_xen_class_init, >>> -}; >>> - >>> static void piix4_realize(PCIDevice *dev, Error **errp) >>> { >>> ERRP_GUARD(); >>> @@ -534,7 +515,6 @@ static void piix3_register_types(void) >>> { >>> type_register_static(&piix_pci_type_info); >>> type_register_static(&piix3_info); >>> - type_register_static(&piix3_xen_info); >>> type_register_static(&piix4_info); >>> } >>> >>> diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h >>> index 65ad8569da..b1fc94a742 100644 >>> --- a/include/hw/southbridge/piix.h >>> +++ b/include/hw/southbridge/piix.h >>> @@ -77,7 +77,6 @@ struct PIIXState { >>> OBJECT_DECLARE_SIMPLE_TYPE(PIIXState, PIIX_PCI_DEVICE) >>> >>> #define TYPE_PIIX3_DEVICE "PIIX3" >>> -#define TYPE_PIIX3_XEN_DEVICE "PIIX3-xen" >>> #define TYPE_PIIX4_PCI_DEVICE "piix4-isa" >>> >>> #endif >> >
On 1/4/23 3:44 PM, Bernhard Beschow wrote: > > > Am 4. Januar 2023 17:54:16 UTC schrieb Chuck Zmudzinski <brchuckz@aol.com>: >>On 1/4/23 10:35 AM, Philippe Mathieu-Daudé wrote: >>> +Markus/Thomas >>> >>> On 4/1/23 15:44, Bernhard Beschow wrote: >>>> During the last patches, TYPE_PIIX3_XEN_DEVICE turned into a clone of >>>> TYPE_PIIX3_DEVICE. Remove this redundancy. >>>> >>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >>>> --- >>>> hw/i386/pc_piix.c | 4 +--- >>>> hw/isa/piix.c | 20 -------------------- >>>> include/hw/southbridge/piix.h | 1 - >>>> 3 files changed, 1 insertion(+), 24 deletions(-) >>>> >>>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c >>>> index 5738d9cdca..6b8de3d59d 100644 >>>> --- a/hw/i386/pc_piix.c >>>> +++ b/hw/i386/pc_piix.c >>>> @@ -235,8 +235,6 @@ static void pc_init1(MachineState *machine, >>>> if (pcmc->pci_enabled) { >>>> DeviceState *dev; >>>> PCIDevice *pci_dev; >>>> - const char *type = xen_enabled() ? TYPE_PIIX3_XEN_DEVICE >>>> - : TYPE_PIIX3_DEVICE; >>>> int i; >>>> >>>> pci_bus = i440fx_init(pci_type, >>>> @@ -250,7 +248,7 @@ static void pc_init1(MachineState *machine, >>>> : pci_slot_get_pirq); >>>> pcms->bus = pci_bus; >>>> >>>> - pci_dev = pci_new_multifunction(-1, true, type); >>>> + pci_dev = pci_new_multifunction(-1, true, TYPE_PIIX3_DEVICE); >>>> object_property_set_bool(OBJECT(pci_dev), "has-usb", >>>> machine_usb(machine), &error_abort); >>>> object_property_set_bool(OBJECT(pci_dev), "has-acpi", >>>> diff --git a/hw/isa/piix.c b/hw/isa/piix.c >>>> index 98e9b12661..e4587352c9 100644 >>>> --- a/hw/isa/piix.c >>>> +++ b/hw/isa/piix.c >>>> @@ -33,7 +33,6 @@ >>>> #include "hw/qdev-properties.h" >>>> #include "hw/ide/piix.h" >>>> #include "hw/isa/isa.h" >>>> -#include "hw/xen/xen.h" >>>> #include "sysemu/runstate.h" >>>> #include "migration/vmstate.h" >>>> #include "hw/acpi/acpi_aml_interface.h" >>>> @@ -465,24 +464,6 @@ static const TypeInfo piix3_info = { >>>> .class_init = piix3_class_init, >>>> }; >>>> >>>> -static void piix3_xen_class_init(ObjectClass *klass, void *data) >>>> -{ >>>> - DeviceClass *dc = DEVICE_CLASS(klass); >>>> - PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); >>>> - >>>> - k->realize = piix3_realize; >>>> - /* 82371SB PIIX3 PCI-to-ISA bridge (Step A1) */ >>>> - k->device_id = PCI_DEVICE_ID_INTEL_82371SB_0; >>>> - dc->vmsd = &vmstate_piix3; >>> >>> IIUC, since this device is user-creatable, we can't simply remove it >>> without going thru the deprecation process. Alternatively we could >>> add a type alias: >>> >>> -- >8 -- >>> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c >>> index 4b0ef65780..d94f7ea369 100644 >>> --- a/softmmu/qdev-monitor.c >>> +++ b/softmmu/qdev-monitor.c >>> @@ -64,6 +64,7 @@ typedef struct QDevAlias >>> QEMU_ARCH_LOONGARCH) >>> #define QEMU_ARCH_VIRTIO_CCW (QEMU_ARCH_S390X) >>> #define QEMU_ARCH_VIRTIO_MMIO (QEMU_ARCH_M68K) >>> +#define QEMU_ARCH_XEN (QEMU_ARCH_ARM | QEMU_ARCH_I386) >>> >>> /* Please keep this table sorted by typename. */ >>> static const QDevAlias qdev_alias_table[] = { >>> @@ -111,6 +112,7 @@ static const QDevAlias qdev_alias_table[] = { >>> { "virtio-tablet-device", "virtio-tablet", QEMU_ARCH_VIRTIO_MMIO }, >>> { "virtio-tablet-ccw", "virtio-tablet", QEMU_ARCH_VIRTIO_CCW }, >>> { "virtio-tablet-pci", "virtio-tablet", QEMU_ARCH_VIRTIO_PCI }, >>> + { "PIIX3", "PIIX3-xen", QEMU_ARCH_XEN }, >> >>Hi Bernhard, >> >>Can you comment if this should be: >> >>+ { "PIIX", "PIIX3-xen", QEMU_ARCH_XEN }, >> >>instead? IIUC, the patch series also removed PIIX3 and PIIX4 and >>replaced them with PIIX. Or am I not understanding correctly? > > PIIX3 is correct. The PIIX consolidation is just about sharing code between the PIIX3 and PIIX4 south bridges and should not cause any user or guest observable differences. I realize that now. I see the PIIX3 device still exists after applying the patch set. Thanks, Chuck
On 1/4/23 3:31 PM, Bernhard Beschow wrote: > > > Am 4. Januar 2023 19:29:35 UTC schrieb Chuck Zmudzinski <brchuckz@aol.com>: >>On 1/4/23 1:48 PM, Philippe Mathieu-Daudé wrote: >>> On 4/1/23 18:54, Chuck Zmudzinski wrote: >>>> On 1/4/23 10:35 AM, Philippe Mathieu-Daudé wrote: >>>>> +Markus/Thomas >>>>> >>>>> On 4/1/23 15:44, Bernhard Beschow wrote: >>>>>> During the last patches, TYPE_PIIX3_XEN_DEVICE turned into a clone of >>>>>> TYPE_PIIX3_DEVICE. Remove this redundancy. >>>>>> >>>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >>>>>> --- >>>>>> hw/i386/pc_piix.c | 4 +--- >>>>>> hw/isa/piix.c | 20 -------------------- >>>>>> include/hw/southbridge/piix.h | 1 - >>>>>> 3 files changed, 1 insertion(+), 24 deletions(-) >>>>>> >>>>>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c >>>>>> index 5738d9cdca..6b8de3d59d 100644 >>>>>> --- a/hw/i386/pc_piix.c >>>>>> +++ b/hw/i386/pc_piix.c >>>>>> @@ -235,8 +235,6 @@ static void pc_init1(MachineState *machine, >>>>>> if (pcmc->pci_enabled) { >>>>>> DeviceState *dev; >>>>>> PCIDevice *pci_dev; >>>>>> - const char *type = xen_enabled() ? TYPE_PIIX3_XEN_DEVICE >>>>>> - : TYPE_PIIX3_DEVICE; >>>>>> int i; >>>>>> >>>>>> pci_bus = i440fx_init(pci_type, >>>>>> @@ -250,7 +248,7 @@ static void pc_init1(MachineState *machine, >>>>>> : pci_slot_get_pirq); >>>>>> pcms->bus = pci_bus; >>>>>> >>>>>> - pci_dev = pci_new_multifunction(-1, true, type); >>>>>> + pci_dev = pci_new_multifunction(-1, true, TYPE_PIIX3_DEVICE); >>>>>> object_property_set_bool(OBJECT(pci_dev), "has-usb", >>>>>> machine_usb(machine), &error_abort); >>>>>> object_property_set_bool(OBJECT(pci_dev), "has-acpi", >>>>>> diff --git a/hw/isa/piix.c b/hw/isa/piix.c >>>>>> index 98e9b12661..e4587352c9 100644 >>>>>> --- a/hw/isa/piix.c >>>>>> +++ b/hw/isa/piix.c >>>>>> @@ -33,7 +33,6 @@ >>>>>> #include "hw/qdev-properties.h" >>>>>> #include "hw/ide/piix.h" >>>>>> #include "hw/isa/isa.h" >>>>>> -#include "hw/xen/xen.h" >>>>>> #include "sysemu/runstate.h" >>>>>> #include "migration/vmstate.h" >>>>>> #include "hw/acpi/acpi_aml_interface.h" >>>>>> @@ -465,24 +464,6 @@ static const TypeInfo piix3_info = { >>>>>> .class_init = piix3_class_init, >>>>>> }; >>>>>> >>>>>> -static void piix3_xen_class_init(ObjectClass *klass, void *data) >>>>>> -{ >>>>>> - DeviceClass *dc = DEVICE_CLASS(klass); >>>>>> - PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); >>>>>> - >>>>>> - k->realize = piix3_realize; >>>>>> - /* 82371SB PIIX3 PCI-to-ISA bridge (Step A1) */ >>>>>> - k->device_id = PCI_DEVICE_ID_INTEL_82371SB_0; >>>>>> - dc->vmsd = &vmstate_piix3; >>>>> >>>>> IIUC, since this device is user-creatable, we can't simply remove it >>>>> without going thru the deprecation process. Alternatively we could >>>>> add a type alias: >>>>> >>>>> -- >8 -- >>>>> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c >>>>> index 4b0ef65780..d94f7ea369 100644 >>>>> --- a/softmmu/qdev-monitor.c >>>>> +++ b/softmmu/qdev-monitor.c >>>>> @@ -64,6 +64,7 @@ typedef struct QDevAlias >>>>> QEMU_ARCH_LOONGARCH) >>>>> #define QEMU_ARCH_VIRTIO_CCW (QEMU_ARCH_S390X) >>>>> #define QEMU_ARCH_VIRTIO_MMIO (QEMU_ARCH_M68K) >>>>> +#define QEMU_ARCH_XEN (QEMU_ARCH_ARM | QEMU_ARCH_I386) >>>>> >>>>> /* Please keep this table sorted by typename. */ >>>>> static const QDevAlias qdev_alias_table[] = { >>>>> @@ -111,6 +112,7 @@ static const QDevAlias qdev_alias_table[] = { >>>>> { "virtio-tablet-device", "virtio-tablet", QEMU_ARCH_VIRTIO_MMIO }, >>>>> { "virtio-tablet-ccw", "virtio-tablet", QEMU_ARCH_VIRTIO_CCW }, >>>>> { "virtio-tablet-pci", "virtio-tablet", QEMU_ARCH_VIRTIO_PCI }, >>>>> + { "PIIX3", "PIIX3-xen", QEMU_ARCH_XEN }, >>>> >>>> Hi Bernhard, >>>> >>>> Can you comment if this should be: >>>> >>>> + { "PIIX", "PIIX3-xen", QEMU_ARCH_XEN }, >>>> >>>> instead? IIUC, the patch series also removed PIIX3 and PIIX4 and >>>> replaced them with PIIX. Or am I not understanding correctly? >>> >>> There is a confusion in QEMU between PCI bridges, the first PCI >>> function they implement, and the other PCI functions. >>> >>> Here TYPE_PIIX3_DEVICE means for "PCI function part of the PIIX >>> south bridge chipset, which expose a PCI-to-ISA bridge". A better >>> name could be TYPE_PIIX3_ISA_PCI_DEVICE. Unfortunately this >>> device is named "PIIX3" with no indication of ISA bridge. >> >> >>Thanks, you are right, I see the PIIX3 device still exists after >>this patch set is applied. >> >>chuckz@debian:~/sources-sid/qemu/qemu-7.50+dfsg/hw/i386$ grep -r PIIX3 * >>pc_piix.c: pci_dev = pci_new_multifunction(-1, true, TYPE_PIIX3_DEVICE); >> >>I also understand there is the PCI-to-ISA bridge at 00:01.0 on the PCI bus: >> >>chuckz@debian:~$ lspci >>00:00.0 Host bridge: Intel Corporation 440FX - 82441FX PMC [Natoma] (rev 02) >>00:01.0 ISA bridge: Intel Corporation 82371SB PIIX3 ISA [Natoma/Triton II] >>00:01.1 IDE interface: Intel Corporation 82371SB PIIX3 IDE [Natoma/Triton II] >>00:01.2 USB controller: Intel Corporation 82371SB PIIX3 USB [Natoma/Triton II] (rev 01) >>00:01.3 Bridge: Intel Corporation 82371AB/EB/MB PIIX4 ACPI (rev 03) >>00:02.0 Unassigned class [ff80]: XenSource, Inc. Xen Platform Device (rev 01) >>00:03.0 VGA compatible controller: Device 1234:1111 (rev 02) >> >>I also see with this patch, there is a bridge that is a PIIX4 ACPI at 00:01.3. > > Yeah, this PIIX4 ACPI device is what we consider a "Frankenstein" device here on the list. Indeed my PIIX consolidation series aims for eventually replacing the remaining PIIX3 devices with PIIX4 ones to present a realistic environment to guests. The series you tested makes Xen work with PIIX4. With a couple of more patches you might be able to opt into a realistic PIIX4 emulation in the future! That's exactly what I want to hear as a future milestone, and thanks for your work on this! Kind regards, Chuck
On 4/1/23 20:29, Chuck Zmudzinski wrote: > On 1/4/23 1:48 PM, Philippe Mathieu-Daudé wrote: >> Here TYPE_PIIX3_DEVICE means for "PCI function part of the PIIX >> south bridge chipset, which expose a PCI-to-ISA bridge". A better >> name could be TYPE_PIIX3_ISA_PCI_DEVICE. Unfortunately this >> device is named "PIIX3" with no indication of ISA bridge. > > > Thanks, you are right, I see the PIIX3 device still exists after > this patch set is applied. > > chuckz@debian:~/sources-sid/qemu/qemu-7.50+dfsg/hw/i386$ grep -r PIIX3 * > pc_piix.c: pci_dev = pci_new_multifunction(-1, true, TYPE_PIIX3_DEVICE); > > I also understand there is the PCI-to-ISA bridge at 00:01.0 on the PCI bus: > > chuckz@debian:~$ lspci > 00:00.0 Host bridge: Intel Corporation 440FX - 82441FX PMC [Natoma] (rev 02) All these entries ('PCI functions') ...: > 00:01.0 ISA bridge: Intel Corporation 82371SB PIIX3 ISA [Natoma/Triton II] > 00:01.1 IDE interface: Intel Corporation 82371SB PIIX3 IDE [Natoma/Triton II] > 00:01.2 USB controller: Intel Corporation 82371SB PIIX3 USB [Natoma/Triton II] (rev 01) > 00:01.3 Bridge: Intel Corporation 82371AB/EB/MB PIIX4 ACPI (rev 03) ... are part of the same *device*: the PIIX south bridge. This device is enumerated as #1 on the PCI bus #0. It currently exposes 4 functions: ISA/IDE/USB/ACPI. > 00:02.0 Unassigned class [ff80]: XenSource, Inc. Xen Platform Device (rev 01) > 00:03.0 VGA compatible controller: Device 1234:1111 (rev 02) > > I also see with this patch, there is a bridge that is a PIIX4 ACPI at 00:01.3. > I get the exact same output from lspci without the patch series, so that gives > me confidence it is working as designed. Historically the PIIX3 and PIIX4 QEMU models have been written by different people with different goals. - PIIX3 comes from x86 machines and is important for KVM/Xen accelerators - PIIX4 was developed by hobbyist for MIPS machines PIIX4 added the ACPI function which was proven helpful for x86 machines. OS such Linux don't consider the PIIX south bridge as a whole chipset, and enumerate each PCI function individually. So it was possible to add the PIIX4 ACPI function to a PIIX3... A config that doesn't exist with real hardware :/ While QEMU aims at modeling real HW, this config is still very useful for KVM/Xen. So this Frankenstein config is accepted / maintained. Bernhard is doing an incredible work merging the PIIX3/PIIX4 differences into a more maintainable model :) Regards, Phil.
On 4/1/23 17:42, Chuck Zmudzinski wrote: > On 1/4/23 9:44 AM, Bernhard Beschow wrote: >> During the last patches, TYPE_PIIX3_XEN_DEVICE turned into a clone of >> TYPE_PIIX3_DEVICE. Remove this redundancy. >> >> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >> --- >> hw/i386/pc_piix.c | 4 +--- >> hw/isa/piix.c | 20 -------------------- >> include/hw/southbridge/piix.h | 1 - >> 3 files changed, 1 insertion(+), 24 deletions(-) >> >> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c >> index 5738d9cdca..6b8de3d59d 100644 >> --- a/hw/i386/pc_piix.c >> +++ b/hw/i386/pc_piix.c >> @@ -235,8 +235,6 @@ static void pc_init1(MachineState *machine, >> if (pcmc->pci_enabled) { >> DeviceState *dev; >> PCIDevice *pci_dev; >> - const char *type = xen_enabled() ? TYPE_PIIX3_XEN_DEVICE >> - : TYPE_PIIX3_DEVICE; >> int i; >> >> pci_bus = i440fx_init(pci_type, >> @@ -250,7 +248,7 @@ static void pc_init1(MachineState *machine, >> : pci_slot_get_pirq); >> pcms->bus = pci_bus; >> >> - pci_dev = pci_new_multifunction(-1, true, type); >> + pci_dev = pci_new_multifunction(-1, true, TYPE_PIIX3_DEVICE); >> object_property_set_bool(OBJECT(pci_dev), "has-usb", >> machine_usb(machine), &error_abort); >> object_property_set_bool(OBJECT(pci_dev), "has-acpi", >> diff --git a/hw/isa/piix.c b/hw/isa/piix.c >> index 98e9b12661..e4587352c9 100644 >> --- a/hw/isa/piix.c >> +++ b/hw/isa/piix.c >> @@ -33,7 +33,6 @@ >> #include "hw/qdev-properties.h" >> #include "hw/ide/piix.h" >> #include "hw/isa/isa.h" >> -#include "hw/xen/xen.h" >> #include "sysemu/runstate.h" >> #include "migration/vmstate.h" >> #include "hw/acpi/acpi_aml_interface.h" >> @@ -465,24 +464,6 @@ static const TypeInfo piix3_info = { >> .class_init = piix3_class_init, >> }; >> >> -static void piix3_xen_class_init(ObjectClass *klass, void *data) >> -{ >> - DeviceClass *dc = DEVICE_CLASS(klass); >> - PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); >> - >> - k->realize = piix3_realize; >> - /* 82371SB PIIX3 PCI-to-ISA bridge (Step A1) */ >> - k->device_id = PCI_DEVICE_ID_INTEL_82371SB_0; >> - dc->vmsd = &vmstate_piix3; >> -} >> - >> -static const TypeInfo piix3_xen_info = { >> - .name = TYPE_PIIX3_XEN_DEVICE, >> - .parent = TYPE_PIIX_PCI_DEVICE, >> - .instance_init = piix3_init, >> - .class_init = piix3_xen_class_init, >> -}; >> - >> static void piix4_realize(PCIDevice *dev, Error **errp) >> { >> ERRP_GUARD(); >> @@ -534,7 +515,6 @@ static void piix3_register_types(void) >> { >> type_register_static(&piix_pci_type_info); >> type_register_static(&piix3_info); >> - type_register_static(&piix3_xen_info); >> type_register_static(&piix4_info); >> } >> >> diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h >> index 65ad8569da..b1fc94a742 100644 >> --- a/include/hw/southbridge/piix.h >> +++ b/include/hw/southbridge/piix.h >> @@ -77,7 +77,6 @@ struct PIIXState { >> OBJECT_DECLARE_SIMPLE_TYPE(PIIXState, PIIX_PCI_DEVICE) >> >> #define TYPE_PIIX3_DEVICE "PIIX3" >> -#define TYPE_PIIX3_XEN_DEVICE "PIIX3-xen" >> #define TYPE_PIIX4_PCI_DEVICE "piix4-isa" >> >> #endif > > > This fixes the regression with the emulated usb tablet device that I reported in v1 here: > > https://lore.kernel.org/qemu-devel/aed4f2c1-83f7-163a-fb44-f284376668dc@aol.com/ > > I tested this patch again with all the prerequisites and now with v2 there are no regressions. > > Tested-by: Chuck Zmudzinski <brchuckz@aol.com> (IIUC Chuck meant to send this tag to the cover letter)
On 1/4/23 5:35 PM, Philippe Mathieu-Daudé wrote: > On 4/1/23 17:42, Chuck Zmudzinski wrote: >> On 1/4/23 9:44 AM, Bernhard Beschow wrote: >>> During the last patches, TYPE_PIIX3_XEN_DEVICE turned into a clone of >>> TYPE_PIIX3_DEVICE. Remove this redundancy. >>> >>> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >>> --- >>> hw/i386/pc_piix.c | 4 +--- >>> hw/isa/piix.c | 20 -------------------- >>> include/hw/southbridge/piix.h | 1 - >>> 3 files changed, 1 insertion(+), 24 deletions(-) >>> >>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c >>> index 5738d9cdca..6b8de3d59d 100644 >>> --- a/hw/i386/pc_piix.c >>> +++ b/hw/i386/pc_piix.c >>> @@ -235,8 +235,6 @@ static void pc_init1(MachineState *machine, >>> if (pcmc->pci_enabled) { >>> DeviceState *dev; >>> PCIDevice *pci_dev; >>> - const char *type = xen_enabled() ? TYPE_PIIX3_XEN_DEVICE >>> - : TYPE_PIIX3_DEVICE; >>> int i; >>> >>> pci_bus = i440fx_init(pci_type, >>> @@ -250,7 +248,7 @@ static void pc_init1(MachineState *machine, >>> : pci_slot_get_pirq); >>> pcms->bus = pci_bus; >>> >>> - pci_dev = pci_new_multifunction(-1, true, type); >>> + pci_dev = pci_new_multifunction(-1, true, TYPE_PIIX3_DEVICE); >>> object_property_set_bool(OBJECT(pci_dev), "has-usb", >>> machine_usb(machine), &error_abort); >>> object_property_set_bool(OBJECT(pci_dev), "has-acpi", >>> diff --git a/hw/isa/piix.c b/hw/isa/piix.c >>> index 98e9b12661..e4587352c9 100644 >>> --- a/hw/isa/piix.c >>> +++ b/hw/isa/piix.c >>> @@ -33,7 +33,6 @@ >>> #include "hw/qdev-properties.h" >>> #include "hw/ide/piix.h" >>> #include "hw/isa/isa.h" >>> -#include "hw/xen/xen.h" >>> #include "sysemu/runstate.h" >>> #include "migration/vmstate.h" >>> #include "hw/acpi/acpi_aml_interface.h" >>> @@ -465,24 +464,6 @@ static const TypeInfo piix3_info = { >>> .class_init = piix3_class_init, >>> }; >>> >>> -static void piix3_xen_class_init(ObjectClass *klass, void *data) >>> -{ >>> - DeviceClass *dc = DEVICE_CLASS(klass); >>> - PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); >>> - >>> - k->realize = piix3_realize; >>> - /* 82371SB PIIX3 PCI-to-ISA bridge (Step A1) */ >>> - k->device_id = PCI_DEVICE_ID_INTEL_82371SB_0; >>> - dc->vmsd = &vmstate_piix3; >>> -} >>> - >>> -static const TypeInfo piix3_xen_info = { >>> - .name = TYPE_PIIX3_XEN_DEVICE, >>> - .parent = TYPE_PIIX_PCI_DEVICE, >>> - .instance_init = piix3_init, >>> - .class_init = piix3_xen_class_init, >>> -}; >>> - >>> static void piix4_realize(PCIDevice *dev, Error **errp) >>> { >>> ERRP_GUARD(); >>> @@ -534,7 +515,6 @@ static void piix3_register_types(void) >>> { >>> type_register_static(&piix_pci_type_info); >>> type_register_static(&piix3_info); >>> - type_register_static(&piix3_xen_info); >>> type_register_static(&piix4_info); >>> } >>> >>> diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h >>> index 65ad8569da..b1fc94a742 100644 >>> --- a/include/hw/southbridge/piix.h >>> +++ b/include/hw/southbridge/piix.h >>> @@ -77,7 +77,6 @@ struct PIIXState { >>> OBJECT_DECLARE_SIMPLE_TYPE(PIIXState, PIIX_PCI_DEVICE) >>> >>> #define TYPE_PIIX3_DEVICE "PIIX3" >>> -#define TYPE_PIIX3_XEN_DEVICE "PIIX3-xen" >>> #define TYPE_PIIX4_PCI_DEVICE "piix4-isa" >>> >>> #endif >> >> >> This fixes the regression with the emulated usb tablet device that I reported in v1 here: >> >> https://lore.kernel.org/qemu-devel/aed4f2c1-83f7-163a-fb44-f284376668dc@aol.com/ >> >> I tested this patch again with all the prerequisites and now with v2 there are no regressions. >> >> Tested-by: Chuck Zmudzinski <brchuckz@aol.com> > > (IIUC Chuck meant to send this tag to the cover letter) > Yes, I tested the whole patch series, not just this individual patch. I tagged this one because it is the last patch in the series.
On 1/4/2023 5:35 PM, Philippe Mathieu-Daudé wrote: > On 4/1/23 17:42, Chuck Zmudzinski wrote: > > On 1/4/23 9:44 AM, Bernhard Beschow wrote: > >> During the last patches, TYPE_PIIX3_XEN_DEVICE turned into a clone of > >> TYPE_PIIX3_DEVICE. Remove this redundancy. > >> > >> Signed-off-by: Bernhard Beschow <shentey@gmail.com> > >> --- > >> hw/i386/pc_piix.c | 4 +--- > >> hw/isa/piix.c | 20 -------------------- > >> include/hw/southbridge/piix.h | 1 - > >> 3 files changed, 1 insertion(+), 24 deletions(-) > >> > >> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > >> index 5738d9cdca..6b8de3d59d 100644 > >> --- a/hw/i386/pc_piix.c > >> +++ b/hw/i386/pc_piix.c > >> @@ -235,8 +235,6 @@ static void pc_init1(MachineState *machine, > >> if (pcmc->pci_enabled) { > >> DeviceState *dev; > >> PCIDevice *pci_dev; > >> - const char *type = xen_enabled() ? TYPE_PIIX3_XEN_DEVICE > >> - : TYPE_PIIX3_DEVICE; > >> int i; > >> > >> pci_bus = i440fx_init(pci_type, > >> @@ -250,7 +248,7 @@ static void pc_init1(MachineState *machine, > >> : pci_slot_get_pirq); > >> pcms->bus = pci_bus; > >> > >> - pci_dev = pci_new_multifunction(-1, true, type); > >> + pci_dev = pci_new_multifunction(-1, true, TYPE_PIIX3_DEVICE); > >> object_property_set_bool(OBJECT(pci_dev), "has-usb", > >> machine_usb(machine), &error_abort); > >> object_property_set_bool(OBJECT(pci_dev), "has-acpi", > >> diff --git a/hw/isa/piix.c b/hw/isa/piix.c > >> index 98e9b12661..e4587352c9 100644 > >> --- a/hw/isa/piix.c > >> +++ b/hw/isa/piix.c > >> @@ -33,7 +33,6 @@ > >> #include "hw/qdev-properties.h" > >> #include "hw/ide/piix.h" > >> #include "hw/isa/isa.h" > >> -#include "hw/xen/xen.h" > >> #include "sysemu/runstate.h" > >> #include "migration/vmstate.h" > >> #include "hw/acpi/acpi_aml_interface.h" > >> @@ -465,24 +464,6 @@ static const TypeInfo piix3_info = { > >> .class_init = piix3_class_init, > >> }; > >> > >> -static void piix3_xen_class_init(ObjectClass *klass, void *data) > >> -{ > >> - DeviceClass *dc = DEVICE_CLASS(klass); > >> - PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); > >> - > >> - k->realize = piix3_realize; > >> - /* 82371SB PIIX3 PCI-to-ISA bridge (Step A1) */ > >> - k->device_id = PCI_DEVICE_ID_INTEL_82371SB_0; > >> - dc->vmsd = &vmstate_piix3; > >> -} > >> - > >> -static const TypeInfo piix3_xen_info = { > >> - .name = TYPE_PIIX3_XEN_DEVICE, > >> - .parent = TYPE_PIIX_PCI_DEVICE, > >> - .instance_init = piix3_init, > >> - .class_init = piix3_xen_class_init, > >> -}; > >> - > >> static void piix4_realize(PCIDevice *dev, Error **errp) > >> { > >> ERRP_GUARD(); > >> @@ -534,7 +515,6 @@ static void piix3_register_types(void) > >> { > >> type_register_static(&piix_pci_type_info); > >> type_register_static(&piix3_info); > >> - type_register_static(&piix3_xen_info); > >> type_register_static(&piix4_info); > >> } > >> > >> diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h > >> index 65ad8569da..b1fc94a742 100644 > >> --- a/include/hw/southbridge/piix.h > >> +++ b/include/hw/southbridge/piix.h > >> @@ -77,7 +77,6 @@ struct PIIXState { > >> OBJECT_DECLARE_SIMPLE_TYPE(PIIXState, PIIX_PCI_DEVICE) > >> > >> #define TYPE_PIIX3_DEVICE "PIIX3" > >> -#define TYPE_PIIX3_XEN_DEVICE "PIIX3-xen" > >> #define TYPE_PIIX4_PCI_DEVICE "piix4-isa" > >> > >> #endif > > > > > > This fixes the regression with the emulated usb tablet device that I reported in v1 here: > > > > https://lore.kernel.org/qemu-devel/aed4f2c1-83f7-163a-fb44-f284376668dc@aol.com/ > > > > I tested this patch again with all the prerequisites and now with v2 there are no regressions. > > > > Tested-by: Chuck Zmudzinski <brchuckz@aol.com> > > (IIUC Chuck meant to send this tag to the cover letter) > Is it customary to tag the cover letter instead? I thought it appropriate to tag the last commit in the series because it best represents the actual commit on which tests were carried out. Also, the cover letter does not actually represent a real commit that can be tested, except maybe the last commit in the series. I did read the document on submitting patches, but I don't remember if it specifies how to tag a series of patches with the Tested-by tag. Kind regards, Chuck
On 1/4/2023 5:18 PM, Philippe Mathieu-Daudé wrote: > On 4/1/23 20:29, Chuck Zmudzinski wrote: > > On 1/4/23 1:48 PM, Philippe Mathieu-Daudé wrote: > > >> Here TYPE_PIIX3_DEVICE means for "PCI function part of the PIIX > >> south bridge chipset, which expose a PCI-to-ISA bridge". A better > >> name could be TYPE_PIIX3_ISA_PCI_DEVICE. Unfortunately this > >> device is named "PIIX3" with no indication of ISA bridge. > > > > > > Thanks, you are right, I see the PIIX3 device still exists after > > this patch set is applied. > > > > chuckz@debian:~/sources-sid/qemu/qemu-7.50+dfsg/hw/i386$ grep -r PIIX3 * > > pc_piix.c: pci_dev = pci_new_multifunction(-1, true, TYPE_PIIX3_DEVICE); > > > > I also understand there is the PCI-to-ISA bridge at 00:01.0 on the PCI bus: > > > > chuckz@debian:~$ lspci > > 00:00.0 Host bridge: Intel Corporation 440FX - 82441FX PMC [Natoma] (rev 02) > > All these entries ('PCI functions') ...: > > > 00:01.0 ISA bridge: Intel Corporation 82371SB PIIX3 ISA [Natoma/Triton II] > > 00:01.1 IDE interface: Intel Corporation 82371SB PIIX3 IDE [Natoma/Triton II] > > 00:01.2 USB controller: Intel Corporation 82371SB PIIX3 USB [Natoma/Triton II] (rev 01) > > 00:01.3 Bridge: Intel Corporation 82371AB/EB/MB PIIX4 ACPI (rev 03) > > ... are part of the same *device*: the PIIX south bridge. > > This device is enumerated as #1 on the PCI bus #0. > It currently exposes 4 functions: ISA/IDE/USB/ACPI. > > > 00:02.0 Unassigned class [ff80]: XenSource, Inc. Xen Platform Device (rev 01) > > 00:03.0 VGA compatible controller: Device 1234:1111 (rev 02) > > > > I also see with this patch, there is a bridge that is a PIIX4 ACPI at 00:01.3. > > I get the exact same output from lspci without the patch series, so that gives > > me confidence it is working as designed. > > Historically the PIIX3 and PIIX4 QEMU models have been written by > different people with different goals. > > - PIIX3 comes from x86 machines and is important for KVM/Xen > accelerators > - PIIX4 was developed by hobbyist for MIPS machines > > PIIX4 added the ACPI function which was proven helpful for x86 machines. > > OS such Linux don't consider the PIIX south bridge as a whole chipset, > and enumerate each PCI function individually. So it was possible to add > the PIIX4 ACPI function to a PIIX3... A config that doesn't exist with > real hardware :/ > While QEMU aims at modeling real HW, this config is still very useful > for KVM/Xen. So this Frankenstein config is accepted / maintained. > > Bernhard is doing an incredible work merging the PIIX3/PIIX4 differences > into a more maintainable model :) > > Regards, > > Phil. Thanks for the nice explanation of the history. I understand the PIIX3 is associated with the i440fx machine type for i386 - it goes all the way back to 1995 I think with the original 32-bit Pentium processor and Windows 95. So it is a worthwhile effort to work on updating this to something newer, and of course kvm can use the newer Q35 chipset which goes back to 2009 or so, I think, but xen/x86 languishes on the i440fx for now. Best regards, Chuck
Am 4. Januar 2023 15:35:33 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>: >+Markus/Thomas > >On 4/1/23 15:44, Bernhard Beschow wrote: >> During the last patches, TYPE_PIIX3_XEN_DEVICE turned into a clone of >> TYPE_PIIX3_DEVICE. Remove this redundancy. >> >> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >> --- >> hw/i386/pc_piix.c | 4 +--- >> hw/isa/piix.c | 20 -------------------- >> include/hw/southbridge/piix.h | 1 - >> 3 files changed, 1 insertion(+), 24 deletions(-) >> >> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c >> index 5738d9cdca..6b8de3d59d 100644 >> --- a/hw/i386/pc_piix.c >> +++ b/hw/i386/pc_piix.c >> @@ -235,8 +235,6 @@ static void pc_init1(MachineState *machine, >> if (pcmc->pci_enabled) { >> DeviceState *dev; >> PCIDevice *pci_dev; >> - const char *type = xen_enabled() ? TYPE_PIIX3_XEN_DEVICE >> - : TYPE_PIIX3_DEVICE; >> int i; >> pci_bus = i440fx_init(pci_type, >> @@ -250,7 +248,7 @@ static void pc_init1(MachineState *machine, >> : pci_slot_get_pirq); >> pcms->bus = pci_bus; >> - pci_dev = pci_new_multifunction(-1, true, type); >> + pci_dev = pci_new_multifunction(-1, true, TYPE_PIIX3_DEVICE); >> object_property_set_bool(OBJECT(pci_dev), "has-usb", >> machine_usb(machine), &error_abort); >> object_property_set_bool(OBJECT(pci_dev), "has-acpi", >> diff --git a/hw/isa/piix.c b/hw/isa/piix.c >> index 98e9b12661..e4587352c9 100644 >> --- a/hw/isa/piix.c >> +++ b/hw/isa/piix.c >> @@ -33,7 +33,6 @@ >> #include "hw/qdev-properties.h" >> #include "hw/ide/piix.h" >> #include "hw/isa/isa.h" >> -#include "hw/xen/xen.h" >> #include "sysemu/runstate.h" >> #include "migration/vmstate.h" >> #include "hw/acpi/acpi_aml_interface.h" >> @@ -465,24 +464,6 @@ static const TypeInfo piix3_info = { >> .class_init = piix3_class_init, >> }; >> -static void piix3_xen_class_init(ObjectClass *klass, void *data) >> -{ >> - DeviceClass *dc = DEVICE_CLASS(klass); >> - PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); >> - >> - k->realize = piix3_realize; >> - /* 82371SB PIIX3 PCI-to-ISA bridge (Step A1) */ >> - k->device_id = PCI_DEVICE_ID_INTEL_82371SB_0; >> - dc->vmsd = &vmstate_piix3; > >IIUC, since this device is user-creatable, we can't simply remove it >without going thru the deprecation process. AFAICS this device is actually not user-creatable since dc->user_creatable is set to false once in the base class. I think it is safe to remove the Xen class unless there are ABI issues. Best regards, Bernhard >Alternatively we could >add a type alias: > >-- >8 -- >diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c >index 4b0ef65780..d94f7ea369 100644 >--- a/softmmu/qdev-monitor.c >+++ b/softmmu/qdev-monitor.c >@@ -64,6 +64,7 @@ typedef struct QDevAlias > QEMU_ARCH_LOONGARCH) > #define QEMU_ARCH_VIRTIO_CCW (QEMU_ARCH_S390X) > #define QEMU_ARCH_VIRTIO_MMIO (QEMU_ARCH_M68K) >+#define QEMU_ARCH_XEN (QEMU_ARCH_ARM | QEMU_ARCH_I386) > > /* Please keep this table sorted by typename. */ > static const QDevAlias qdev_alias_table[] = { >@@ -111,6 +112,7 @@ static const QDevAlias qdev_alias_table[] = { > { "virtio-tablet-device", "virtio-tablet", QEMU_ARCH_VIRTIO_MMIO }, > { "virtio-tablet-ccw", "virtio-tablet", QEMU_ARCH_VIRTIO_CCW }, > { "virtio-tablet-pci", "virtio-tablet", QEMU_ARCH_VIRTIO_PCI }, >+ { "PIIX3", "PIIX3-xen", QEMU_ARCH_XEN }, > { } > }; >--- > >But I'm not sure due to this comment from commit ee46d8a503 >(2011-12-22 15:24:20 -0600): > >47) /* >48) * Aliases were a bad idea from the start. Let's keep them >49) * from spreading further. >50) */ > >Maybe using qdev_alias_table[] during device deprecation is >acceptable? > >> -} >> - >> -static const TypeInfo piix3_xen_info = { >> - .name = TYPE_PIIX3_XEN_DEVICE, >> - .parent = TYPE_PIIX_PCI_DEVICE, >> - .instance_init = piix3_init, >> - .class_init = piix3_xen_class_init, >> -}; >> - >> static void piix4_realize(PCIDevice *dev, Error **errp) >> { >> ERRP_GUARD(); >> @@ -534,7 +515,6 @@ static void piix3_register_types(void) >> { >> type_register_static(&piix_pci_type_info); >> type_register_static(&piix3_info); >> - type_register_static(&piix3_xen_info); >> type_register_static(&piix4_info); >> } >> diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h >> index 65ad8569da..b1fc94a742 100644 >> --- a/include/hw/southbridge/piix.h >> +++ b/include/hw/southbridge/piix.h >> @@ -77,7 +77,6 @@ struct PIIXState { >> OBJECT_DECLARE_SIMPLE_TYPE(PIIXState, PIIX_PCI_DEVICE) >> #define TYPE_PIIX3_DEVICE "PIIX3" >> -#define TYPE_PIIX3_XEN_DEVICE "PIIX3-xen" >> #define TYPE_PIIX4_PCI_DEVICE "piix4-isa" >> #endif >
On 6/1/23 12:57, Bernhard Beschow wrote: > > > Am 4. Januar 2023 15:35:33 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>: >> +Markus/Thomas >> >> On 4/1/23 15:44, Bernhard Beschow wrote: >>> During the last patches, TYPE_PIIX3_XEN_DEVICE turned into a clone of >>> TYPE_PIIX3_DEVICE. Remove this redundancy. >>> >>> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >>> --- >>> hw/i386/pc_piix.c | 4 +--- >>> hw/isa/piix.c | 20 -------------------- >>> include/hw/southbridge/piix.h | 1 - >>> 3 files changed, 1 insertion(+), 24 deletions(-) >>> -static void piix3_xen_class_init(ObjectClass *klass, void *data) >>> -{ >>> - DeviceClass *dc = DEVICE_CLASS(klass); >>> - PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); >>> - >>> - k->realize = piix3_realize; >>> - /* 82371SB PIIX3 PCI-to-ISA bridge (Step A1) */ >>> - k->device_id = PCI_DEVICE_ID_INTEL_82371SB_0; >>> - dc->vmsd = &vmstate_piix3; >> >> IIUC, since this device is user-creatable, we can't simply remove it >> without going thru the deprecation process. > > AFAICS this device is actually not user-creatable since dc->user_creatable is set to false once in the base class. I think it is safe to remove the Xen class unless there are ABI issues. Great news!
On 1/6/23 7:25 AM, Philippe Mathieu-Daudé wrote: > On 6/1/23 12:57, Bernhard Beschow wrote: >> >> >> Am 4. Januar 2023 15:35:33 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>: >>> +Markus/Thomas >>> >>> On 4/1/23 15:44, Bernhard Beschow wrote: >>>> During the last patches, TYPE_PIIX3_XEN_DEVICE turned into a clone of >>>> TYPE_PIIX3_DEVICE. Remove this redundancy. >>>> >>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >>>> --- >>>> hw/i386/pc_piix.c | 4 +--- >>>> hw/isa/piix.c | 20 -------------------- >>>> include/hw/southbridge/piix.h | 1 - >>>> 3 files changed, 1 insertion(+), 24 deletions(-) > > >>>> -static void piix3_xen_class_init(ObjectClass *klass, void *data) >>>> -{ >>>> - DeviceClass *dc = DEVICE_CLASS(klass); >>>> - PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); >>>> - >>>> - k->realize = piix3_realize; >>>> - /* 82371SB PIIX3 PCI-to-ISA bridge (Step A1) */ >>>> - k->device_id = PCI_DEVICE_ID_INTEL_82371SB_0; >>>> - dc->vmsd = &vmstate_piix3; >>> >>> IIUC, since this device is user-creatable, we can't simply remove it >>> without going thru the deprecation process. >> >> AFAICS this device is actually not user-creatable since dc->user_creatable is set to false once in the base class. I think it is safe to remove the Xen class unless there are ABI issues. > Great news! I don't know if this means the device is user-creatable: chuckz@bullseye:~$ qemu-system-i386 -device piix3-ide-xen,help piix3-ide-xen options: addr=<int32> - Slot and optional function number, example: 06.0 or 06 (default: -1) failover_pair_id=<str> multifunction=<bool> - on/off (default: false) rombar=<uint32> - (default: 1) romfile=<str> x-pcie-extcap-init=<bool> - on/off (default: true) x-pcie-lnksta-dllla=<bool> - on/off (default: true) Today I am running qemu-5.2 on Debian 11, so this output is for qemu 5.2, and that version of qemu has a piix3-ide-xen device. Is that this same device that is being removed? If so, it seems to me that at least as of qemu 5.2, the device was user-creatable. Chuck
On 1/6/23 2:08 PM, Chuck Zmudzinski wrote: > On 1/6/23 7:25 AM, Philippe Mathieu-Daudé wrote: >> On 6/1/23 12:57, Bernhard Beschow wrote: >>> >>> >>> Am 4. Januar 2023 15:35:33 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>: >>>> +Markus/Thomas >>>> >>>> On 4/1/23 15:44, Bernhard Beschow wrote: >>>>> During the last patches, TYPE_PIIX3_XEN_DEVICE turned into a clone of >>>>> TYPE_PIIX3_DEVICE. Remove this redundancy. >>>>> >>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >>>>> --- >>>>> hw/i386/pc_piix.c | 4 +--- >>>>> hw/isa/piix.c | 20 -------------------- >>>>> include/hw/southbridge/piix.h | 1 - >>>>> 3 files changed, 1 insertion(+), 24 deletions(-) >> >> >>>>> -static void piix3_xen_class_init(ObjectClass *klass, void *data) >>>>> -{ >>>>> - DeviceClass *dc = DEVICE_CLASS(klass); >>>>> - PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); >>>>> - >>>>> - k->realize = piix3_realize; >>>>> - /* 82371SB PIIX3 PCI-to-ISA bridge (Step A1) */ >>>>> - k->device_id = PCI_DEVICE_ID_INTEL_82371SB_0; >>>>> - dc->vmsd = &vmstate_piix3; >>>> >>>> IIUC, since this device is user-creatable, we can't simply remove it >>>> without going thru the deprecation process. >>> >>> AFAICS this device is actually not user-creatable since dc->user_creatable is set to false once in the base class. I think it is safe to remove the Xen class unless there are ABI issues. >> Great news! > > I don't know if this means the device is user-creatable: > > chuckz@bullseye:~$ qemu-system-i386 -device piix3-ide-xen,help > piix3-ide-xen options: > addr=<int32> - Slot and optional function number, example: 06.0 or 06 (default: -1) > failover_pair_id=<str> > multifunction=<bool> - on/off (default: false) > rombar=<uint32> - (default: 1) > romfile=<str> > x-pcie-extcap-init=<bool> - on/off (default: true) > x-pcie-lnksta-dllla=<bool> - on/off (default: true) > > Today I am running qemu-5.2 on Debian 11, so this output is for > qemu 5.2, and that version of qemu has a piix3-ide-xen device. > Is that this same device that is being removed? If so, it seems to > me that at least as of qemu 5.2, the device was user-creatable. > > Chuck Good news! It looks the device was removed as user-creatable since version 5.2: chuckz@debian:~$ qemu-system-i386-7.50 -device help | grep piix name "piix3-usb-uhci", bus PCI name "piix4-usb-uhci", bus PCI name "piix3-ide", bus PCI name "piix4-ide", bus PCI chuckz@debian:~$ qemu-system-i386-7.50-bernhard-v2 -device help | grep piix name "piix3-usb-uhci", bus PCI name "piix4-usb-uhci", bus PCI name "piix3-ide", bus PCI name "piix4-ide", bus PCI chuckz@debian:~$ The piix3-ide-xen device is not present either with or without Bernhard's patches for current qemu 7.50, the development version for qemu 8.0 Cheers, Chuck
On 1/6/23 6:04 PM, Chuck Zmudzinski wrote: > On 1/6/23 2:08 PM, Chuck Zmudzinski wrote: >> On 1/6/23 7:25 AM, Philippe Mathieu-Daudé wrote: >>> On 6/1/23 12:57, Bernhard Beschow wrote: >>>> >>>> >>>> Am 4. Januar 2023 15:35:33 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>: >>>>> +Markus/Thomas >>>>> >>>>> On 4/1/23 15:44, Bernhard Beschow wrote: >>>>>> During the last patches, TYPE_PIIX3_XEN_DEVICE turned into a clone of >>>>>> TYPE_PIIX3_DEVICE. Remove this redundancy. >>>>>> >>>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >>>>>> --- >>>>>> hw/i386/pc_piix.c | 4 +--- >>>>>> hw/isa/piix.c | 20 -------------------- >>>>>> include/hw/southbridge/piix.h | 1 - >>>>>> 3 files changed, 1 insertion(+), 24 deletions(-) >>> >>> >>>>>> -static void piix3_xen_class_init(ObjectClass *klass, void *data) >>>>>> -{ >>>>>> - DeviceClass *dc = DEVICE_CLASS(klass); >>>>>> - PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); >>>>>> - >>>>>> - k->realize = piix3_realize; >>>>>> - /* 82371SB PIIX3 PCI-to-ISA bridge (Step A1) */ >>>>>> - k->device_id = PCI_DEVICE_ID_INTEL_82371SB_0; >>>>>> - dc->vmsd = &vmstate_piix3; >>>>> >>>>> IIUC, since this device is user-creatable, we can't simply remove it >>>>> without going thru the deprecation process. >>>> >>>> AFAICS this device is actually not user-creatable since dc->user_creatable is set to false once in the base class. I think it is safe to remove the Xen class unless there are ABI issues. >>> Great news! >> >> I don't know if this means the device is user-creatable: >> >> chuckz@bullseye:~$ qemu-system-i386 -device piix3-ide-xen,help >> piix3-ide-xen options: >> addr=<int32> - Slot and optional function number, example: 06.0 or 06 (default: -1) >> failover_pair_id=<str> >> multifunction=<bool> - on/off (default: false) >> rombar=<uint32> - (default: 1) >> romfile=<str> >> x-pcie-extcap-init=<bool> - on/off (default: true) >> x-pcie-lnksta-dllla=<bool> - on/off (default: true) >> >> Today I am running qemu-5.2 on Debian 11, so this output is for >> qemu 5.2, and that version of qemu has a piix3-ide-xen device. >> Is that this same device that is being removed? If so, it seems to >> me that at least as of qemu 5.2, the device was user-creatable. >> >> Chuck > > Good news! It looks the device was removed as user-creatable since version 5.2: > > chuckz@debian:~$ qemu-system-i386-7.50 -device help | grep piix > name "piix3-usb-uhci", bus PCI > name "piix4-usb-uhci", bus PCI > name "piix3-ide", bus PCI > name "piix4-ide", bus PCI > chuckz@debian:~$ qemu-system-i386-7.50-bernhard-v2 -device help | grep piix > name "piix3-usb-uhci", bus PCI > name "piix4-usb-uhci", bus PCI > name "piix3-ide", bus PCI > name "piix4-ide", bus PCI > chuckz@debian:~$ > > The piix3-ide-xen device is not present either with or without Bernhard's patches > for current qemu 7.50, the development version for qemu 8.0 > > Cheers, > > Chuck I traced where the pciix3-ide-xen device was removed: It was 7851b21a81 (hw/ide/piix: Remove redundant "piix3-ide-xen" device class) https://gitlab.com/qemu-project/qemu/-/commit/7851b21a8192750adecbcf6e8780a20de5891ad6 about six months ago. That was between 7.0 and 7.1. So the device being removed here is definitely not user-creatable, but it appears that this piix3-ide-xen device that was removed between 7.0 and 7.1 was user-creatable. Does that one need to go through the deprecation process? Or, since no one has complained it is gone, maybe we don't need to worry about it? Cheers, Chuck
Am 7. Januar 2023 01:08:46 UTC schrieb Chuck Zmudzinski <brchuckz@aol.com>: >On 1/6/23 6:04 PM, Chuck Zmudzinski wrote: >> On 1/6/23 2:08 PM, Chuck Zmudzinski wrote: >>> On 1/6/23 7:25 AM, Philippe Mathieu-Daudé wrote: >>>> On 6/1/23 12:57, Bernhard Beschow wrote: >>>>> >>>>> >>>>> Am 4. Januar 2023 15:35:33 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>: >>>>>> +Markus/Thomas >>>>>> >>>>>> On 4/1/23 15:44, Bernhard Beschow wrote: >>>>>>> During the last patches, TYPE_PIIX3_XEN_DEVICE turned into a clone of >>>>>>> TYPE_PIIX3_DEVICE. Remove this redundancy. >>>>>>> >>>>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >>>>>>> --- >>>>>>> hw/i386/pc_piix.c | 4 +--- >>>>>>> hw/isa/piix.c | 20 -------------------- >>>>>>> include/hw/southbridge/piix.h | 1 - >>>>>>> 3 files changed, 1 insertion(+), 24 deletions(-) >>>> >>>> >>>>>>> -static void piix3_xen_class_init(ObjectClass *klass, void *data) >>>>>>> -{ >>>>>>> - DeviceClass *dc = DEVICE_CLASS(klass); >>>>>>> - PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); >>>>>>> - >>>>>>> - k->realize = piix3_realize; >>>>>>> - /* 82371SB PIIX3 PCI-to-ISA bridge (Step A1) */ >>>>>>> - k->device_id = PCI_DEVICE_ID_INTEL_82371SB_0; >>>>>>> - dc->vmsd = &vmstate_piix3; >>>>>> >>>>>> IIUC, since this device is user-creatable, we can't simply remove it >>>>>> without going thru the deprecation process. >>>>> >>>>> AFAICS this device is actually not user-creatable since dc->user_creatable is set to false once in the base class. I think it is safe to remove the Xen class unless there are ABI issues. >>>> Great news! >>> >>> I don't know if this means the device is user-creatable: >>> >>> chuckz@bullseye:~$ qemu-system-i386 -device piix3-ide-xen,help >>> piix3-ide-xen options: >>> addr=<int32> - Slot and optional function number, example: 06.0 or 06 (default: -1) >>> failover_pair_id=<str> >>> multifunction=<bool> - on/off (default: false) >>> rombar=<uint32> - (default: 1) >>> romfile=<str> >>> x-pcie-extcap-init=<bool> - on/off (default: true) >>> x-pcie-lnksta-dllla=<bool> - on/off (default: true) >>> >>> Today I am running qemu-5.2 on Debian 11, so this output is for >>> qemu 5.2, and that version of qemu has a piix3-ide-xen device. >>> Is that this same device that is being removed? If so, it seems to >>> me that at least as of qemu 5.2, the device was user-creatable. >>> >>> Chuck >> >> Good news! It looks the device was removed as user-creatable since version 5.2: >> >> chuckz@debian:~$ qemu-system-i386-7.50 -device help | grep piix >> name "piix3-usb-uhci", bus PCI >> name "piix4-usb-uhci", bus PCI >> name "piix3-ide", bus PCI >> name "piix4-ide", bus PCI >> chuckz@debian:~$ qemu-system-i386-7.50-bernhard-v2 -device help | grep piix >> name "piix3-usb-uhci", bus PCI >> name "piix4-usb-uhci", bus PCI >> name "piix3-ide", bus PCI >> name "piix4-ide", bus PCI >> chuckz@debian:~$ >> >> The piix3-ide-xen device is not present either with or without Bernhard's patches >> for current qemu 7.50, the development version for qemu 8.0 >> >> Cheers, >> >> Chuck > > >I traced where the pciix3-ide-xen device was removed: > >It was 7851b21a81 (hw/ide/piix: Remove redundant "piix3-ide-xen" device class) > >https://gitlab.com/qemu-project/qemu/-/commit/7851b21a8192750adecbcf6e8780a20de5891ad6 > >about six months ago. That was between 7.0 and 7.1. So the device being removed >here is definitely not user-creatable, but it appears that this piix3-ide-xen >device that was removed between 7.0 and 7.1 was user-creatable. Does that one >need to go through the deprecation process? Or, since no one has complained >it is gone, maybe we don't need to worry about it? Good point! Looks like it fell through the cracks... There are voices who claim that this device and its non-Xen counterpart should have never been user-creatable in the firtst place: https://patchwork.kernel.org/project/qemu-devel/patch/20190718091740.6834-1-philmd@redhat.com/ Best regards, Bernhard > >Cheers, > >Chuck
On 1/7/23 6:05 AM, Bernhard Beschow wrote: > Am 7. Januar 2023 01:08:46 UTC schrieb Chuck Zmudzinski <brchuckz@aol.com>: > >On 1/6/23 6:04 PM, Chuck Zmudzinski wrote: > >> On 1/6/23 2:08 PM, Chuck Zmudzinski wrote: > >>> On 1/6/23 7:25 AM, Philippe Mathieu-Daudé wrote: > >>>> On 6/1/23 12:57, Bernhard Beschow wrote: > >>>>> > >>>>> > >>>>> Am 4. Januar 2023 15:35:33 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>: > >>>>>> +Markus/Thomas > >>>>>> > >>>>>> On 4/1/23 15:44, Bernhard Beschow wrote: > >>>>>>> During the last patches, TYPE_PIIX3_XEN_DEVICE turned into a clone of > >>>>>>> TYPE_PIIX3_DEVICE. Remove this redundancy. > >>>>>>> > >>>>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com> > >>>>>>> --- > >>>>>>> hw/i386/pc_piix.c | 4 +--- > >>>>>>> hw/isa/piix.c | 20 -------------------- > >>>>>>> include/hw/southbridge/piix.h | 1 - > >>>>>>> 3 files changed, 1 insertion(+), 24 deletions(-) > >>>> > >>>> > >>>>>>> -static void piix3_xen_class_init(ObjectClass *klass, void *data) > >>>>>>> -{ > >>>>>>> - DeviceClass *dc = DEVICE_CLASS(klass); > >>>>>>> - PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); > >>>>>>> - > >>>>>>> - k->realize = piix3_realize; > >>>>>>> - /* 82371SB PIIX3 PCI-to-ISA bridge (Step A1) */ > >>>>>>> - k->device_id = PCI_DEVICE_ID_INTEL_82371SB_0; > >>>>>>> - dc->vmsd = &vmstate_piix3; > >>>>>> > >>>>>> IIUC, since this device is user-creatable, we can't simply remove it > >>>>>> without going thru the deprecation process. > >>>>> > >>>>> AFAICS this device is actually not user-creatable since dc->user_creatable is set to false once in the base class. I think it is safe to remove the Xen class unless there are ABI issues. > >>>> Great news! > >>> > >>> I don't know if this means the device is user-creatable: > >>> > >>> chuckz@bullseye:~$ qemu-system-i386 -device piix3-ide-xen,help > >>> piix3-ide-xen options: > >>> addr=<int32> - Slot and optional function number, example: 06.0 or 06 (default: -1) > >>> failover_pair_id=<str> > >>> multifunction=<bool> - on/off (default: false) > >>> rombar=<uint32> - (default: 1) > >>> romfile=<str> > >>> x-pcie-extcap-init=<bool> - on/off (default: true) > >>> x-pcie-lnksta-dllla=<bool> - on/off (default: true) > >>> > >>> Today I am running qemu-5.2 on Debian 11, so this output is for > >>> qemu 5.2, and that version of qemu has a piix3-ide-xen device. > >>> Is that this same device that is being removed? If so, it seems to > >>> me that at least as of qemu 5.2, the device was user-creatable. > >>> > >>> Chuck > >> > >> Good news! It looks the device was removed as user-creatable since version 5.2: > >> > >> chuckz@debian:~$ qemu-system-i386-7.50 -device help | grep piix > >> name "piix3-usb-uhci", bus PCI > >> name "piix4-usb-uhci", bus PCI > >> name "piix3-ide", bus PCI > >> name "piix4-ide", bus PCI > >> chuckz@debian:~$ qemu-system-i386-7.50-bernhard-v2 -device help | grep piix > >> name "piix3-usb-uhci", bus PCI > >> name "piix4-usb-uhci", bus PCI > >> name "piix3-ide", bus PCI > >> name "piix4-ide", bus PCI > >> chuckz@debian:~$ > >> > >> The piix3-ide-xen device is not present either with or without Bernhard's patches > >> for current qemu 7.50, the development version for qemu 8.0 > >> > >> Cheers, > >> > >> Chuck > > > > > >I traced where the pciix3-ide-xen device was removed: > > > >It was 7851b21a81 (hw/ide/piix: Remove redundant "piix3-ide-xen" device class) > > > >https://gitlab.com/qemu-project/qemu/-/commit/7851b21a8192750adecbcf6e8780a20de5891ad6 > > > >about six months ago. That was between 7.0 and 7.1. So the device being removed > >here is definitely not user-creatable, but it appears that this piix3-ide-xen > >device that was removed between 7.0 and 7.1 was user-creatable. Does that one > >need to go through the deprecation process? Or, since no one has complained > >it is gone, maybe we don't need to worry about it? > > Good point! Looks like it fell through the cracks... > > There are voices who claim that this device and its non-Xen counterpart should have never been user-creatable in the firtst place: > https://patchwork.kernel.org/project/qemu-devel/patch/20190718091740.6834-1-philmd@redhat.com/ Of course, only the xen variant was removed, so only users of the xen variant were affected by the removal of the device. Any affected users probably just substituted the non-xen variant for the xen variant in their machines and didn't experience any problems. Kind regards, Chuck
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c index 5738d9cdca..6b8de3d59d 100644 --- a/hw/i386/pc_piix.c +++ b/hw/i386/pc_piix.c @@ -235,8 +235,6 @@ static void pc_init1(MachineState *machine, if (pcmc->pci_enabled) { DeviceState *dev; PCIDevice *pci_dev; - const char *type = xen_enabled() ? TYPE_PIIX3_XEN_DEVICE - : TYPE_PIIX3_DEVICE; int i; pci_bus = i440fx_init(pci_type, @@ -250,7 +248,7 @@ static void pc_init1(MachineState *machine, : pci_slot_get_pirq); pcms->bus = pci_bus; - pci_dev = pci_new_multifunction(-1, true, type); + pci_dev = pci_new_multifunction(-1, true, TYPE_PIIX3_DEVICE); object_property_set_bool(OBJECT(pci_dev), "has-usb", machine_usb(machine), &error_abort); object_property_set_bool(OBJECT(pci_dev), "has-acpi", diff --git a/hw/isa/piix.c b/hw/isa/piix.c index 98e9b12661..e4587352c9 100644 --- a/hw/isa/piix.c +++ b/hw/isa/piix.c @@ -33,7 +33,6 @@ #include "hw/qdev-properties.h" #include "hw/ide/piix.h" #include "hw/isa/isa.h" -#include "hw/xen/xen.h" #include "sysemu/runstate.h" #include "migration/vmstate.h" #include "hw/acpi/acpi_aml_interface.h" @@ -465,24 +464,6 @@ static const TypeInfo piix3_info = { .class_init = piix3_class_init, }; -static void piix3_xen_class_init(ObjectClass *klass, void *data) -{ - DeviceClass *dc = DEVICE_CLASS(klass); - PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); - - k->realize = piix3_realize; - /* 82371SB PIIX3 PCI-to-ISA bridge (Step A1) */ - k->device_id = PCI_DEVICE_ID_INTEL_82371SB_0; - dc->vmsd = &vmstate_piix3; -} - -static const TypeInfo piix3_xen_info = { - .name = TYPE_PIIX3_XEN_DEVICE, - .parent = TYPE_PIIX_PCI_DEVICE, - .instance_init = piix3_init, - .class_init = piix3_xen_class_init, -}; - static void piix4_realize(PCIDevice *dev, Error **errp) { ERRP_GUARD(); @@ -534,7 +515,6 @@ static void piix3_register_types(void) { type_register_static(&piix_pci_type_info); type_register_static(&piix3_info); - type_register_static(&piix3_xen_info); type_register_static(&piix4_info); } diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h index 65ad8569da..b1fc94a742 100644 --- a/include/hw/southbridge/piix.h +++ b/include/hw/southbridge/piix.h @@ -77,7 +77,6 @@ struct PIIXState { OBJECT_DECLARE_SIMPLE_TYPE(PIIXState, PIIX_PCI_DEVICE) #define TYPE_PIIX3_DEVICE "PIIX3" -#define TYPE_PIIX3_XEN_DEVICE "PIIX3-xen" #define TYPE_PIIX4_PCI_DEVICE "piix4-isa" #endif
During the last patches, TYPE_PIIX3_XEN_DEVICE turned into a clone of TYPE_PIIX3_DEVICE. Remove this redundancy. Signed-off-by: Bernhard Beschow <shentey@gmail.com> --- hw/i386/pc_piix.c | 4 +--- hw/isa/piix.c | 20 -------------------- include/hw/southbridge/piix.h | 1 - 3 files changed, 1 insertion(+), 24 deletions(-)