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

[Bernhard Beschow <shentey@xxxxxxxxx>]

Am 20. September 2023 14:44:23 UTC schrieb Chuck Zmudzinski <brchuckz@xxxxxxx>:
>On 9/19/2023 4:02 PM, Bernhard Beschow wrote:
>> 
>> 
>> Am 3. April 2023 12:27:14 UTC schrieb Jason Andryuk <jandryuk@xxxxxxxxx>:
>>>On Mon, Apr 3, 2023 at 5:33 AM Anthony PERARD <anthony.perard@xxxxxxxxxx> 
>>>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@xxxxxxxxxx>:
>>>> > >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
>