[PATCH v2 0/6] Resolve TYPE_PIIX3_XEN_DEVICE

[PATCH v2 0/6] Resolve TYPE_PIIX3_XEN_DEVICE

[Bernhard Beschow]

This series first renders TYPE_PIIX3_XEN_DEVICE redundant and finally removes
it. The motivation is to 1/ decouple PIIX from Xen and 2/ to make Xen in the PC
machine agnostic to the precise southbridge being used. 2/ will become
particularily interesting once PIIX4 becomes usable in the PC machine, avoiding
the "Frankenstein" use of PIIX4_ACPI in PIIX3.

v2:
- xen_piix3_set_irq() is already generic. Just rename it. (Chuck)

Testing done:
None, because I don't know how to conduct this properly :(

Based-on: <20221221170003.2929-1-shentey@gmail.com>
          "[PATCH v4 00/30] Consolidate PIIX south bridges"

Bernhard Beschow (6):
  include/hw/xen/xen: Rename xen_piix3_set_irq() to xen_intx_set_irq()
  hw/isa/piix: Reuse piix3_realize() in piix3_xen_realize()
  hw/isa/piix: Wire up Xen PCI IRQ handling outside of PIIX3
  hw/isa/piix: Avoid Xen-specific variant of piix_write_config()
  hw/isa/piix: Resolve redundant k->config_write assignments
  hw/isa/piix: Resolve redundant TYPE_PIIX3_XEN_DEVICE

 hw/i386/pc_piix.c             | 34 ++++++++++++++++--
 hw/i386/xen/xen-hvm.c         |  2 +-
 hw/isa/piix.c                 | 66 +----------------------------------
 include/hw/southbridge/piix.h |  1 -
 include/hw/xen/xen.h          |  2 +-
 stubs/xen-hw-stub.c           |  2 +-
 6 files changed, 35 insertions(+), 72 deletions(-)

-- 
2.39.0

[PATCH v2 1/6] include/hw/xen/xen: Rename xen_piix3_set_irq() to xen_intx_set_irq()

[Bernhard Beschow]

xen_piix3_set_irq() isn't PIIX specific: PIIX is a single PCI device
while xen_piix3_set_irq() maps multiple PCI devices to their respective
IRQs, which is board-specific. Rename xen_piix3_set_irq() to communicate
this.

Also rename XEN_PIIX_NUM_PIRQS to XEN_IOAPIC_NUM_PIRQS since the Xen's
IOAPIC rather than PIIX has this many interrupt routes.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/i386/xen/xen-hvm.c | 2 +-
 hw/isa/piix.c         | 4 ++--
 include/hw/xen/xen.h  | 2 +-
 stubs/xen-hw-stub.c   | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index e4293d6d66..558c43309e 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -142,7 +142,7 @@ int xen_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num)
     return irq_num + (PCI_SLOT(pci_dev->devfn) << 2);
 }
 
-void xen_piix3_set_irq(void *opaque, int irq_num, int level)
+void xen_intx_set_irq(void *opaque, int irq_num, int level)
 {
     xen_set_pci_intx_level(xen_domid, 0, 0, irq_num >> 2,
                            irq_num & 3, level);
diff --git a/hw/isa/piix.c b/hw/isa/piix.c
index ae8a27c53c..a7a4eec206 100644
--- a/hw/isa/piix.c
+++ b/hw/isa/piix.c
@@ -38,7 +38,7 @@
 #include "migration/vmstate.h"
 #include "hw/acpi/acpi_aml_interface.h"
 
-#define XEN_PIIX_NUM_PIRQS      128ULL
+#define XEN_IOAPIC_NUM_PIRQS    128ULL
 
 static void piix_set_irq_pic(PIIXState *piix, int pic_irq)
 {
@@ -504,7 +504,7 @@ static void piix3_xen_realize(PCIDevice *dev, Error **errp)
      * connected to the IOAPIC directly.
      * These additional routes can be discovered through ACPI.
      */
-    pci_bus_irqs(pci_bus, xen_piix3_set_irq, piix3, XEN_PIIX_NUM_PIRQS);
+    pci_bus_irqs(pci_bus, xen_intx_set_irq, piix3, XEN_IOAPIC_NUM_PIRQS);
 }
 
 static void piix3_xen_class_init(ObjectClass *klass, void *data)
diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
index afdf9c436a..7c83ecf6b9 100644
--- a/include/hw/xen/xen.h
+++ b/include/hw/xen/xen.h
@@ -22,7 +22,7 @@ extern bool xen_domid_restrict;
 
 int xen_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num);
 int xen_set_pci_link_route(uint8_t link, uint8_t irq);
-void xen_piix3_set_irq(void *opaque, int irq_num, int level);
+void xen_intx_set_irq(void *opaque, int irq_num, int level);
 void xen_hvm_inject_msi(uint64_t addr, uint32_t data);
 int xen_is_pirq_msi(uint32_t msi_data);
 
diff --git a/stubs/xen-hw-stub.c b/stubs/xen-hw-stub.c
index 34a22f2ad7..7d7ffe83a9 100644
--- a/stubs/xen-hw-stub.c
+++ b/stubs/xen-hw-stub.c
@@ -15,7 +15,7 @@ int xen_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num)
     return -1;
 }
 
-void xen_piix3_set_irq(void *opaque, int irq_num, int level)
+void xen_intx_set_irq(void *opaque, int irq_num, int level)
 {
 }
 
-- 
2.39.0

[PATCH v2 2/6] hw/isa/piix: Reuse piix3_realize() in piix3_xen_realize()

[Bernhard Beschow]

This is a preparational patch for the next one to make the following
more obvious:

First, pci_bus_irqs() is now called twice in case of Xen where the
second call overrides the pci_set_irq_fn with the Xen variant.

Second, pci_bus_set_route_irq_fn() is now also called in Xen mode.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/isa/piix.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/isa/piix.c b/hw/isa/piix.c
index a7a4eec206..25707479eb 100644
--- a/hw/isa/piix.c
+++ b/hw/isa/piix.c
@@ -493,7 +493,7 @@ static void piix3_xen_realize(PCIDevice *dev, Error **errp)
     PIIXState *piix3 = PIIX_PCI_DEVICE(dev);
     PCIBus *pci_bus = pci_get_bus(dev);
 
-    pci_piix_realize(dev, TYPE_PIIX3_USB_UHCI, errp);
+    piix3_realize(dev, errp);
     if (*errp) {
         return;
     }
-- 
2.39.0

[PATCH v2 3/6] hw/isa/piix: Wire up Xen PCI IRQ handling outside of PIIX3

[Bernhard Beschow]

xen_intx_set_irq() doesn't depend on PIIX state. In order to resolve
TYPE_PIIX3_XEN_DEVICE and in order to make Xen agnostic about the
precise south bridge being used, set up Xen's PCI IRQ handling of PIIX3
in the board.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/i386/pc_piix.c | 12 ++++++++++++
 hw/isa/piix.c     | 24 +-----------------------
 2 files changed, 13 insertions(+), 23 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index aacdb72b7c..792dcd3ce8 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -67,6 +67,7 @@
 #include "kvm/kvm-cpu.h"
 
 #define MAX_IDE_BUS 2
+#define XEN_IOAPIC_NUM_PIRQS 128ULL
 
 #ifdef CONFIG_IDE_ISA
 static const int ide_iobase[MAX_IDE_BUS] = { 0x1f0, 0x170 };
@@ -246,6 +247,17 @@ static void pc_init1(MachineState *machine,
                                  &error_abort);
         pci_realize_and_unref(pci_dev, pci_bus, &error_fatal);
 
+        if (xen_enabled()) {
+            /*
+             * Xen supports additional interrupt routes from the PCI devices to
+             * the IOAPIC: the four pins of each PCI device on the bus are also
+             * connected to the IOAPIC directly.
+             * These additional routes can be discovered through ACPI.
+             */
+            pci_bus_irqs(pci_bus, xen_intx_set_irq, pci_dev,
+                         XEN_IOAPIC_NUM_PIRQS);
+        }
+
         dev = DEVICE(object_resolve_path_component(OBJECT(pci_dev), "pic"));
         for (i = 0; i < ISA_NUM_IRQS; i++) {
             qdev_connect_gpio_out(dev, i, x86ms->gsi[i]);
diff --git a/hw/isa/piix.c b/hw/isa/piix.c
index 25707479eb..ac04781f46 100644
--- a/hw/isa/piix.c
+++ b/hw/isa/piix.c
@@ -38,8 +38,6 @@
 #include "migration/vmstate.h"
 #include "hw/acpi/acpi_aml_interface.h"
 
-#define XEN_IOAPIC_NUM_PIRQS    128ULL
-
 static void piix_set_irq_pic(PIIXState *piix, int pic_irq)
 {
     qemu_set_irq(piix->pic.in_irqs[pic_irq],
@@ -487,33 +485,13 @@ static const TypeInfo piix3_info = {
     .class_init    = piix3_class_init,
 };
 
-static void piix3_xen_realize(PCIDevice *dev, Error **errp)
-{
-    ERRP_GUARD();
-    PIIXState *piix3 = PIIX_PCI_DEVICE(dev);
-    PCIBus *pci_bus = pci_get_bus(dev);
-
-    piix3_realize(dev, errp);
-    if (*errp) {
-        return;
-    }
-
-    /*
-     * Xen supports additional interrupt routes from the PCI devices to
-     * the IOAPIC: the four pins of each PCI device on the bus are also
-     * connected to the IOAPIC directly.
-     * These additional routes can be discovered through ACPI.
-     */
-    pci_bus_irqs(pci_bus, xen_intx_set_irq, piix3, XEN_IOAPIC_NUM_PIRQS);
-}
-
 static void piix3_xen_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
     k->config_write = piix3_write_config_xen;
-    k->realize = piix3_xen_realize;
+    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;
-- 
2.39.0

[PATCH v2 4/6] hw/isa/piix: Avoid Xen-specific variant of piix_write_config()

[Bernhard Beschow]

Subscribe to pci_bus_fire_intx_routing_notifier() instead which allows for
having a common piix_write_config() for all PIIX device models.

While at it, move the subscription into machine code in order to resolve
TYPE_PIIX3_XEN_DEVICE.

In a possible future followup, pci_bus_fire_intx_routing_notifier() could
be adjusted in such a way that subscribing to it doesn't require
knowledge of the device firing it.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/i386/pc_piix.c | 18 ++++++++++++++++++
 hw/isa/piix.c     | 22 +---------------------
 2 files changed, 19 insertions(+), 21 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 792dcd3ce8..5738d9cdca 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -86,6 +86,21 @@ static int pci_slot_get_pirq(PCIDevice *pci_dev, int pci_intx)
     return (pci_intx + slot_addend) & 3;
 }
 
+static void piix_intx_routing_notifier_xen(PCIDevice *dev)
+{
+    int i;
+
+    /* Scan for updates to PCI link routes (0x60-0x63). */
+    for (i = 0; i < PIIX_NUM_PIRQS; i++) {
+        uint8_t v = dev->config_read(dev, PIIX_PIRQCA + i, 1);
+        if (v & 0x80) {
+            v = 0;
+        }
+        v &= 0xf;
+        xen_set_pci_link_route(i, v);
+    }
+}
+
 /* PC hardware initialisation */
 static void pc_init1(MachineState *machine,
                      const char *host_type, const char *pci_type)
@@ -248,6 +263,9 @@ static void pc_init1(MachineState *machine,
         pci_realize_and_unref(pci_dev, pci_bus, &error_fatal);
 
         if (xen_enabled()) {
+            pci_device_set_intx_routing_notifier(
+                        pci_dev, piix_intx_routing_notifier_xen);
+
             /*
              * Xen supports additional interrupt routes from the PCI devices to
              * the IOAPIC: the four pins of each PCI device on the bus are also
diff --git a/hw/isa/piix.c b/hw/isa/piix.c
index ac04781f46..d4cdb3dadb 100644
--- a/hw/isa/piix.c
+++ b/hw/isa/piix.c
@@ -125,26 +125,6 @@ static void piix_write_config(PCIDevice *dev, uint32_t address, uint32_t val,
     }
 }
 
-static void piix3_write_config_xen(PCIDevice *dev,
-                                   uint32_t address, uint32_t val, int len)
-{
-    int i;
-
-    /* Scan for updates to PCI link routes (0x60-0x63). */
-    for (i = 0; i < len; i++) {
-        uint8_t v = (val >> (8 * i)) & 0xff;
-        if (v & 0x80) {
-            v = 0;
-        }
-        v &= 0xf;
-        if (((address + i) >= PIIX_PIRQCA) && ((address + i) <= PIIX_PIRQCD)) {
-            xen_set_pci_link_route(address + i - PIIX_PIRQCA, v);
-        }
-    }
-
-    piix_write_config(dev, address, val, len);
-}
-
 static void piix_reset(DeviceState *dev)
 {
     PIIXState *d = PIIX_PCI_DEVICE(dev);
@@ -490,7 +470,7 @@ static void piix3_xen_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-    k->config_write = piix3_write_config_xen;
+    k->config_write = piix_write_config;
     k->realize = piix3_realize;
     /* 82371SB PIIX3 PCI-to-ISA bridge (Step A1) */
     k->device_id = PCI_DEVICE_ID_INTEL_82371SB_0;
-- 
2.39.0

[PATCH v2 5/6] hw/isa/piix: Resolve redundant k->config_write assignments

[Bernhard Beschow]

The previous patch unified handling of piix_write_config() accross all
PIIX device models which allows for assigning k->config_write once in the
base class.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/isa/piix.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/isa/piix.c b/hw/isa/piix.c
index d4cdb3dadb..98e9b12661 100644
--- a/hw/isa/piix.c
+++ b/hw/isa/piix.c
@@ -396,6 +396,7 @@ static void pci_piix_class_init(ObjectClass *klass, void *data)
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
     AcpiDevAmlIfClass *adevc = ACPI_DEV_AML_IF_CLASS(klass);
 
+    k->config_write = piix_write_config;
     dc->reset       = piix_reset;
     dc->desc        = "ISA bridge";
     dc->hotpluggable   = false;
@@ -451,7 +452,6 @@ static void piix3_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-    k->config_write = piix_write_config;
     k->realize = piix3_realize;
     /* 82371SB PIIX3 PCI-to-ISA bridge (Step A1) */
     k->device_id = PCI_DEVICE_ID_INTEL_82371SB_0;
@@ -470,7 +470,6 @@ static void piix3_xen_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-    k->config_write = piix_write_config;
     k->realize = piix3_realize;
     /* 82371SB PIIX3 PCI-to-ISA bridge (Step A1) */
     k->device_id = PCI_DEVICE_ID_INTEL_82371SB_0;
@@ -519,7 +518,6 @@ static void piix4_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-    k->config_write = piix_write_config;
     k->realize = piix4_realize;
     k->device_id = PCI_DEVICE_ID_INTEL_82371AB_0;
     dc->vmsd = &vmstate_piix4;
-- 
2.39.0

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

[Bernhard Beschow]

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
-- 
2.39.0

Re: [PATCH v2 1/6] include/hw/xen/xen: Rename xen_piix3_set_irq() to xen_intx_set_irq()

[Philippe Mathieu-Daudé]

On 4/1/23 15:44, Bernhard Beschow wrote:
> xen_piix3_set_irq() isn't PIIX specific: PIIX is a single PCI device
> while xen_piix3_set_irq() maps multiple PCI devices to their respective
> IRQs, which is board-specific. Rename xen_piix3_set_irq() to communicate
> this.
> 
> Also rename XEN_PIIX_NUM_PIRQS to XEN_IOAPIC_NUM_PIRQS since the Xen's
> IOAPIC rather than PIIX has this many interrupt routes.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/i386/xen/xen-hvm.c | 2 +-
>   hw/isa/piix.c         | 4 ++--
>   include/hw/xen/xen.h  | 2 +-
>   stubs/xen-hw-stub.c   | 2 +-
>   4 files changed, 5 insertions(+), 5 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v2 3/6] hw/isa/piix: Wire up Xen PCI IRQ handling outside of PIIX3

[Philippe Mathieu-Daudé]

On 4/1/23 15:44, Bernhard Beschow wrote:
> xen_intx_set_irq() doesn't depend on PIIX state. In order to resolve
> TYPE_PIIX3_XEN_DEVICE and in order to make Xen agnostic about the
> precise south bridge being used, set up Xen's PCI IRQ handling of PIIX3
> in the board.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/i386/pc_piix.c | 12 ++++++++++++
>   hw/isa/piix.c     | 24 +-----------------------
>   2 files changed, 13 insertions(+), 23 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v2 5/6] hw/isa/piix: Resolve redundant k->config_write assignments

[Philippe Mathieu-Daudé]

On 4/1/23 15:44, Bernhard Beschow wrote:
> The previous patch unified handling of piix_write_config() accross all
> PIIX device models which allows for assigning k->config_write once in the
> base class.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/isa/piix.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

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

[Philippe Mathieu-Daudé]

+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

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

[Chuck Zmudzinski]

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>

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

[Chuck Zmudzinski]

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
> 

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

[Philippe Mathieu-Daudé]

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.

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

[Chuck Zmudzinski]

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.

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

[Bernhard Beschow]

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

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

[Bernhard Beschow]

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.

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

[Bernhard Beschow]

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
>> 
>

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

[Chuck Zmudzinski]

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

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

[Chuck Zmudzinski]

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

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

[Philippe Mathieu-Daudé]

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.

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

[Philippe Mathieu-Daudé]

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)

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

[Chuck Zmudzinski]

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.

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

[Chuck Zmudzinski]

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

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

[Chuck Zmudzinski]

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

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

[Bernhard Beschow]

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
>

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

[Philippe Mathieu-Daudé]

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!

Re: [PATCH v2 3/6] hw/isa/piix: Wire up Xen PCI IRQ handling outside of PIIX3

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 193 bytes --]

On Wed, 2023-01-04 at 15:44 +0100, Bernhard Beschow wrote:
> +        if (xen_enabled()) {

Could this perhaps be if (xen_mode != XEN_DISABLED) once we merge the
Xen-on-KVM series?

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

Re: [PATCH v2 3/6] hw/isa/piix: Wire up Xen PCI IRQ handling outside of PIIX3

[Chuck Zmudzinski]

On 1/6/23 12:35 PM, David Woodhouse wrote:
> On Wed, 2023-01-04 at 15:44 +0100, Bernhard Beschow wrote:
>> +        if (xen_enabled()) {
> 
> Could this perhaps be if (xen_mode != XEN_DISABLED) once we merge the
> Xen-on-KVM series?

I am not an expert and just on here as a user/tester, but I think it
would depend on whether or not the Xen-on-KVM mode uses Xen PCI IRQ
handling or Linux/KVM PCI IRQ handling.

Chuck

Re: [PATCH v2 3/6] hw/isa/piix: Wire up Xen PCI IRQ handling outside of PIIX3

[Chuck Zmudzinski]

On 1/6/23 12:46 PM, Chuck Zmudzinski wrote:
> On 1/6/23 12:35 PM, David Woodhouse wrote:
>> On Wed, 2023-01-04 at 15:44 +0100, Bernhard Beschow wrote:
>>> +        if (xen_enabled()) {
>> 
>> Could this perhaps be if (xen_mode != XEN_DISABLED) once we merge the
>> Xen-on-KVM series?
> 
> I am not an expert and just on here as a user/tester, but I think it
> would depend on whether or not the Xen-on-KVM mode uses Xen PCI IRQ
> handling or Linux/KVM PCI IRQ handling.
> 
> Chuck

I could try Bernhard's patches in a Xen-on-KVM configuration if that might help.
I would need help configuring the Xen-on-KVM mode to do it, though, because
I have never tried Xen-on-KVM before.

The test I could do would be to:

1) Test my Xen guest that uses the current PIIX3-xen device in the
Xen-on-KVM environment and get that working.

2) Apply Bernhard's patch series and see what, if anything, needs to
be done to make Bernhard's patch work in Xen-on-KVM. I have already
verified that Bernhard's patches work well with Xen on bare metal, so I
don't think we need to answer this question before going forward with
Bernhard's patches. This can be patched later to support Xen-on-KVM if
necessary as part of or in addition to the Xen-on-KVM series.

Chuck

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

[Chuck Zmudzinski]

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

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

[Chuck Zmudzinski]

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

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

[Chuck Zmudzinski]

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

Re: [PATCH v2 3/6] hw/isa/piix: Wire up Xen PCI IRQ handling outside of PIIX3

[Bernhard Beschow]

Am 6. Januar 2023 17:35:18 UTC schrieb David Woodhouse <dwmw2@infradead.org>:
>On Wed, 2023-01-04 at 15:44 +0100, Bernhard Beschow wrote:
>> +        if (xen_enabled()) {
>
>Could this perhaps be if (xen_mode != XEN_DISABLED) once we merge the
>Xen-on-KVM series?

It's the same condition which selected TYPE_PIIX3_XEN_DEVICE until the last patch of this series. If you had to change this condition in your series then just perform the same change here instead.

Best regards,
Bernhard

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

[Bernhard Beschow]

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

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

[Chuck Zmudzinski]

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

Re: [PATCH v2 0/6] Resolve TYPE_PIIX3_XEN_DEVICE

[Bernhard Beschow]

Am 4. Januar 2023 14:44:31 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>This series first renders TYPE_PIIX3_XEN_DEVICE redundant and finally removes
>
>it. The motivation is to 1/ decouple PIIX from Xen and 2/ to make Xen in the PC
>
>machine agnostic to the precise southbridge being used. 2/ will become
>
>particularily interesting once PIIX4 becomes usable in the PC machine, avoiding
>
>the "Frankenstein" use of PIIX4_ACPI in PIIX3.
>
>
>
>v2:
>
>- xen_piix3_set_irq() is already generic. Just rename it. (Chuck)
>
>
>
>Testing done:
>
>None, because I don't know how to conduct this properly :(

Ping

Successfully tested by Chuck. Patches 2, 4 and 6 still need review.

I can rebase to master if that eases review -- please let me know. Currently this series is based on my "Consolidate PIIX south bridges" series:

>Based-on: <20221221170003.2929-1-shentey@gmail.com>
>
>          "[PATCH v4 00/30] Consolidate PIIX south bridges"
>
>
>
>Bernhard Beschow (6):
>
>  include/hw/xen/xen: Rename xen_piix3_set_irq() to xen_intx_set_irq()
>
>  hw/isa/piix: Reuse piix3_realize() in piix3_xen_realize()
>
>  hw/isa/piix: Wire up Xen PCI IRQ handling outside of PIIX3
>
>  hw/isa/piix: Avoid Xen-specific variant of piix_write_config()
>
>  hw/isa/piix: Resolve redundant k->config_write assignments
>
>  hw/isa/piix: Resolve redundant TYPE_PIIX3_XEN_DEVICE
>
>
>
> hw/i386/pc_piix.c             | 34 ++++++++++++++++--
>
> hw/i386/xen/xen-hvm.c         |  2 +-
>
> hw/isa/piix.c                 | 66 +----------------------------------
>
> include/hw/southbridge/piix.h |  1 -
>
> include/hw/xen/xen.h          |  2 +-
>
> stubs/xen-hw-stub.c           |  2 +-
>
> 6 files changed, 35 insertions(+), 72 deletions(-)
>
>
>
>-- >
>2.39.0
>
>
>

Re: [PATCH v2 0/6] Resolve TYPE_PIIX3_XEN_DEVICE

[Michael S. Tsirkin]

On Wed, Jan 04, 2023 at 03:44:31PM +0100, Bernhard Beschow wrote:
> This series first renders TYPE_PIIX3_XEN_DEVICE redundant and finally removes
> it. The motivation is to 1/ decouple PIIX from Xen and 2/ to make Xen in the PC
> machine agnostic to the precise southbridge being used. 2/ will become
> particularily interesting once PIIX4 becomes usable in the PC machine, avoiding
> the "Frankenstein" use of PIIX4_ACPI in PIIX3.

Looks ok to me.
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

Feel free to merge through Xen tree.

> v2:
> - xen_piix3_set_irq() is already generic. Just rename it. (Chuck)
> 
> Testing done:
> None, because I don't know how to conduct this properly :(
> 
> Based-on: <20221221170003.2929-1-shentey@gmail.com>
>           "[PATCH v4 00/30] Consolidate PIIX south bridges"
> 
> Bernhard Beschow (6):
>   include/hw/xen/xen: Rename xen_piix3_set_irq() to xen_intx_set_irq()
>   hw/isa/piix: Reuse piix3_realize() in piix3_xen_realize()
>   hw/isa/piix: Wire up Xen PCI IRQ handling outside of PIIX3
>   hw/isa/piix: Avoid Xen-specific variant of piix_write_config()
>   hw/isa/piix: Resolve redundant k->config_write assignments
>   hw/isa/piix: Resolve redundant TYPE_PIIX3_XEN_DEVICE
> 
>  hw/i386/pc_piix.c             | 34 ++++++++++++++++--
>  hw/i386/xen/xen-hvm.c         |  2 +-
>  hw/isa/piix.c                 | 66 +----------------------------------
>  include/hw/southbridge/piix.h |  1 -
>  include/hw/xen/xen.h          |  2 +-
>  stubs/xen-hw-stub.c           |  2 +-
>  6 files changed, 35 insertions(+), 72 deletions(-)
> 
> -- 
> 2.39.0
> 

Re: [PATCH v2 0/6] Resolve TYPE_PIIX3_XEN_DEVICE

[Anthony PERARD via]

On Wed, Jan 18, 2023 at 05:13:03AM -0500, Michael S. Tsirkin wrote:
> On Wed, Jan 04, 2023 at 03:44:31PM +0100, Bernhard Beschow wrote:
> > This series first renders TYPE_PIIX3_XEN_DEVICE redundant and finally removes
> > it. The motivation is to 1/ decouple PIIX from Xen and 2/ to make Xen in the PC
> > machine agnostic to the precise southbridge being used. 2/ will become
> > particularily interesting once PIIX4 becomes usable in the PC machine, avoiding
> > the "Frankenstein" use of PIIX4_ACPI in PIIX3.
> 
> Looks ok to me.
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> 
> Feel free to merge through Xen tree.

Hi Bernhard,

The series currently doesn't apply on master. And a quick try at
applying the series it is based on also failed. Could you rebase it , or
maybe you would prefer to wait until the other series "Consolidate
PIIX..." is fully applied?

Thanks.

> > Testing done:
> > None, because I don't know how to conduct this properly :(
> > 
> > Based-on: <20221221170003.2929-1-shentey@gmail.com>
> >           "[PATCH v4 00/30] Consolidate PIIX south bridges"

-- 
Anthony PERARD

Re: [PATCH v2 0/6] Resolve TYPE_PIIX3_XEN_DEVICE

[Anthony PERARD]

On Wed, Jan 18, 2023 at 05:13:03AM -0500, Michael S. Tsirkin wrote:
> On Wed, Jan 04, 2023 at 03:44:31PM +0100, Bernhard Beschow wrote:
> > This series first renders TYPE_PIIX3_XEN_DEVICE redundant and finally removes
> > it. The motivation is to 1/ decouple PIIX from Xen and 2/ to make Xen in the PC
> > machine agnostic to the precise southbridge being used. 2/ will become
> > particularily interesting once PIIX4 becomes usable in the PC machine, avoiding
> > the "Frankenstein" use of PIIX4_ACPI in PIIX3.
> 
> Looks ok to me.
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> 
> Feel free to merge through Xen tree.

Hi Bernhard,

The series currently doesn't apply on master. And a quick try at
applying the series it is based on also failed. Could you rebase it , or
maybe you would prefer to wait until the other series "Consolidate
PIIX..." is fully applied?

Thanks.

> > Testing done:
> > None, because I don't know how to conduct this properly :(
> > 
> > Based-on: <20221221170003.2929-1-shentey@gmail.com>
> >           "[PATCH v4 00/30] Consolidate PIIX south bridges"

-- 
Anthony PERARD

Re: [PATCH v2 0/6] Resolve TYPE_PIIX3_XEN_DEVICE

[Bernhard Beschow]

Am 24. Januar 2023 16:11:47 UTC schrieb Anthony PERARD <anthony.perard@citrix.com>:
>On Wed, Jan 18, 2023 at 05:13:03AM -0500, Michael S. Tsirkin wrote:
>> On Wed, Jan 04, 2023 at 03:44:31PM +0100, Bernhard Beschow wrote:
>> > This series first renders TYPE_PIIX3_XEN_DEVICE redundant and finally removes
>> > it. The motivation is to 1/ decouple PIIX from Xen and 2/ to make Xen in the PC
>> > machine agnostic to the precise southbridge being used. 2/ will become
>> > particularily interesting once PIIX4 becomes usable in the PC machine, avoiding
>> > the "Frankenstein" use of PIIX4_ACPI in PIIX3.
>> 
>> Looks ok to me.
>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>> 
>> Feel free to merge through Xen tree.
>
>Hi Bernhard,

Hi Anthony,

>The series currently doesn't apply on master. And a quick try at
>applying the series it is based on also failed. Could you rebase it , or
>maybe you would prefer to wait until the other series "Consolidate
>PIIX..." is fully applied?

Thanks for looking into it!

You can get the compilable series from https://patchew.org/QEMU/20230104144437.27479-1-shentey@gmail.com/ . If it doesn't work for you let me know, then I can rebase onto master. All necessary dependencies for the series are upstreamed meanwhile.

Thanks,
Bernhard
>
>Thanks.
>
>> > Testing done:
>> > None, because I don't know how to conduct this properly :(
>> > 
>> > Based-on: <20221221170003.2929-1-shentey@gmail.com>
>> >           "[PATCH v4 00/30] Consolidate PIIX south bridges"
>

Re: [PATCH v2 0/6] Resolve TYPE_PIIX3_XEN_DEVICE

[Bernhard Beschow]

Am 24. Januar 2023 17:07:30 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>
>
>Am 24. Januar 2023 16:11:47 UTC schrieb Anthony PERARD <anthony.perard@citrix.com>:
>>On Wed, Jan 18, 2023 at 05:13:03AM -0500, Michael S. Tsirkin wrote:
>>> On Wed, Jan 04, 2023 at 03:44:31PM +0100, Bernhard Beschow wrote:
>>> > This series first renders TYPE_PIIX3_XEN_DEVICE redundant and finally removes
>>> > it. The motivation is to 1/ decouple PIIX from Xen and 2/ to make Xen in the PC
>>> > machine agnostic to the precise southbridge being used. 2/ will become
>>> > particularily interesting once PIIX4 becomes usable in the PC machine, avoiding
>>> > the "Frankenstein" use of PIIX4_ACPI in PIIX3.
>>> 
>>> Looks ok to me.
>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>>> 
>>> Feel free to merge through Xen tree.
>>
>>Hi Bernhard,
>
>Hi Anthony,
>
>>The series currently doesn't apply on master. And a quick try at
>>applying the series it is based on also failed. Could you rebase it , or
>>maybe you would prefer to wait until the other series "Consolidate
>>PIIX..." is fully applied?
>
>Thanks for looking into it!
>
>You can get the compilable series from https://patchew.org/QEMU/20230104144437.27479-1-shentey@gmail.com/ . If it doesn't work for you let me know, then I can rebase onto master. All necessary dependencies for the series are upstreamed meanwhile.

Ping

>
>Thanks,
>Bernhard
>>
>>Thanks.
>>
>>> > Testing done:
>>> > None, because I don't know how to conduct this properly :(
>>> > 
>>> > Based-on: <20221221170003.2929-1-shentey@gmail.com>
>>> >           "[PATCH v4 00/30] Consolidate PIIX south bridges"
>>

Re: [PATCH v2 0/6] Resolve TYPE_PIIX3_XEN_DEVICE

[Bernhard Beschow]

Am 1. Februar 2023 08:11:10 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>
>
>Am 24. Januar 2023 17:07:30 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>>
>>
>>Am 24. Januar 2023 16:11:47 UTC schrieb Anthony PERARD <anthony.perard@citrix.com>:
>>>On Wed, Jan 18, 2023 at 05:13:03AM -0500, Michael S. Tsirkin wrote:
>>>> On Wed, Jan 04, 2023 at 03:44:31PM +0100, Bernhard Beschow wrote:
>>>> > This series first renders TYPE_PIIX3_XEN_DEVICE redundant and finally removes
>>>> > it. The motivation is to 1/ decouple PIIX from Xen and 2/ to make Xen in the PC
>>>> > machine agnostic to the precise southbridge being used. 2/ will become
>>>> > particularily interesting once PIIX4 becomes usable in the PC machine, avoiding
>>>> > the "Frankenstein" use of PIIX4_ACPI in PIIX3.
>>>> 
>>>> Looks ok to me.
>>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>>>> 
>>>> Feel free to merge through Xen tree.
>>>
>>>Hi Bernhard,
>>
>>Hi Anthony,
>>
>>>The series currently doesn't apply on master. And a quick try at
>>>applying the series it is based on also failed. Could you rebase it , or
>>>maybe you would prefer to wait until the other series "Consolidate
>>>PIIX..." is fully applied?
>>
>>Thanks for looking into it!
>>
>>You can get the compilable series from https://patchew.org/QEMU/20230104144437.27479-1-shentey@gmail.com/ . If it doesn't work for you let me know, then I can rebase onto master. All necessary dependencies for the series are upstreamed meanwhile.
>
>Ping

Ping^2

>
>>
>>Thanks,
>>Bernhard
>>>
>>>Thanks.
>>>
>>>> > Testing done:
>>>> > None, because I don't know how to conduct this properly :(
>>>> > 
>>>> > Based-on: <20221221170003.2929-1-shentey@gmail.com>
>>>> >           "[PATCH v4 00/30] Consolidate PIIX south bridges"
>>>

Re: [PATCH v2 0/6] Resolve TYPE_PIIX3_XEN_DEVICE

[Chuck Zmudzinski]

On 2/9/2023 4:53 PM, Bernhard Beschow wrote:
> Am 1. Februar 2023 08:11:10 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
> >
> >
> >Am 24. Januar 2023 17:07:30 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
> >>
> >>
> >>Am 24. Januar 2023 16:11:47 UTC schrieb Anthony PERARD <anthony.perard@citrix.com>:
> >>>On Wed, Jan 18, 2023 at 05:13:03AM -0500, Michael S. Tsirkin wrote:
> >>>> On Wed, Jan 04, 2023 at 03:44:31PM +0100, Bernhard Beschow wrote:
> >>>> > This series first renders TYPE_PIIX3_XEN_DEVICE redundant and finally removes
> >>>> > it. The motivation is to 1/ decouple PIIX from Xen and 2/ to make Xen in the PC
> >>>> > machine agnostic to the precise southbridge being used. 2/ will become
> >>>> > particularily interesting once PIIX4 becomes usable in the PC machine, avoiding
> >>>> > the "Frankenstein" use of PIIX4_ACPI in PIIX3.
> >>>> 
> >>>> Looks ok to me.
> >>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> >>>> 
> >>>> Feel free to merge through Xen tree.
> >>>
> >>>Hi Bernhard,
> >>
> >>Hi Anthony,
> >>
> >>>The series currently doesn't apply on master. And a quick try at
> >>>applying the series it is based on also failed. Could you rebase it , or
> >>>maybe you would prefer to wait until the other series "Consolidate
> >>>PIIX..." is fully applied?
> >>
> >>Thanks for looking into it!
> >>
> >>You can get the compilable series from https://patchew.org/QEMU/20230104144437.27479-1-shentey@gmail.com/ . If it doesn't work for you let me know, then I can rebase onto master. All necessary dependencies for the series are upstreamed meanwhile.
> >
> >Ping
>
> Ping^2

Hi Bernhard,

I took a look at this today to see why it cannot be applied. I can see clearly that
all the prerequisite patches have *not* been applied to master yet, so I can
understand why Anthony cannot pull this up yet. Specifically, the series that
consolidates PIIX3 and PIIX4 south bridges is not yet applied, and that is one of
the prerequisites. I think you said it was reviewed, but it apparently never got
pulled up into master.

For reference, here is the link to the prerequisite patch set I tested with
this patch set:

https://lore.kernel.org/qemu-devel/20221221170003.2929-1-shentey@gmail.com/

The patch set I tested is a 30-patch series, and I don't know if it has
been partially applied. The title of that patch set is:

This series consolidates the implementations of the PIIX3 and PIIX4 south

So before this patch set to resolve the TYPE_PIIX3_XEN_DEVICE can be
applied, the patch set to consolidate the PIIX3 and PIIX4 south bridges
needs to be applied.

I don't know if the feature freeze means these patches that do not add any
new features still need to wait until the next development cycle.

Kind regards,

Chuck

> >
> >>
> >>Thanks,
> >>Bernhard
> >>>
> >>>Thanks.
> >>>
> >>>> > Testing done:
> >>>> > None, because I don't know how to conduct this properly :(
> >>>> > 
> >>>> > Based-on: <20221221170003.2929-1-shentey@gmail.com>
> >>>> >           "[PATCH v4 00/30] Consolidate PIIX south bridges"
> >>>

Re: [PATCH v2 0/6] Resolve TYPE_PIIX3_XEN_DEVICE

[Bernhard Beschow]

Am 11. März 2023 22:20:29 UTC schrieb Chuck Zmudzinski <brchuckz@aol.com>:
>On 2/9/2023 4:53 PM, Bernhard Beschow wrote:
>> Am 1. Februar 2023 08:11:10 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>> >
>> >
>> >Am 24. Januar 2023 17:07:30 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>> >>
>> >>
>> >>Am 24. Januar 2023 16:11:47 UTC schrieb Anthony PERARD <anthony.perard@citrix.com>:
>> >>>On Wed, Jan 18, 2023 at 05:13:03AM -0500, Michael S. Tsirkin wrote:
>> >>>> On Wed, Jan 04, 2023 at 03:44:31PM +0100, Bernhard Beschow wrote:
>> >>>> > This series first renders TYPE_PIIX3_XEN_DEVICE redundant and finally removes
>> >>>> > it. The motivation is to 1/ decouple PIIX from Xen and 2/ to make Xen in the PC
>> >>>> > machine agnostic to the precise southbridge being used. 2/ will become
>> >>>> > particularily interesting once PIIX4 becomes usable in the PC machine, avoiding
>> >>>> > the "Frankenstein" use of PIIX4_ACPI in PIIX3.
>> >>>> 
>> >>>> Looks ok to me.
>> >>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>> >>>> 
>> >>>> Feel free to merge through Xen tree.
>> >>>
>> >>>Hi Bernhard,
>> >>
>> >>Hi Anthony,
>> >>
>> >>>The series currently doesn't apply on master. And a quick try at
>> >>>applying the series it is based on also failed. Could you rebase it , or
>> >>>maybe you would prefer to wait until the other series "Consolidate
>> >>>PIIX..." is fully applied?
>> >>
>> >>Thanks for looking into it!
>> >>
>> >>You can get the compilable series from https://patchew.org/QEMU/20230104144437.27479-1-shentey@gmail.com/ . If it doesn't work for you let me know, then I can rebase onto master. All necessary dependencies for the series are upstreamed meanwhile.
>> >
>> >Ping
>>
>> Ping^2
>
>Hi Bernhard,

Hi Chuck,

>I took a look at this today to see why it cannot be applied.

Thanks for looking at it!

>I can see clearly that
>all the prerequisite patches have *not* been applied to master yet, so I can
>understand why Anthony cannot pull this up yet. Specifically, the series that
>consolidates PIIX3 and PIIX4 south bridges is not yet applied, and that is one of
>the prerequisites. I think you said it was reviewed, but it apparently never got
>pulled up into master.

Correct, the PIIX consolidation series isn't merged yet. This series currently depends on it to avoid merge conflicts but doesn't need it otherwise. Back then I anticipated that the consolidation series would land in master soon since it was fully reviewed before this one. But that turned out not to be the case.

This series depends on some necessary refactoring [1] which I did in the context of PIIX consolidation which is already in master. So this series can easily be rebased onto master and it even simplifies the consolidation series a bit. I'll take this route now and I'll post a v3.

Best regards,
Bernhard

[1] https://lore.kernel.org/qemu-devel/20221120150550.63059-1-shentey@gmail.com/

>
>For reference, here is the link to the prerequisite patch set I tested with
>this patch set:
>
>https://lore.kernel.org/qemu-devel/20221221170003.2929-1-shentey@gmail.com/
>
>The patch set I tested is a 30-patch series, and I don't know if it has
>been partially applied. The title of that patch set is:
>
>This series consolidates the implementations of the PIIX3 and PIIX4 south
>
>So before this patch set to resolve the TYPE_PIIX3_XEN_DEVICE can be
>applied, the patch set to consolidate the PIIX3 and PIIX4 south bridges
>needs to be applied.
>
>I don't know if the feature freeze means these patches that do not add any
>new features still need to wait until the next development cycle.
>
>Kind regards,
>
>Chuck
>
>> >
>> >>
>> >>Thanks,
>> >>Bernhard
>> >>>
>> >>>Thanks.
>> >>>
>> >>>> > Testing done:
>> >>>> > None, because I don't know how to conduct this properly :(
>> >>>> > 
>> >>>> > Based-on: <20221221170003.2929-1-shentey@gmail.com>
>> >>>> >           "[PATCH v4 00/30] Consolidate PIIX south bridges"
>> >>>
>

Re: [PATCH v2 0/6] Resolve TYPE_PIIX3_XEN_DEVICE

[Chuck Zmudzinski]

On 3/12/23 5:22 AM, Bernhard Beschow wrote:
> 
> 
> Am 11. März 2023 22:20:29 UTC schrieb Chuck Zmudzinski <brchuckz@aol.com>:
>>On 2/9/2023 4:53 PM, Bernhard Beschow wrote:
>>> Am 1. Februar 2023 08:11:10 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>>> >
>>> >
>>> >Am 24. Januar 2023 17:07:30 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>>> >>
>>> >>
>>> >>Am 24. Januar 2023 16:11:47 UTC schrieb Anthony PERARD <anthony.perard@citrix.com>:
>>> >>>On Wed, Jan 18, 2023 at 05:13:03AM -0500, Michael S. Tsirkin wrote:
>>> >>>> On Wed, Jan 04, 2023 at 03:44:31PM +0100, Bernhard Beschow wrote:
>>> >>>> > This series first renders TYPE_PIIX3_XEN_DEVICE redundant and finally removes
>>> >>>> > it. The motivation is to 1/ decouple PIIX from Xen and 2/ to make Xen in the PC
>>> >>>> > machine agnostic to the precise southbridge being used. 2/ will become
>>> >>>> > particularily interesting once PIIX4 becomes usable in the PC machine, avoiding
>>> >>>> > the "Frankenstein" use of PIIX4_ACPI in PIIX3.
>>> >>>> 
>>> >>>> Looks ok to me.
>>> >>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>>> >>>> 
>>> >>>> Feel free to merge through Xen tree.
>>> >>>
>>> >>>Hi Bernhard,
>>> >>
>>> >>Hi Anthony,
>>> >>
>>> >>>The series currently doesn't apply on master. And a quick try at
>>> >>>applying the series it is based on also failed. Could you rebase it , or
>>> >>>maybe you would prefer to wait until the other series "Consolidate
>>> >>>PIIX..." is fully applied?
>>> >>
>>> >>Thanks for looking into it!
>>> >>
>>> >>You can get the compilable series from https://patchew.org/QEMU/20230104144437.27479-1-shentey@gmail.com/ . If it doesn't work for you let me know, then I can rebase onto master. All necessary dependencies for the series are upstreamed meanwhile.
>>> >
>>> >Ping
>>>
>>> Ping^2
>>
>>Hi Bernhard,
> 
> Hi Chuck,
> 
>>I took a look at this today to see why it cannot be applied.
> 
> Thanks for looking at it!
> 
>>I can see clearly that
>>all the prerequisite patches have *not* been applied to master yet, so I can
>>understand why Anthony cannot pull this up yet. Specifically, the series that
>>consolidates PIIX3 and PIIX4 south bridges is not yet applied, and that is one of
>>the prerequisites. I think you said it was reviewed, but it apparently never got
>>pulled up into master.
> 
> Correct, the PIIX consolidation series isn't merged yet. This series currently depends on it to avoid merge conflicts but doesn't need it otherwise. Back then I anticipated that the consolidation series would land in master soon since it was fully reviewed before this one. But that turned out not to be the case.
> 
> This series depends on some necessary refactoring [1] which I did in the context of PIIX consolidation which is already in master. So this series can easily be rebased onto master and it even simplifies the consolidation series a bit. I'll take this route now and I'll post a v3.

Thanks for posting v3, I was at a loss trying to figure out how to merge the 30-patch piix
consolidation series into the master branch. I just tested your recent v3 (all 6 patches)
on top of the current master branch and it works well on my Xen guests, so you can keep my
Tested-by tag on that patch series!

Kind regards,

Chuck

Re: [PATCH v2 0/6] Resolve TYPE_PIIX3_XEN_DEVICE

[Bernhard Beschow]

Am 12. März 2023 21:02:03 UTC schrieb Chuck Zmudzinski <brchuckz@aol.com>:
>On 3/12/23 5:22 AM, Bernhard Beschow wrote:
>> 
>> 
>> Am 11. März 2023 22:20:29 UTC schrieb Chuck Zmudzinski <brchuckz@aol.com>:
>>>On 2/9/2023 4:53 PM, Bernhard Beschow wrote:
>>>> Am 1. Februar 2023 08:11:10 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>>>> >
>>>> >
>>>> >Am 24. Januar 2023 17:07:30 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>>>> >>
>>>> >>
>>>> >>Am 24. Januar 2023 16:11:47 UTC schrieb Anthony PERARD <anthony.perard@citrix.com>:
>>>> >>>On Wed, Jan 18, 2023 at 05:13:03AM -0500, Michael S. Tsirkin wrote:
>>>> >>>> On Wed, Jan 04, 2023 at 03:44:31PM +0100, Bernhard Beschow wrote:
>>>> >>>> > This series first renders TYPE_PIIX3_XEN_DEVICE redundant and finally removes
>>>> >>>> > it. The motivation is to 1/ decouple PIIX from Xen and 2/ to make Xen in the PC
>>>> >>>> > machine agnostic to the precise southbridge being used. 2/ will become
>>>> >>>> > particularily interesting once PIIX4 becomes usable in the PC machine, avoiding
>>>> >>>> > the "Frankenstein" use of PIIX4_ACPI in PIIX3.
>>>> >>>> 
>>>> >>>> Looks ok to me.
>>>> >>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>>>> >>>> 
>>>> >>>> Feel free to merge through Xen tree.
>>>> >>>
>>>> >>>Hi Bernhard,
>>>> >>
>>>> >>Hi Anthony,
>>>> >>
>>>> >>>The series currently doesn't apply on master. And a quick try at
>>>> >>>applying the series it is based on also failed. Could you rebase it , or
>>>> >>>maybe you would prefer to wait until the other series "Consolidate
>>>> >>>PIIX..." is fully applied?
>>>> >>
>>>> >>Thanks for looking into it!
>>>> >>
>>>> >>You can get the compilable series from https://patchew.org/QEMU/20230104144437.27479-1-shentey@gmail.com/ . If it doesn't work for you let me know, then I can rebase onto master. All necessary dependencies for the series are upstreamed meanwhile.
>>>> >
>>>> >Ping
>>>>
>>>> Ping^2
>>>
>>>Hi Bernhard,
>> 
>> Hi Chuck,
>> 
>>>I took a look at this today to see why it cannot be applied.
>> 
>> Thanks for looking at it!
>> 
>>>I can see clearly that
>>>all the prerequisite patches have *not* been applied to master yet, so I can
>>>understand why Anthony cannot pull this up yet. Specifically, the series that
>>>consolidates PIIX3 and PIIX4 south bridges is not yet applied, and that is one of
>>>the prerequisites. I think you said it was reviewed, but it apparently never got
>>>pulled up into master.
>> 
>> Correct, the PIIX consolidation series isn't merged yet. This series currently depends on it to avoid merge conflicts but doesn't need it otherwise. Back then I anticipated that the consolidation series would land in master soon since it was fully reviewed before this one. But that turned out not to be the case.
>> 
>> This series depends on some necessary refactoring [1] which I did in the context of PIIX consolidation which is already in master. So this series can easily be rebased onto master and it even simplifies the consolidation series a bit. I'll take this route now and I'll post a v3.
>
>Thanks for posting v3, I was at a loss trying to figure out how to merge the 30-patch piix
>consolidation series into the master branch.

Yeah, the code suffered from bit rod...

>I just tested your recent v3 (all 6 patches)
>on top of the current master branch and it works well on my Xen guests, so you can keep my
>Tested-by tag on that patch series!

Thanks!

Best regards,
Bernhard

>
>Kind regards,
>
>Chuck