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