[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3] hw/acpi: add an assertion check for non-null return from
From: |
Ani Sinha |
Subject: |
Re: [PATCH v3] hw/acpi: add an assertion check for non-null return from acpi_get_i386_pci_host |
Date: |
Wed, 11 Aug 2021 20:30:36 +0530 (IST) |
User-agent: |
Alpine 2.22 (DEB 394 2020-01-19) |
On Fri, 6 Aug 2021, Philippe Mathieu-Daudé wrote:
> On 8/6/21 12:52 PM, Ani Sinha wrote:
> > 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?
>
> PIIX, PIIX3 and PIIX4 are generic chipsets, not X86-specific.
>
> QEMU's PIIX3 is a Frankenstein to support virtualization to a chipset
> not designed for it.
> If you look at it, the X86 machine use a PIIX3 but the PIIX3 doesn't
> even provide an ACPI function. It appeared in the PIIX4. The kludge is
> to instanciate the PIIX4.acpi from the PIIX3 and X86 ppl are happy with
> it, but it makes it ugly for the other architectures.
>
> >>>> Apparently this is by design:
> >>>> https://qemu.readthedocs.io/en/stable/system/target-mips.html
>
> What do you mean "by design"? The Malta uses a PIIX4 chipset for its
> southbridge indeed.
I meant it was intentional and not by accident.
>
> >>>> 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.
>
> Linux on Malta does use the ACPI features from the PIIX4.
>
> Please dig in the archives, Igor / myself already argued enough about
> this topic 2 years ago. The consensus was "yes, it is badly implemented,
> but it works and we don't have time to get it cleaner, pc machine is
> way more used than the malta one, so let not break the pc machines."
>
> See:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg613194.html
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg690435.html
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg725504.html
Ok I see what you were trying to do. I wanted to stub out the cpu and
memory hotplug bits for non x86 from piix4_acpi_system_hot_add_init().
Howwver, sadly, arm uses memory hotplug for its GED device:
cff51ac978c4fa0b3d0de0fd ("hw/arm/virt: Enable device memory cold/hot plug
with ACPI boot")
This makes things messy and complicated.
Anyways, I am trying something and will send out a patch if I am
successful.
- Re: [PATCH v3] hw/acpi: add an assertion check for non-null return from acpi_get_i386_pci_host, (continued)
- Re: [PATCH v3] hw/acpi: add an assertion check for non-null return from acpi_get_i386_pci_host, Ani Sinha, 2021/08/05
- Re: [PATCH v3] hw/acpi: add an assertion check for non-null return from acpi_get_i386_pci_host, Igor Mammedov, 2021/08/06
- Re: [PATCH v3] hw/acpi: add an assertion check for non-null return from acpi_get_i386_pci_host, Ani Sinha, 2021/08/06
- Re: [PATCH v3] hw/acpi: add an assertion check for non-null return from acpi_get_i386_pci_host, Philippe Mathieu-Daudé, 2021/08/06
- Re: [PATCH v3] hw/acpi: add an assertion check for non-null return from acpi_get_i386_pci_host, Philippe Mathieu-Daudé, 2021/08/06
- Re: [PATCH v3] hw/acpi: add an assertion check for non-null return from acpi_get_i386_pci_host, Ani Sinha, 2021/08/09
- Re: [PATCH v3] hw/acpi: add an assertion check for non-null return from acpi_get_i386_pci_host,
Ani Sinha <=