Re: [PATCH v2 6/6] hw/isa/piix: Resolve redundant TYPE_PIIX3_XEN_DEVICE

[Bernhard Beschow <shentey@xxxxxxxxx>]

Am 4. Januar 2023 17:54:16 UTC schrieb Chuck Zmudzinski <brchuckz@xxxxxxx>:
>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@xxxxxxxxx>
>>> ---
>>>   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
>> 
>