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