[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] hw/acpi: refactor acpi hp modules so that targets can just u
From: |
Ani Sinha |
Subject: |
Re: [PATCH] hw/acpi: refactor acpi hp modules so that targets can just use what they need |
Date: |
Thu, 19 Aug 2021 18:22:34 +0530 (IST) |
User-agent: |
Alpine 2.22 (DEB 394 2020-01-19) |
On Thu, 19 Aug 2021, Philippe Mathieu-Daudé wrote:
> On 8/12/21 9:14 AM, Ani Sinha wrote:
> > 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>
> > ---
> > configs/devices/mips-softmmu/common.mak | 5 +--
> > hw/acpi/Kconfig | 10 +++++
> > hw/acpi/acpi-cpu-hotplug-stub.c | 50 +++++++++++++++++++++++++
> > hw/acpi/acpi-mem-hotplug-stub.c | 35 +++++++++++++++++
> > hw/acpi/acpi-nvdimm-stub.c | 8 ++++
> > hw/acpi/acpi-pci-hotplug-stub.c | 47 +++++++++++++++++++++++
> > hw/acpi/meson.build | 14 +++++--
> > 7 files changed, 161 insertions(+), 8 deletions(-)
> > create mode 100644 hw/acpi/acpi-cpu-hotplug-stub.c
> > create mode 100644 hw/acpi/acpi-mem-hotplug-stub.c
> > create mode 100644 hw/acpi/acpi-nvdimm-stub.c
> > create mode 100644 hw/acpi/acpi-pci-hotplug-stub.c
> >
> > diff --git a/configs/devices/mips-softmmu/common.mak
> > b/configs/devices/mips-softmmu/common.mak
> > index ea78fe7275..752b62b1e6 100644
> > --- a/configs/devices/mips-softmmu/common.mak
> > +++ b/configs/devices/mips-softmmu/common.mak
> > @@ -18,10 +18,7 @@ CONFIG_PCSPK=y
> > CONFIG_PCKBD=y
> > CONFIG_FDC=y
> > CONFIG_ACPI=y
> > -CONFIG_ACPI_X86=y
> > -CONFIG_ACPI_MEMORY_HOTPLUG=y
> > -CONFIG_ACPI_NVDIMM=y
> > -CONFIG_ACPI_CPU_HOTPLUG=y
> > +CONFIG_ACPI_PIIX4=y
> > CONFIG_APM=y
> > CONFIG_I8257=y
> > CONFIG_PIIX4=y
> > diff --git a/hw/acpi/Kconfig b/hw/acpi/Kconfig
> > index cfc4ede8d9..3b5e118c54 100644
> > --- a/hw/acpi/Kconfig
> > +++ b/hw/acpi/Kconfig
> > @@ -8,6 +8,8 @@ config ACPI_X86
> > select ACPI_CPU_HOTPLUG
> > select ACPI_MEMORY_HOTPLUG
> > select ACPI_HMAT
> > + select ACPI_PIIX4
> > + select ACPI_PCIHP
> >
> > config ACPI_X86_ICH
> > bool
> > @@ -24,6 +26,14 @@ config ACPI_NVDIMM
> > bool
> > depends on ACPI
> >
> > +config ACPI_PIIX4
> > + bool
> > + depends on ACPI
> > +
> > +config ACPI_PCIHP
> > + bool
> > + depends on ACPI
> > +
> > config ACPI_HMAT
> > bool
> > depends on ACPI
> > diff --git a/hw/acpi/acpi-cpu-hotplug-stub.c
> > b/hw/acpi/acpi-cpu-hotplug-stub.c
> > new file mode 100644
> > index 0000000000..3fc4b14c26
> > --- /dev/null
> > +++ b/hw/acpi/acpi-cpu-hotplug-stub.c
> > @@ -0,0 +1,50 @@
> > +#include "qemu/osdep.h"
> > +#include "hw/acpi/cpu_hotplug.h"
> > +#include "migration/vmstate.h"
> > +
> > +
> > +/* Following stubs are all related to ACPI cpu hotplug */
> > +const VMStateDescription vmstate_cpu_hotplug;
> > +
> > +void acpi_switch_to_modern_cphp(AcpiCpuHotplug *gpe_cpu,
> > + CPUHotplugState *cpuhp_state,
> > + uint16_t io_port)
> > +{
> > + return;
>
> I suppose if you replace all 'return' by 'g_assert_not_reached()'
> both issues reproducers crash?
yes, I presume so. For example, with the following change :
diff --git a/hw/acpi/acpi-cpu-hotplug-stub.c b/hw/acpi/acpi-cpu-hotplug-stub.c
index 3fc4b14c26..9725e4a81b 100644
--- a/hw/acpi/acpi-cpu-hotplug-stub.c
+++ b/hw/acpi/acpi-cpu-hotplug-stub.c
@@ -16,7 +16,7 @@ void acpi_switch_to_modern_cphp(AcpiCpuHotplug *gpe_cpu,
void legacy_acpi_cpu_hotplug_init(MemoryRegion *parent, Object *owner,
AcpiCpuHotplug *gpe_cpu, uint16_t base)
{
- return;
+ g_assert_not_reached();
}
void acpi_cpu_ospm_status(CPUHotplugState *cpu_st, ACPIOSTInfoList ***list)
I get the following crash :
./qemu-system-mips -machine malta -bios /dev/null -nodefaults -monitor stdio -S
QEMU 6.0.93 monitor - type 'help' for more information
(qemu) **
ERROR:../hw/acpi/acpi-cpu-hotplug-stub.c:19:legacy_acpi_cpu_hotplug_init:
code should not be reached
Bail out!
ERROR:../hw/acpi/acpi-cpu-hotplug-stub.c:19:legacy_acpi_cpu_hotplug_init:
code should not be reached
Aborted (core dumped)