Re: [PULL 06/35] hw/acpi: refactor acpi hp modules so that targets can just use what they need

From: Ani Sinha <ani@anisinha.ca>
To: Peter Maydell <peter.maydell@linaro.org>
Cc: "Michael S. Tsirkin" <mst@redhat.com>,
	qemu-devel@nongnu.org, "Ani Sinha" <ani@anisinha.ca>,
	"Philippe Mathieu-Daudé" <f4bug@amsat.org>,
	"Aurelien Jarno" <aurelien@aurel32.net>,
	"Jiaxun Yang" <jiaxun.yang@flygoat.com>,
	"Aleksandar Rikalo" <aleksandar.rikalo@syrmia.com>,
	"Igor Mammedov" <imammedo@redhat.com>
Subject: Re: [PULL 06/35] hw/acpi: refactor acpi hp modules so that targets can just use what they need
Date: Wed, 20 Jul 2022 11:37:19 -0700 (PDT)	[thread overview]
Message-ID: <alpine.DEB.2.22.394.2207201132430.9370@anisinha-lenovo> (raw)
In-Reply-To: <CAFEAcA9WBo2Kn9BPz1y2JCxpBGnBWDOtgLFiu31V4PL2m6b7bQ@mail.gmail.com>

On Tue, 19 Jul 2022, Peter Maydell wrote:

> On Sat, 4 Sept 2021 at 22:36, Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > From: Ani Sinha <ani@anisinha.ca>
> >
> > Currently various acpi hotplug modules like cpu hotplug, memory hotplug, pci
> > hotplug, nvdimm hotplug are all pulled in when CONFIG_ACPI_X86 is turned on.
> > This brings in support for whole lot of subsystems that some targets like
> > mips does not need. They are added just to satisfy symbol dependencies. This
> > is ugly and should be avoided. Targets should be able to pull in just what they
> > need and no more. For example, mips only needs support for PIIX4 and does not
> > need acpi pci hotplug support or cpu hotplug support or memory hotplug support
> > etc. This change is an effort to clean this up.
> > In this change, new config variables are added for various acpi hotplug
> > subsystems. Targets like mips can only enable PIIX4 support and not the rest
> > of all the other modules which were being previously pulled in as a part of
> > CONFIG_ACPI_X86. Function stubs make sure that symbols which piix4 needs but
> > are not required by mips (for example, symbols specific to pci hotplug etc)
> > are available to satisfy the dependencies.
> >
> > Currently, this change only addresses issues with mips malta targets. In future
> > we might be able to clean up other targets which are similarly pulling in lot
> > of unnecessary hotplug modules by enabling ACPI_X86.
> >
> > This change should also address issues such as the following:
> > https://gitlab.com/qemu-project/qemu/-/issues/221
> > https://gitlab.com/qemu-project/qemu/-/issues/193
> >
> > Signed-off-by: Ani Sinha <ani@anisinha.ca>
> > Message-Id: <20210812071409.492299-1-ani@anisinha.ca>
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> Hi; I'm trying to track down the fix for a bug that I think
> was introduced by this change. Specifically, if you
> configure with a target list of
>  '--target-list=mips-linux-user,mips64-linux-user,mipsel-linux-user,mipsn32-linux-user,mipsn32el-linux-user,mips-softmmu,mipsel-softmmu,mips64-softmmu,mips64el-softmmu'
>
> (ie "just mips"), then run 'make check', the iotest 267 fails
> because QEMU segfaults trying to do a VM save/restore on
> qemu-system-mips. (You can run just that iotest by cd'ing
> into the build dir's tests/qemu-iotests/ subdir and running
>   ./check -qcow2 -gdb 267
> if you like).
>
> This is because hw/acpi/piix4.c (used by the MIPS malta board)
> has a vmstate that includes use of the VMSTATE_PCI_HOTPLUG()
> macro. This macro uses the vmstate_acpi_pcihp_pci_status
> vmstate struct. If the MIPS target is built along with some
> other targets which require CONFIG_ACPI_PCIHP then we correctly
> get the real definition of that vmstate struct from pcihp.c.
> However, if we are only building the MIPS targets then
> CONFIG_ACPI_PCIHP is false, and we get the dummy definition
> of the struct from acpi-pci-hotplug-stub.c. The dummy definition
> obviously doesn't actually work for migrating anything, and
> in fact the migration code ends up segfaulting because
> the 'name' field in the struct is NULL. (MIPS builds and
> uses hw/acpi/piix4.c because
> configs/devices/mips-softmmu/common.mak sets CONFIG_ACPI_PIIX4=y,
> and it needs this because piix4_init() unconditionally
> creates a TYPE_PIIX4_PM device. It's possible this is a bug
> revealed/introduced by the recent piix4 refactoring, but I
> had a look at the code at the time this change was committed
> and afaict back then it also created the PIIX4_PM device,
> just in a different place. Indeed it is this commit which adds
> the CONFIG_ACPI_PIIX4=y to the config!)
>
> How is this intended to work? The obvious fix from my point
> of view would just be to say "piix4.c requires pcihp.c"
> and cause CONFIG_ACPI_PIIX4 to pull in CONFIG_ACPI_PCIHP,
> but that seems like it would be rather undoing the point
> of this change.

Yes. From the commit log and the vague recollection I have about this
change :

> For example, mips only needs support for PIIX4 and does not
> need acpi pci hotplug support or cpu hotplug support or memory hotplug
support
> etc

So does malta really need acpi hotplug? If not, then the stubbing out of
the vmstate struct is correct.

But if Malta requires ACPI_PIIX4 and it
> creates the PIIX4_PM device, it needs the real pcihp.c and
> not the stubs, doesn't it ?
>
> thanks
> -- PMM
>