Re: [PATCH v3] hw/acpi: add an assertion check for non-null return from acpi_get_i386_pci_host

[Ani Sinha]

On Fri, 6 Aug 2021, Igor Mammedov wrote:

> On Thu, 5 Aug 2021 19:42:35 +0530 (IST)
> Ani Sinha <ani@anisinha.ca> wrote:
>
> > On Thu, 5 Aug 2021, Ani Sinha wrote:
> >
> > >
> > >
> > > On Thu, 5 Aug 2021, Ani Sinha wrote:
> > >
> > > >
> > > >
> > > > On Thu, 5 Aug 2021, Igor Mammedov wrote:
> > > >
> > > > > On Mon, 26 Jul 2021 22:27:43 +0530
> > > > > Ani Sinha <ani@anisinha.ca> wrote:
> > > > >
> > > > > > All existing code using acpi_get_i386_pci_host() checks for a 
> > > > > > non-null
> > > > > > return value from this function call. Instead of returning early 
> > > > > > when the value
> > > > > > returned is NULL, assert instead. Since there are only two possible 
> > > > > > host buses
> > > > > > for i386 - q35 and i440fx, a null value return from the function 
> > > > > > does not make
> > > > > > sense in most cases and is likely an error situation.
> > > > > >
> > > > > > Fixes: c0e427d6eb5fef ("hw/acpi/ich9: Enable ACPI PCI hot-plug")
> > > > > >
> > > > > > Signed-off-by: Ani Sinha <ani@anisinha.ca>
> > > > > > ---
> > > > > >  hw/acpi/pcihp.c      |  8 ++++++++
> > > > > >  hw/i386/acpi-build.c | 15 ++++++---------
> > > > > >  2 files changed, 14 insertions(+), 9 deletions(-)
> > > > > >
> > > > > > changelog:
> > > > > > v1: initial patch
> > > > > > v2: removed comment addition - that can be sent as a separate patch.
> > > > > > v3: added assertion for null host values for all cases except one.
> > > > > >
> > > > > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > > > > > index f4d706e47d..054ee8cbc5 100644
> > > > > > --- a/hw/acpi/pcihp.c
> > > > > > +++ b/hw/acpi/pcihp.c
> > > > > > @@ -116,6 +116,12 @@ static void acpi_set_pci_info(void)
> > > > > >      bsel_is_set = true;
> > > > > >
> > > > > >      if (!host) {
> > > > > > +        /*
> > > > > > +         * This function can be eventually called from
> > > > > > +         * qemu_devices_reset() -> acpi_pcihp_reset() even
> > > > > > +         * for architectures other than i386. Hence, we need
> > > > > > +         * to ignore null values for host here.
> > > > > > +         */
> > > > > >          return;
> > > > > >      }
> > > > >
> > > > > I suspect it's a MIPS target that call this code unnecessarily.
> > > > > It would be better to get rid of this condition altogether.
> > > > > Frr that I can suggest to make acpi_pcihp_reset() stub and
> > > > > replace pcihp.c with stub (perhaps use acpi-x86-stub.c) when building
> > > > > for MIPS.
> > > > >
> > > > > then a bunch of asserts/ifs won't be necessary,
> > > > > just one in acpi_get_i386_pci_host() will be sufficient.
> > > > >
> > > >
> > > > OK this is a good idea.
> > > > I can see that mips-softmmu-config-devices.h has
> > > > CONFIG_ACPI_X86 turned on for mips. This does not seem right.
> > > >
> > > > The issue here is:
> > > >
> > > > $ grep -R CONFIG_ACPI_X86 *
> > > > devices/mips-softmmu/common.mak:CONFIG_ACPI_X86=y
> > > >
> > > > So after
> > > >
> > > > -CONFIG_ACPI_X86=y
> > > > -CONFIG_PIIX4=y
> > > >
> > > > (the second one is needed because after removing first one we get:
> > > >
> > > > /usr/bin/ld: libcommon.fa.p/hw_isa_piix4.c.o: in function 
> > > > `piix4_create':
> > > > /home/anisinha/workspace/qemu/build/../hw/isa/piix4.c:269: undefined
> > > > reference to `piix4_pm_init'
> > > >
> > > > This is because in hw/acpi/meson.build, piix4.c is conditional on
> > > > CONFIG_ACPI_X86. )
> > > >
> > > > /usr/bin/ld: libqemu-mips-softmmu.fa.p/hw_mips_gt64xxx_pci.c.o: in
> > > > function `gt64120_pci_set_irq':
> > > > /home/anisinha/workspace/qemu/build/../hw/mips/gt64xxx_pci.c:1020:
> > > > undefined reference to `piix4_dev'
> > > > /usr/bin/ld: libqemu-mips-softmmu.fa.p/hw_mips_malta.c.o: in function
> > > > `mips_malta_init':
> > > > /home/anisinha/workspace/qemu/build/../hw/mips/malta.c:1404: undefined
> > > > reference to `piix4_create'
> > > >
> > > > So should mips be doing piix stuff anyway? Is Piix4 etc not x86 
> > > > specific?
> > >
> > > Apparently this is by design:
> > > https://qemu.readthedocs.io/en/stable/system/target-mips.html
> > >
> > > which means mips malta will continue to use the x86 specific functions
> > > like acpi_pcihp_reset(). Creating a stub for this with acpi-x86-stub.c
> > > will result in a double symbol definition because CONFIG_PC is off for
> > > mips.
> > >
> >
> > Also to be noted that there is a stub for acpi_get_i386_pci_host() which
> > simply returns NULL. This activates when CONFIG_PC is disabled. It is this
> > stub that gets called for mips and hence the check for non-null host is
> > needed in acpi_set_pci_info() function.
> that were half measures to deal around code that shouldn't be called,
> now with pcihp being used by both pc and q35 we don't have reason to
> keep around null checks modulo mips calling code that shouldn't be
> called there to begin with.

So malta mips does not need ACPI hotplug? In that case, maybe we should
not make pcihp.c dependent on CONFIG_ACPI_X86. Ideas welcome.