From: Ani Sinha <ani@anisinha.ca>
To: Ani Sinha <ani@anisinha.ca>
Cc: Eduardo Habkost <ehabkost@redhat.com>,
"Michael S. Tsirkin" <mst@redhat.com>,
Richard Henderson <richard.henderson@linaro.org>,
jusual@redhat.com, qemu-devel@nongnu.org,
Paolo Bonzini <pbonzini@redhat.com>,
Igor Mammedov <imammedo@redhat.com>
Subject: Re: [PATCH v3] hw/acpi: add an assertion check for non-null return from acpi_get_i386_pci_host
Date: Thu, 5 Aug 2021 18:29:20 +0530 (IST) [thread overview]
Message-ID: <alpine.DEB.2.22.394.2108051826190.291909@anisinha-lenovo> (raw)
In-Reply-To: <alpine.DEB.2.22.394.2108051705240.291909@anisinha-lenovo>
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.
next prev parent reply other threads:[~2021-08-05 13:06 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-26 16:57 [PATCH v3] hw/acpi: add an assertion check for non-null return from acpi_get_i386_pci_host Ani Sinha
2021-07-28 13:03 ` Michael S. Tsirkin
2021-07-28 14:12 ` Ani Sinha
2021-07-29 4:38 ` Ani Sinha
2021-07-29 6:10 ` Ani Sinha
2021-08-03 11:34 ` Ani Sinha
2021-08-05 9:15 ` Igor Mammedov
2021-08-05 12:09 ` Ani Sinha
2021-08-05 12:59 ` Ani Sinha [this message]
2021-08-05 14:12 ` Ani Sinha
2021-08-05 22:13 ` Michael S. Tsirkin
2021-08-06 10:33 ` Igor Mammedov
2021-08-06 10:37 ` Igor Mammedov
2021-08-06 10:52 ` Ani Sinha
2021-08-06 14:01 ` Philippe Mathieu-Daudé
2021-08-06 16:38 ` Philippe Mathieu-Daudé
2021-08-09 16:21 ` Ani Sinha
2021-08-11 15:00 ` Ani Sinha
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=alpine.DEB.2.22.394.2108051826190.291909@anisinha-lenovo \
--to=ani@anisinha.ca \
--cc=ehabkost@redhat.com \
--cc=imammedo@redhat.com \
--cc=jusual@redhat.com \
--cc=mst@redhat.com \
--cc=pbonzini@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=richard.henderson@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).