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

From: Jason Andryuk <jandryuk@gmail.com>
To: Anthony PERARD <anthony.perard@citrix.com>
Cc: "Bernhard Beschow" <shentey@gmail.com>,
	qemu-devel@nongnu.org,
	"Marcel Apfelbaum" <marcel.apfelbaum@gmail.com>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"David Woodhouse" <dwmw@amazon.co.uk>,
	"Hervé Poussineau" <hpoussin@reactos.org>,
	"Aurelien Jarno" <aurelien@aurel32.net>,
	"Eduardo Habkost" <eduardo@habkost.net>,
	"Paul Durrant" <paul@xen.org>,
	xen-devel@lists.xenproject.org,
	"Michael S. Tsirkin" <mst@redhat.com>,
	"Stefano Stabellini" <sstabellini@kernel.org>,
	"Richard Henderson" <richard.henderson@linaro.org>,
	"Philippe Mathieu-Daudé" <philmd@linaro.org>,
	"Chuck Zmudzinski" <brchuckz@aol.com>
Subject: Re: [PATCH v3 2/6] hw/isa/piix3: Reuse piix3_realize() in piix3_xen_realize()
Date: Mon, 3 Apr 2023 08:27:14 -0400	[thread overview]
Message-ID: <CAKf6xpvxf=F52etJ8o3eLQV4JVD5WM57znGoP3ctONRf7uPisA@mail.gmail.com> (raw)
In-Reply-To: <622b9674-fffd-4634-ac30-d0db3230478e@perard>

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