[PATCH v3 0/6] Resolve TYPE_PIIX3_XEN_DEVICE

[PATCH v3 0/6] Resolve TYPE_PIIX3_XEN_DEVICE

[Bernhard Beschow]

There is currently a dedicated PIIX3 device model for use under Xen. By reusing
existing PCI API during initialization this device model can be eliminated and
the plain PIIX3 device model can be used instead.

Resolving TYPE_PIIX3_XEN_DEVICE results in less code while also making Xen
agnostic towards the precise south bridge being used in the PC machine. The
latter might become particularily interesting once PIIX4 becomes usable in the
PC machine, avoiding the "Frankenstein" use of PIIX4_ACPI in PIIX3.

Testing done:
- `make check`
- `make check-avocado`
- Run `xl create` with the following config:
    name = "Manjaro"
    type = 'hvm'
    memory = 1536
    apic = 1
    usb = 1
    disk = [ "file:manjaro-kde-21.2.6-220416-linux515.iso,hdc:cdrom,r" ]
    device_model_override = "/usr/bin/qemu-system-x86_64"
    vga = "stdvga"
    sdl = 1
- `qemu-system-x86_64 -M pc -m 2G -cpu host \
    -accel kvm,xen-version=0x4000a,kernel-irqchip=split \
    -cdrom manjaro-kde-21.2.6-220416-linux515.iso`
- `qemu-system-x86_64 -M pc -m 2G -cpu host -accel kvm \
    -cdrom manjaro-kde-21.2.6-220416-linux515.iso`

v3:
- Rebase onto master

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

Tested-by: Chuck Zmudzinski <brchuckz@aol.com>

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

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

-- 
2.39.2

[PATCH v3 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>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/xen/xen.h  | 2 +-
 hw/i386/xen/xen-hvm.c | 2 +-
 hw/isa/piix3.c        | 4 ++--
 stubs/xen-hw-stub.c   | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
index 2bd8ec742d..37ecc91fc3 100644
--- a/include/hw/xen/xen.h
+++ b/include/hw/xen/xen.h
@@ -39,7 +39,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/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index 56641a550e..ab8f1b61ee 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -143,7 +143,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/piix3.c b/hw/isa/piix3.c
index a9cb39bf21..1b3e23f0d7 100644
--- a/hw/isa/piix3.c
+++ b/hw/isa/piix3.c
@@ -34,7 +34,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 piix3_set_irq_pic(PIIX3State *piix3, int pic_irq)
 {
@@ -405,7 +405,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/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.2

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

[Anthony PERARD via]

On Sun, Mar 12, 2023 at 01:02:16PM +0100, 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>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD

[PATCH v3 2/6] hw/isa/piix3: 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>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/isa/piix3.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
index 1b3e23f0d7..a86cd23ef4 100644
--- a/hw/isa/piix3.c
+++ b/hw/isa/piix3.c
@@ -394,7 +394,7 @@ static void piix3_xen_realize(PCIDevice *dev, Error **errp)
     PIIX3State *piix3 = PIIX3_PCI_DEVICE(dev);
     PCIBus *pci_bus = pci_get_bus(dev);
 
-    pci_piix3_realize(dev, errp);
+    piix3_realize(dev, errp);
     if (*errp) {
         return;
     }
-- 
2.39.2

Re: [PATCH v3 2/6] hw/isa/piix3: Reuse piix3_realize() in piix3_xen_realize()

[Anthony PERARD via]

On Sun, Mar 12, 2023 at 01:02:17PM +0100, Bernhard Beschow wrote:
> 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.

pci_bus_irqs() does allocates pci_bus->irq_count, so the second call in
piix3_xen_realize() will leak `pci_bus->irq_count`. Could you look if
pci_bus_irqs_cleanup() can be called before the second pci_bus_irqs()
call, or maybe some other way to avoid the leak?

> Second, pci_bus_set_route_irq_fn() is now also called in Xen mode.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

Beside the leak which I think can happen only once, patch is fine:
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD

Re: [PATCH v3 2/6] hw/isa/piix3: Reuse piix3_realize() in piix3_xen_realize()

[Bernhard Beschow]

Am 30. März 2023 13:00:25 UTC schrieb Anthony PERARD <anthony.perard@citrix.com>:
>On Sun, Mar 12, 2023 at 01:02:17PM +0100, Bernhard Beschow wrote:
>> 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.
>
>pci_bus_irqs() does allocates pci_bus->irq_count, so the second call in
>piix3_xen_realize() will leak `pci_bus->irq_count`. Could you look if
>pci_bus_irqs_cleanup() can be called before the second pci_bus_irqs()
>call, or maybe some other way to avoid the leak?

Thanks for catching this! I'll post a v4.

I think the most fool-proof way to fix this is to free irq_count just before the assignment. pci_bus_irqs_cleanup() would then have to NULL the attribute such that pci_bus_irqs() can be called afterwards.

BTW: I tried running qemu-system-x86_64 with PIIX4 rather than PIIX3 as Xen guest with my pc-piix4 branch without success. This branch essentially just provides slightly different PCI IDs for PIIX. Does xl or something else in Xen check these? If not then this means I'm still missing something. Under KVM this branch works just fine. Any idea?

Thanks,
Bernhard

>
>> Second, pci_bus_set_route_irq_fn() is now also called in Xen mode.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>
>Beside the leak which I think can happen only once, patch is fine:
>Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
>
>Thanks,
>

Re: [PATCH v3 2/6] hw/isa/piix3: Reuse piix3_realize() in piix3_xen_realize()

[Bernhard Beschow]

Am 1. April 2023 22:36:45 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>
>
>Am 30. März 2023 13:00:25 UTC schrieb Anthony PERARD <anthony.perard@citrix.com>:
>>On Sun, Mar 12, 2023 at 01:02:17PM +0100, Bernhard Beschow wrote:
>>> 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.
>>
>>pci_bus_irqs() does allocates pci_bus->irq_count, so the second call in
>>piix3_xen_realize() will leak `pci_bus->irq_count`. Could you look if
>>pci_bus_irqs_cleanup() can be called before the second pci_bus_irqs()
>>call, or maybe some other way to avoid the leak?
>
>Thanks for catching this! I'll post a v4.

V4 is out.

Thanks,
Bernhard

>
>I think the most fool-proof way to fix this is to free irq_count just before the assignment. pci_bus_irqs_cleanup() would then have to NULL the attribute such that pci_bus_irqs() can be called afterwards.
>
>BTW: I tried running qemu-system-x86_64 with PIIX4 rather than PIIX3 as Xen guest with my pc-piix4 branch without success. This branch essentially just provides slightly different PCI IDs for PIIX. Does xl or something else in Xen check these? If not then this means I'm still missing something. Under KVM this branch works just fine. Any idea?
>
>Thanks,
>Bernhard
>
>>
>>> Second, pci_bus_set_route_irq_fn() is now also called in Xen mode.
>>> 
>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>>
>>Beside the leak which I think can happen only once, patch is fine:
>>Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
>>
>>Thanks,
>>

Re: [PATCH v3 2/6] hw/isa/piix3: Reuse piix3_realize() in piix3_xen_realize()

[Anthony PERARD via]

On Sat, Apr 01, 2023 at 10:36:45PM +0000, Bernhard Beschow wrote:
> 
> 
> Am 30. März 2023 13:00:25 UTC schrieb Anthony PERARD <anthony.perard@citrix.com>:
> >On Sun, Mar 12, 2023 at 01:02:17PM +0100, Bernhard Beschow wrote:
> >> 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.
> >
> >pci_bus_irqs() does allocates pci_bus->irq_count, so the second call in
> >piix3_xen_realize() will leak `pci_bus->irq_count`. Could you look if
> >pci_bus_irqs_cleanup() can be called before the second pci_bus_irqs()
> >call, or maybe some other way to avoid the leak?
> 
> Thanks for catching this! I'll post a v4.
> 
> I think the most fool-proof way to fix this is to free irq_count just before the assignment. pci_bus_irqs_cleanup() would then have to NULL the attribute such that pci_bus_irqs() can be called afterwards.
> 
> BTW: I tried running qemu-system-x86_64 with PIIX4 rather than PIIX3 as Xen guest with my pc-piix4 branch without success. This branch essentially just provides slightly different PCI IDs for PIIX. Does xl or something else in Xen check these? If not then this means I'm still missing something. Under KVM this branch works just fine. Any idea?

Maybe the ACPI tables provided by libxl needs to be updated.
Or maybe something in the firmware (SeaBIOS or OVMF/OvmfXen) check the
id (I know that the PCI id of the root bus is checked, but I don't know
if that's the one that's been changed).

Thanks,

-- 
Anthony PERARD

Re: [PATCH v3 2/6] hw/isa/piix3: Reuse piix3_realize() in piix3_xen_realize()

[Jason Andryuk]

On Mon, Apr 3, 2023 at 5:33 AM Anthony PERARD <anthony.perard@citrix.com> wrote:
>
> On Sat, Apr 01, 2023 at 10:36:45PM +0000, Bernhard Beschow wrote:
> >
> >
> > Am 30. März 2023 13:00:25 UTC schrieb Anthony PERARD <anthony.perard@citrix.com>:
> > >On Sun, Mar 12, 2023 at 01:02:17PM +0100, Bernhard Beschow wrote:
> > >> 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.
> > >
> > >pci_bus_irqs() does allocates pci_bus->irq_count, so the second call in
> > >piix3_xen_realize() will leak `pci_bus->irq_count`. Could you look if
> > >pci_bus_irqs_cleanup() can be called before the second pci_bus_irqs()
> > >call, or maybe some other way to avoid the leak?
> >
> > Thanks for catching this! I'll post a v4.
> >
> > I think the most fool-proof way to fix this is to free irq_count just before the assignment. pci_bus_irqs_cleanup() would then have to NULL the attribute such that pci_bus_irqs() can be called afterwards.
> >
> > BTW: I tried running qemu-system-x86_64 with PIIX4 rather than PIIX3 as Xen guest with my pc-piix4 branch without success. This branch essentially just provides slightly different PCI IDs for PIIX. Does xl or something else in Xen check these? If not then this means I'm still missing something. Under KVM this branch works just fine. Any idea?
>
> Maybe the ACPI tables provided by libxl needs to be updated.
> Or maybe something in the firmware (SeaBIOS or OVMF/OvmfXen) check the
> id (I know that the PCI id of the root bus is checked, but I don't know
> if that's the one that's been changed).

Xen also has hvmloader, which runs before SeaBIOS/OVMF.  Looking at
tools/firmware/hvmloader/pci.c, it has
        ASSERT((devfn != PCI_ISA_DEVFN) ||
               ((vendor_id == 0x8086) && (device_id == 0x7000)));

From QEMU, it looks like 0x7000 is PCI_DEVICE_ID_INTEL_82371SB_0, but
PIIX4 uses 0x7110 (PCI_DEVICE_ID_INTEL_82371AB_0).  Maybe try removing
that check?

Regards,
Jason

Re: [PATCH v3 2/6] hw/isa/piix3: Reuse piix3_realize() in piix3_xen_realize()

[Bernhard Beschow]

Am 3. April 2023 12:27:14 UTC schrieb Jason Andryuk <jandryuk@gmail.com>:
>On Mon, Apr 3, 2023 at 5:33 AM Anthony PERARD <anthony.perard@citrix.com> wrote:
>>
>> On Sat, Apr 01, 2023 at 10:36:45PM +0000, Bernhard Beschow wrote:
>> >
>> >
>> > Am 30. März 2023 13:00:25 UTC schrieb Anthony PERARD <anthony.perard@citrix.com>:
>> > >On Sun, Mar 12, 2023 at 01:02:17PM +0100, Bernhard Beschow wrote:
>> > >> 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.
>> > >
>> > >pci_bus_irqs() does allocates pci_bus->irq_count, so the second call in
>> > >piix3_xen_realize() will leak `pci_bus->irq_count`. Could you look if
>> > >pci_bus_irqs_cleanup() can be called before the second pci_bus_irqs()
>> > >call, or maybe some other way to avoid the leak?
>> >
>> > Thanks for catching this! I'll post a v4.
>> >
>> > I think the most fool-proof way to fix this is to free irq_count just before the assignment. pci_bus_irqs_cleanup() would then have to NULL the attribute such that pci_bus_irqs() can be called afterwards.
>> >
>> > BTW: I tried running qemu-system-x86_64 with PIIX4 rather than PIIX3 as Xen guest with my pc-piix4 branch without success. This branch essentially just provides slightly different PCI IDs for PIIX. Does xl or something else in Xen check these? If not then this means I'm still missing something. Under KVM this branch works just fine. Any idea?
>>
>> Maybe the ACPI tables provided by libxl needs to be updated.
>> Or maybe something in the firmware (SeaBIOS or OVMF/OvmfXen) check the
>> id (I know that the PCI id of the root bus is checked, but I don't know
>> if that's the one that's been changed).
>
>Xen also has hvmloader, which runs before SeaBIOS/OVMF.  Looking at
>tools/firmware/hvmloader/pci.c, it has
>        ASSERT((devfn != PCI_ISA_DEVFN) ||
>               ((vendor_id == 0x8086) && (device_id == 0x7000)));
>
>From QEMU, it looks like 0x7000 is PCI_DEVICE_ID_INTEL_82371SB_0, but
>PIIX4 uses 0x7110 (PCI_DEVICE_ID_INTEL_82371AB_0).  Maybe try removing
>that check?

Sounds promising indeed. I'll give it a try!

Regards,
Bernhard

>
>Regards,
>Jason

Re: [PATCH v3 2/6] hw/isa/piix3: Reuse piix3_realize() in piix3_xen_realize()

[Bernhard Beschow]

Am 3. April 2023 12:27:14 UTC schrieb Jason Andryuk <jandryuk@gmail.com>:
>On Mon, Apr 3, 2023 at 5:33 AM Anthony PERARD <anthony.perard@citrix.com> wrote:
>>
>> On Sat, Apr 01, 2023 at 10:36:45PM +0000, Bernhard Beschow wrote:
>> >
>> >
>> > Am 30. März 2023 13:00:25 UTC schrieb Anthony PERARD <anthony.perard@citrix.com>:
>> > >On Sun, Mar 12, 2023 at 01:02:17PM +0100, Bernhard Beschow wrote:
>> > >> 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.
>> > >
>> > >pci_bus_irqs() does allocates pci_bus->irq_count, so the second call in
>> > >piix3_xen_realize() will leak `pci_bus->irq_count`. Could you look if
>> > >pci_bus_irqs_cleanup() can be called before the second pci_bus_irqs()
>> > >call, or maybe some other way to avoid the leak?
>> >
>> > Thanks for catching this! I'll post a v4.
>> >
>> > I think the most fool-proof way to fix this is to free irq_count just before the assignment. pci_bus_irqs_cleanup() would then have to NULL the attribute such that pci_bus_irqs() can be called afterwards.
>> >
>> > BTW: I tried running qemu-system-x86_64 with PIIX4 rather than PIIX3 as Xen guest with my pc-piix4 branch without success. This branch essentially just provides slightly different PCI IDs for PIIX. Does xl or something else in Xen check these? If not then this means I'm still missing something. Under KVM this branch works just fine. Any idea?
>>
>> Maybe the ACPI tables provided by libxl needs to be updated.
>> Or maybe something in the firmware (SeaBIOS or OVMF/OvmfXen) check the
>> id (I know that the PCI id of the root bus is checked, but I don't know
>> if that's the one that's been changed).
>
>Xen also has hvmloader, which runs before SeaBIOS/OVMF.  Looking at
>tools/firmware/hvmloader/pci.c, it has
>        ASSERT((devfn != PCI_ISA_DEVFN) ||
>               ((vendor_id == 0x8086) && (device_id == 0x7000)));
>
>From QEMU, it looks like 0x7000 is PCI_DEVICE_ID_INTEL_82371SB_0, but
>PIIX4 uses 0x7110 (PCI_DEVICE_ID_INTEL_82371AB_0).  Maybe try removing
>that check?

I was finally able to build Xen successfully (without my distribution providing too recent dependencies that prevent compilation). With 0x7110 added in the line above I could indeed run a Xen guest with PIIX4. Yay!

Now I just need to respin my PIIX consolidation series...

Best regards,
Bernhard

>
>Regards,
>Jason

Re: [PATCH v3 2/6] hw/isa/piix3: Reuse piix3_realize() in piix3_xen_realize()

[Chuck Zmudzinski]

On 9/19/2023 4:02 PM, Bernhard Beschow wrote:
> 
> 
> Am 3. April 2023 12:27:14 UTC schrieb Jason Andryuk <jandryuk@gmail.com>:
>>On Mon, Apr 3, 2023 at 5:33 AM Anthony PERARD <anthony.perard@citrix.com> wrote:
>>>
>>> On Sat, Apr 01, 2023 at 10:36:45PM +0000, Bernhard Beschow wrote:
>>> >
>>> >
>>> > Am 30. März 2023 13:00:25 UTC schrieb Anthony PERARD <anthony.perard@citrix.com>:
>>> > >On Sun, Mar 12, 2023 at 01:02:17PM +0100, Bernhard Beschow wrote:
>>> > >> 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.
>>> > >
>>> > >pci_bus_irqs() does allocates pci_bus->irq_count, so the second call in
>>> > >piix3_xen_realize() will leak `pci_bus->irq_count`. Could you look if
>>> > >pci_bus_irqs_cleanup() can be called before the second pci_bus_irqs()
>>> > >call, or maybe some other way to avoid the leak?
>>> >
>>> > Thanks for catching this! I'll post a v4.
>>> >
>>> > I think the most fool-proof way to fix this is to free irq_count just before the assignment. pci_bus_irqs_cleanup() would then have to NULL the attribute such that pci_bus_irqs() can be called afterwards.
>>> >
>>> > BTW: I tried running qemu-system-x86_64 with PIIX4 rather than PIIX3 as Xen guest with my pc-piix4 branch without success. This branch essentially just provides slightly different PCI IDs for PIIX. Does xl or something else in Xen check these? If not then this means I'm still missing something. Under KVM this branch works just fine. Any idea?
>>>
>>> Maybe the ACPI tables provided by libxl needs to be updated.
>>> Or maybe something in the firmware (SeaBIOS or OVMF/OvmfXen) check the
>>> id (I know that the PCI id of the root bus is checked, but I don't know
>>> if that's the one that's been changed).
>>
>>Xen also has hvmloader, which runs before SeaBIOS/OVMF.  Looking at
>>tools/firmware/hvmloader/pci.c, it has
>>        ASSERT((devfn != PCI_ISA_DEVFN) ||
>>               ((vendor_id == 0x8086) && (device_id == 0x7000)));
>>
>>From QEMU, it looks like 0x7000 is PCI_DEVICE_ID_INTEL_82371SB_0, but
>>PIIX4 uses 0x7110 (PCI_DEVICE_ID_INTEL_82371AB_0).  Maybe try removing
>>that check?
> 
> I was finally able to build Xen successfully (without my distribution providing too recent dependencies that prevent compilation). With 0x7110 added in the line above I could indeed run a Xen guest with PIIX4. Yay!
> 
> Now I just need to respin my PIIX consolidation series...

Welcome to the world of running guests on Xen! I am the one who tested your earlier patches with Xen guests, and I just wanted to say thanks for keeping me in the loop. Please Cc me when you post your respin of the PIIX consolidation series since I would like to also test it in my Xen environment. I understand I will also need to patch hvmloader.c on the Xen side to set the correct device id.

Kind regards,

Chuck

> 
> Best regards,
> Bernhard
> 
>>
>>Regards,
>>Jason

Re: [PATCH v3 2/6] hw/isa/piix3: Reuse piix3_realize() in piix3_xen_realize()

[Bernhard Beschow]

Am 20. September 2023 14:44:23 UTC schrieb Chuck Zmudzinski <brchuckz@aol.com>:
>On 9/19/2023 4:02 PM, Bernhard Beschow wrote:
>> 
>> 
>> Am 3. April 2023 12:27:14 UTC schrieb Jason Andryuk <jandryuk@gmail.com>:
>>>On Mon, Apr 3, 2023 at 5:33 AM Anthony PERARD <anthony.perard@citrix.com> wrote:
>>>>
>>>> On Sat, Apr 01, 2023 at 10:36:45PM +0000, Bernhard Beschow wrote:
>>>> >
>>>> >
>>>> > Am 30. März 2023 13:00:25 UTC schrieb Anthony PERARD <anthony.perard@citrix.com>:
>>>> > >On Sun, Mar 12, 2023 at 01:02:17PM +0100, Bernhard Beschow wrote:
>>>> > >> 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.
>>>> > >
>>>> > >pci_bus_irqs() does allocates pci_bus->irq_count, so the second call in
>>>> > >piix3_xen_realize() will leak `pci_bus->irq_count`. Could you look if
>>>> > >pci_bus_irqs_cleanup() can be called before the second pci_bus_irqs()
>>>> > >call, or maybe some other way to avoid the leak?
>>>> >
>>>> > Thanks for catching this! I'll post a v4.
>>>> >
>>>> > I think the most fool-proof way to fix this is to free irq_count just before the assignment. pci_bus_irqs_cleanup() would then have to NULL the attribute such that pci_bus_irqs() can be called afterwards.
>>>> >
>>>> > BTW: I tried running qemu-system-x86_64 with PIIX4 rather than PIIX3 as Xen guest with my pc-piix4 branch without success. This branch essentially just provides slightly different PCI IDs for PIIX. Does xl or something else in Xen check these? If not then this means I'm still missing something. Under KVM this branch works just fine. Any idea?
>>>>
>>>> Maybe the ACPI tables provided by libxl needs to be updated.
>>>> Or maybe something in the firmware (SeaBIOS or OVMF/OvmfXen) check the
>>>> id (I know that the PCI id of the root bus is checked, but I don't know
>>>> if that's the one that's been changed).
>>>
>>>Xen also has hvmloader, which runs before SeaBIOS/OVMF.  Looking at
>>>tools/firmware/hvmloader/pci.c, it has
>>>        ASSERT((devfn != PCI_ISA_DEVFN) ||
>>>               ((vendor_id == 0x8086) && (device_id == 0x7000)));
>>>
>>>From QEMU, it looks like 0x7000 is PCI_DEVICE_ID_INTEL_82371SB_0, but
>>>PIIX4 uses 0x7110 (PCI_DEVICE_ID_INTEL_82371AB_0).  Maybe try removing
>>>that check?
>> 
>> I was finally able to build Xen successfully (without my distribution providing too recent dependencies that prevent compilation). With 0x7110 added in the line above I could indeed run a Xen guest with PIIX4. Yay!
>> 
>> Now I just need to respin my PIIX consolidation series...
>
>Welcome to the world of running guests on Xen! I am the one who tested your earlier patches with Xen guests,

Thanks, I remember for sure!

> and I just wanted to say thanks for keeping me in the loop. Please Cc me when you post your respin of the PIIX consolidation series since I would like to also test it in my Xen environment. I understand I will also need to patch hvmloader.c on the Xen side to set the correct device id.

I'd add your e-mail to the recipients list in my Git then.

For those who want a sneak preview of PIIX4 in the PC machine may compile https://github.com/shentok/qemu/tree/piix-consolidate and run `qemu-system-x86_64 -M pc,south-bridge=piix4-isa`. It should work with all available virtualization technologies.

Best regards,
Bernhard

>
>Kind regards,
>
>Chuck
>
>> 
>> Best regards,
>> Bernhard
>> 
>>>
>>>Regards,
>>>Jason
>

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

[Bernhard Beschow]

xen_intx_set_irq() doesn't depend on PIIX3State. 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>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/i386/pc_piix.c | 13 +++++++++++++
 hw/isa/piix3.c    | 24 +-----------------------
 2 files changed, 14 insertions(+), 23 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 30eedd62a3..99232701b1 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -69,6 +69,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 };
@@ -236,6 +237,18 @@ static void pc_init1(MachineState *machine,
         pcms->bus = pci_bus;
 
         pci_dev = pci_create_simple_multifunction(pci_bus, -1, true, type);
+
+        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);
+        }
+
         piix3 = PIIX3_PCI_DEVICE(pci_dev);
         piix3->pic = x86ms->gsi;
         piix3_devfn = piix3->dev.devfn;
diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
index a86cd23ef4..7a31caf2b6 100644
--- a/hw/isa/piix3.c
+++ b/hw/isa/piix3.c
@@ -34,8 +34,6 @@
 #include "migration/vmstate.h"
 #include "hw/acpi/acpi_aml_interface.h"
 
-#define XEN_IOAPIC_NUM_PIRQS    128ULL
-
 static void piix3_set_irq_pic(PIIX3State *piix3, int pic_irq)
 {
     qemu_set_irq(piix3->pic[pic_irq],
@@ -388,32 +386,12 @@ static const TypeInfo piix3_info = {
     .class_init    = piix3_class_init,
 };
 
-static void piix3_xen_realize(PCIDevice *dev, Error **errp)
-{
-    ERRP_GUARD();
-    PIIX3State *piix3 = PIIX3_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)
 {
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
     k->config_write = piix3_write_config_xen;
-    k->realize = piix3_xen_realize;
+    k->realize = piix3_realize;
 }
 
 static const TypeInfo piix3_xen_info = {
-- 
2.39.2

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

[Anthony PERARD via]

On Sun, Mar 12, 2023 at 01:02:18PM +0100, Bernhard Beschow wrote:
> xen_intx_set_irq() doesn't depend on PIIX3State. 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>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD

[PATCH v3 4/6] hw/isa/piix3: Avoid Xen-specific variant of piix3_write_config()

[Bernhard Beschow]

Subscribe to pci_bus_fire_intx_routing_notifier() instead which allows for
having a common piix3_write_config() for the PIIX3 device models.

While at it, move the subscription into machine code to facilitate resolving
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>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/i386/pc_piix.c | 18 ++++++++++++++++++
 hw/isa/piix3.c    | 22 +---------------------
 2 files changed, 19 insertions(+), 21 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 99232701b1..1b70470dcd 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -88,6 +88,21 @@ static int pc_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)
@@ -239,6 +254,9 @@ static void pc_init1(MachineState *machine,
         pci_dev = pci_create_simple_multifunction(pci_bus, -1, true, type);
 
         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/piix3.c b/hw/isa/piix3.c
index 7a31caf2b6..737f5c6a5d 100644
--- a/hw/isa/piix3.c
+++ b/hw/isa/piix3.c
@@ -121,26 +121,6 @@ static void piix3_write_config(PCIDevice *dev,
     }
 }
 
-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);
-        }
-    }
-
-    piix3_write_config(dev, address, val, len);
-}
-
 static void piix3_reset(DeviceState *dev)
 {
     PIIX3State *d = PIIX3_PCI_DEVICE(dev);
@@ -390,7 +370,7 @@ static void piix3_xen_class_init(ObjectClass *klass, void *data)
 {
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-    k->config_write = piix3_write_config_xen;
+    k->config_write = piix3_write_config;
     k->realize = piix3_realize;
 }
 
-- 
2.39.2

Re: [PATCH v3 4/6] hw/isa/piix3: Avoid Xen-specific variant of piix3_write_config()

[Anthony PERARD via]

On Sun, Mar 12, 2023 at 01:02:19PM +0100, Bernhard Beschow wrote:
> Subscribe to pci_bus_fire_intx_routing_notifier() instead which allows for
> having a common piix3_write_config() for the PIIX3 device models.
> 
> While at it, move the subscription into machine code to facilitate resolving
> 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>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD

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

[Bernhard Beschow]

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

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/isa/piix3.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
index 737f5c6a5d..418940139d 100644
--- a/hw/isa/piix3.c
+++ b/hw/isa/piix3.c
@@ -308,6 +308,7 @@ static void pci_piix3_class_init(ObjectClass *klass, void *data)
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
     AcpiDevAmlIfClass *adevc = ACPI_DEV_AML_IF_CLASS(klass);
 
+    k->config_write = piix3_write_config;
     dc->reset       = piix3_reset;
     dc->desc        = "ISA bridge";
     dc->vmsd        = &vmstate_piix3;
@@ -356,7 +357,6 @@ static void piix3_class_init(ObjectClass *klass, void *data)
 {
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-    k->config_write = piix3_write_config;
     k->realize = piix3_realize;
 }
 
@@ -370,7 +370,6 @@ static void piix3_xen_class_init(ObjectClass *klass, void *data)
 {
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-    k->config_write = piix3_write_config;
     k->realize = piix3_realize;
 }
 
-- 
2.39.2

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

[Anthony PERARD via]

On Sun, Mar 12, 2023 at 01:02:20PM +0100, Bernhard Beschow wrote:
> The previous patch unified handling of piix3_write_config() accross the
> PIIX3 device models which allows for assigning k->config_write once in the
> base class.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD

[PATCH v3 6/6] hw/isa/piix3: 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>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/southbridge/piix.h |  1 -
 hw/i386/pc_piix.c             |  5 ++---
 hw/isa/piix3.c                | 15 ---------------
 3 files changed, 2 insertions(+), 19 deletions(-)

diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
index 0bf48e936d..51be04e984 100644
--- a/include/hw/southbridge/piix.h
+++ b/include/hw/southbridge/piix.h
@@ -64,7 +64,6 @@ DECLARE_INSTANCE_CHECKER(PIIX3State, PIIX3_PCI_DEVICE,
                          TYPE_PIIX3_PCI_DEVICE)
 
 #define TYPE_PIIX3_DEVICE "PIIX3"
-#define TYPE_PIIX3_XEN_DEVICE "PIIX3-xen"
 #define TYPE_PIIX4_PCI_DEVICE "piix4-isa"
 
 #endif
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 1b70470dcd..7ca0d6d14e 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -237,8 +237,6 @@ static void pc_init1(MachineState *machine,
     if (pcmc->pci_enabled) {
         PIIX3State *piix3;
         PCIDevice *pci_dev;
-        const char *type = xen_enabled() ? TYPE_PIIX3_XEN_DEVICE
-                                         : TYPE_PIIX3_DEVICE;
 
         pci_bus = i440fx_init(pci_type,
                               i440fx_host,
@@ -251,7 +249,8 @@ static void pc_init1(MachineState *machine,
                                        : pc_pci_slot_get_pirq);
         pcms->bus = pci_bus;
 
-        pci_dev = pci_create_simple_multifunction(pci_bus, -1, true, type);
+        pci_dev = pci_create_simple_multifunction(pci_bus, -1, true,
+                                                  TYPE_PIIX3_DEVICE);
 
         if (xen_enabled()) {
             pci_device_set_intx_routing_notifier(
diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
index 418940139d..0d6992af67 100644
--- a/hw/isa/piix3.c
+++ b/hw/isa/piix3.c
@@ -29,7 +29,6 @@
 #include "hw/southbridge/piix.h"
 #include "hw/irq.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"
@@ -366,24 +365,10 @@ static const TypeInfo piix3_info = {
     .class_init    = piix3_class_init,
 };
 
-static void piix3_xen_class_init(ObjectClass *klass, void *data)
-{
-    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
-
-    k->realize = piix3_realize;
-}
-
-static const TypeInfo piix3_xen_info = {
-    .name          = TYPE_PIIX3_XEN_DEVICE,
-    .parent        = TYPE_PIIX3_PCI_DEVICE,
-    .class_init    = piix3_xen_class_init,
-};
-
 static void piix3_register_types(void)
 {
     type_register_static(&piix3_pci_type_info);
     type_register_static(&piix3_info);
-    type_register_static(&piix3_xen_info);
 }
 
 type_init(piix3_register_types)
-- 
2.39.2

Re: [PATCH v3 6/6] hw/isa/piix3: Resolve redundant TYPE_PIIX3_XEN_DEVICE

[Anthony PERARD via]

On Sun, Mar 12, 2023 at 01:02:21PM +0100, 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>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD