diff mbox series

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

Message ID 20230312120221.99183-3-shentey@gmail.com (mailing list archive)
State Superseded
Headers show
Series Resolve TYPE_PIIX3_XEN_DEVICE | expand

Commit Message

Bernhard Beschow March 12, 2023, 12:02 p.m. UTC 
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(-)

Comments

Anthony PERARD March 30, 2023, 1 p.m. UTC | #1 
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,
Bernhard Beschow April 1, 2023, 10:36 p.m. UTC | #2 
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,
>
Bernhard Beschow April 3, 2023, 8:08 a.m. UTC | #3 
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,
>>
Anthony PERARD April 3, 2023, 9:32 a.m. UTC | #4 
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,
Jason Andryuk April 3, 2023, 12:27 p.m. UTC | #5 
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
Bernhard Beschow April 3, 2023, 8:36 p.m. UTC | #6 
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
Bernhard Beschow Sept. 19, 2023, 8:02 p.m. UTC | #7 
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
Chuck Zmudzinski Sept. 20, 2023, 2:44 p.m. UTC | #8 
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
Bernhard Beschow Sept. 24, 2023, 3:41 p.m. UTC | #9 
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
>
diff mbox series

Patch

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