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

[Peter Maydell]

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