diff mbox series

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

Message ID 20230104144437.27479-7-shentey@gmail.com (mailing list archive)
State New, archived
Headers show
Series Resolve TYPE_PIIX3_XEN_DEVICE | expand

Commit Message

Bernhard Beschow Jan. 4, 2023, 2:44 p.m. UTC
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(-)

Comments

Philippe Mathieu-Daudé Jan. 4, 2023, 3:35 p.m. UTC | #1
+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
Chuck Zmudzinski Jan. 4, 2023, 4:42 p.m. UTC | #2
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>
Chuck Zmudzinski Jan. 4, 2023, 5:54 p.m. UTC | #3
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
>
Philippe Mathieu-Daudé Jan. 4, 2023, 6:48 p.m. UTC | #4
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.
Chuck Zmudzinski Jan. 4, 2023, 7:29 p.m. UTC | #5
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.
Bernhard Beschow Jan. 4, 2023, 7:45 p.m. UTC | #6
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
Bernhard Beschow Jan. 4, 2023, 8:31 p.m. UTC | #7
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.
Bernhard Beschow Jan. 4, 2023, 8:44 p.m. UTC | #8
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
>> 
>
Chuck Zmudzinski Jan. 4, 2023, 8:50 p.m. UTC | #9
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
Chuck Zmudzinski Jan. 4, 2023, 8:57 p.m. UTC | #10
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
Philippe Mathieu-Daudé Jan. 4, 2023, 10:18 p.m. UTC | #11
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.
Philippe Mathieu-Daudé Jan. 4, 2023, 10:35 p.m. UTC | #12
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)
Chuck Zmudzinski Jan. 5, 2023, 1:57 a.m. UTC | #13
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.
Chuck Zmudzinski Jan. 5, 2023, 2:25 a.m. UTC | #14
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
Chuck Zmudzinski Jan. 5, 2023, 2:34 a.m. UTC | #15
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
Bernhard Beschow Jan. 6, 2023, 11:57 a.m. UTC | #16
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
>
Philippe Mathieu-Daudé Jan. 6, 2023, 12:25 p.m. UTC | #17
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!
Chuck Zmudzinski Jan. 6, 2023, 7:08 p.m. UTC | #18
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
Chuck Zmudzinski Jan. 6, 2023, 11:04 p.m. UTC | #19
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
Chuck Zmudzinski Jan. 7, 2023, 1:08 a.m. UTC | #20
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
Bernhard Beschow Jan. 7, 2023, 11:05 a.m. UTC | #21
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
Chuck Zmudzinski Jan. 7, 2023, 6:08 p.m. UTC | #22
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 mbox series

Patch

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