* Re: [PATCH] hw/acpi: refactor acpi hp modules so that targets can just use what they need
2021-08-12 7:14 [PATCH] hw/acpi: refactor acpi hp modules so that targets can just use what they need Ani Sinha
@ 2021-08-12 13:22 ` Ani Sinha
2021-08-12 13:49 ` Philippe Mathieu-Daudé
2021-08-18 11:46 ` Ani Sinha
` (2 subsequent siblings)
3 siblings, 1 reply; 10+ messages in thread
From: Ani Sinha @ 2021-08-12 13:22 UTC (permalink / raw)
To: Ani Sinha
Cc: Aleksandar Rikalo, Michael S. Tsirkin, qemu-devel,
Philippe Mathieu-Daudé,
Igor Mammedov, philmd, Aurelien Jarno
On Thu, 12 Aug 2021, 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
I do not have a cross compiled mips64 vmlinux handy, so can't verify that
issue #193 was indeed resolved. I have verified that #221 was indeed
fixed.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] hw/acpi: refactor acpi hp modules so that targets can just use what they need
2021-08-12 13:22 ` Ani Sinha
@ 2021-08-12 13:49 ` Philippe Mathieu-Daudé
2021-08-12 15:27 ` Ani Sinha
0 siblings, 1 reply; 10+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-08-12 13:49 UTC (permalink / raw)
To: Ani Sinha
Cc: Aleksandar Rikalo, Michael S. Tsirkin, qemu-devel, Igor Mammedov,
philmd, Aurelien Jarno
On 8/12/21 3:22 PM, Ani Sinha wrote:
> On Thu, 12 Aug 2021, 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
>
> I do not have a cross compiled mips64 vmlinux handy, so can't verify that
> issue #193 was indeed resolved.
The functional tests use some pre-built:
$ git grep I6400 tests/acceptance/
> I have verified that #221 was indeed
> fixed.
Good news!
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] hw/acpi: refactor acpi hp modules so that targets can just use what they need
2021-08-12 13:49 ` Philippe Mathieu-Daudé
@ 2021-08-12 15:27 ` Ani Sinha
0 siblings, 0 replies; 10+ messages in thread
From: Ani Sinha @ 2021-08-12 15:27 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Aleksandar Rikalo, Michael S. Tsirkin, qemu-devel, Ani Sinha,
Igor Mammedov, philmd, Aurelien Jarno
[-- Attachment #1: Type: text/plain, Size: 9725 bytes --]
On Thu, 12 Aug 2021, Philippe Mathieu-Daudé wrote:
> On 8/12/21 3:22 PM, Ani Sinha wrote:
> > On Thu, 12 Aug 2021, 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
> >
> > I do not have a cross compiled mips64 vmlinux handy, so can't verify that
> > issue #193 was indeed resolved.
>
> The functional tests use some pre-built:
>
> $ git grep I6400 tests/acceptance/
>
Cool! Fantastic! Tested #193 as well, no crashes:
$ ./qemu-system-mips64el -cpu I6400 -nographic -append "clocksource=GIC console=ttyS1" -smp 8 -kernel ../vmlinux
Linux version 4.7.0-rc1-dirty (root@2e66df87a9ff) (gcc version 6.3.0
20170516 (Debian 6.3.0-18) ) #1 SMP Sat Feb 1 18:38:13 UTC 2020
earlycon: uart8250 at I/O port 0x3f8 (options '38400n8')
bootconsole [uart8250] enabled
CPU0 revision is: 0001a900 (MIPS I6400)
FPU revision is: 20f30300
MSA revision is: 00000300
MIPS: machine is mti,malta
Software DMA cache coherency enabled
Determined physical RAM map:
memory: 0000000008000000 @ 0000000000000000 (usable)
Zone ranges:
DMA [mem 0x0000000000000000-0x0000000000ffffff]
DMA32 [mem 0x0000000001000000-0x00000000ffffffff]
Normal empty
Movable zone start for each node
Early memory node ranges
node 0: [mem 0x0000000000000000-0x0000000007ffffff]
Initmem setup node 0 [mem 0x0000000000000000-0x0000000007ffffff]
VP topology {8} total 8
Primary instruction cache 64kB, VIPT, 4-way, linesize 64 bytes.
Primary data cache 64kB, 4-way, VIPT, no aliases, linesize 64 bytes
percpu: Embedded 5 pages/cpu @980000000107c000 s29664 r8192 d44064 u81920
Built 1 zonelists in Zone order, mobility grouping on. Total pages: 8163
Kernel command line: clocksource=GIC console=ttyS1
log_buf_len individual max cpu contribution: 4096 bytes
log_buf_len total cpu_extra contributions: 28672 bytes
log_buf_len min size: 32768 bytes
log_buf_len: 65536 bytes
early log buf free: 30424(92%)
PID hash table entries: 512 (order: -2, 4096 bytes)
Dentry cache hash table entries: 16384 (order: 3, 131072 bytes)
Inode-cache hash table entries: 8192 (order: 2, 65536 bytes)
Writing ErrCtl register=00000000
Readback ErrCtl register=00000000
MAAR configuration:
[0]: 0x0000000000010000-0x0000000007ffffff speculate
[1]: disabled
[2]: disabled
[3]: disabled
[4]: disabled
[5]: disabled
[6]: disabled
[7]: disabled
Memory: 121104K/131072K available (5253K kernel code, 380K rwdata, 1276K
rodata, 304K init, 278K bss, 9968K reserved, 0K cma-reserved)
Hierarchical RCU implementation.
Build-time adjustment of leaf fanout to 64.
NR_IRQS:256
CPU frequency 333.33 MHz
GIC frequency 100.00 MHz
clocksource: GIC: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns:
19113018267 ns
clocksource: MIPS: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns:
11467565337 ns
sched_clock: 32 bits at 166MHz, resolution 6ns, wraps every 12884904956ns
Console: colour dummy device 80x25
Calibrating delay loop... 1561.39 BogoMIPS (lpj=7806976)
pid_max: default: 32768 minimum: 301
Mount-cache hash table entries: 2048 (order: 0, 16384 bytes)
Mountpoint-cache hash table entries: 2048 (order: 0, 16384 bytes)
Primary instruction cache 64kB, VIPT, 4-way, linesize 64 bytes.
Primary data cache 64kB, 4-way, VIPT, no aliases, linesize 64 bytes
CPU1 revision is: 0001a900 (MIPS I6400)
FPU revision is: 20f30300
MSA revision is: 00000300
Synchronize counters for CPU 1: done.
Primary instruction cache 64kB, VIPT, 4-way, linesize 64 bytes.
Primary data cache 64kB, 4-way, VIPT, no aliases, linesize 64 bytes
CPU2 revision is: 0001a900 (MIPS I6400)
FPU revision is: 20f30300
MSA revision is: 00000300
Synchronize counters for CPU 2: done.
Primary instruction cache 64kB, VIPT, 4-way, linesize 64 bytes.
Primary data cache 64kB, 4-way, VIPT, no aliases, linesize 64 bytes
CPU3 revision is: 0001a900 (MIPS I6400)
FPU revision is: 20f30300
MSA revision is: 00000300
Synchronize counters for CPU 3: done.
Primary instruction cache 64kB, VIPT, 4-way, linesize 64 bytes.
Primary data cache 64kB, 4-way, VIPT, no aliases, linesize 64 bytes
CPU4 revision is: 0001a900 (MIPS I6400)
FPU revision is: 20f30300
MSA revision is: 00000300
Synchronize counters for CPU 4: done.
Primary instruction cache 64kB, VIPT, 4-way, linesize 64 bytes.
Primary data cache 64kB, 4-way, VIPT, no aliases, linesize 64 bytes
CPU5 revision is: 0001a900 (MIPS I6400)
FPU revision is: 20f30300
MSA revision is: 00000300
Synchronize counters for CPU 5: done.
Primary instruction cache 64kB, VIPT, 4-way, linesize 64 bytes.
Primary data cache 64kB, 4-way, VIPT, no aliases, linesize 64 bytes
CPU6 revision is: 0001a900 (MIPS I6400)
FPU revision is: 20f30300
MSA revision is: 00000300
Synchronize counters for CPU 6: done.
Primary instruction cache 64kB, VIPT, 4-way, linesize 64 bytes.
Primary data cache 64kB, 4-way, VIPT, no aliases, linesize 64 bytes
CPU7 revision is: 0001a900 (MIPS I6400)
FPU revision is: 20f30300
MSA revision is: 00000300
Synchronize counters for CPU 7: done.
Brought up 8 CPUs
devtmpfs: initialized
clocksource: jiffies: mask: 0xffffffff max_cycles: 0xffffffff,
max_idle_ns: 19112604462750000 ns
NET: Registered protocol family 16
pm-cps: CPC does not support clock gating
vgaarb: loaded
SCSI subsystem initialized
PCI host bridge to bus 0000:00
pci_bus 0000:00: root bus resource [mem 0x10000000-0x17ffffff]
pci_bus 0000:00: root bus resource [io 0x1000-0x1fffff]
pci_bus 0000:00: root bus resource [??? 0x00000000 flags 0x0]
pci_bus 0000:00: No busn resource found for root bus, will use [bus 00-ff]
pci 0000:00:00.0: [Firmware Bug]: reg 0x14: invalid BAR (can't size)
pci 0000:00:00.0: [Firmware Bug]: reg 0x18: invalid BAR (can't size)
pci 0000:00:00.0: [Firmware Bug]: reg 0x1c: invalid BAR (can't size)
pci 0000:00:00.0: [Firmware Bug]: reg 0x20: invalid BAR (can't size)
pci 0000:00:00.0: [Firmware Bug]: reg 0x24: invalid BAR (can't size)
pci 0000:00:0a.1: legacy IDE quirk: reg 0x10: [io 0x01f0-0x01f7]
pci 0000:00:0a.1: legacy IDE quirk: reg 0x14: [io 0x03f6]
pci 0000:00:0a.1: legacy IDE quirk: reg 0x18: [io 0x0170-0x0177]
pci 0000:00:0a.1: legacy IDE quirk: reg 0x1c: [io 0x0376]
pci 0000:00:0a.3: quirk: [io 0x1000-0x103f] claimed by PIIX4 ACPI
pci 0000:00:0a.3: quirk: [io 0x1100-0x110f] claimed by PIIX4 SMB
vgaarb: device added: PCI:0000:00:12.0,decodes=io+mem,owns=none,locks=none
pci 0000:00:12.0: BAR 0: assigned [mem 0x10000000-0x11ffffff pref]
pci 0000:00:0b.0: BAR 6: assigned [mem 0x12000000-0x1203ffff pref]
pci 0000:00:12.0: BAR 6: assigned [mem 0x12040000-0x1204ffff pref]
pci 0000:00:12.0: BAR 1: assigned [mem 0x12050000-0x12050fff]
pci 0000:00:0a.2: BAR 4: assigned [io 0x1040-0x105f]
pci 0000:00:0b.0: BAR 0: assigned [io 0x1060-0x107f]
pci 0000:00:0b.0: BAR 1: assigned [mem 0x12051000-0x1205101f]
pci 0000:00:0a.1: BAR 4: assigned [io 0x1080-0x108f]
clocksource: Switched to clocksource GIC
VFS: Disk quotas dquot_6.6.0
VFS: Dquot-cache hash table entries: 2048 (order 0, 16384 bytes)
NET: Registered protocol family 2
TCP established hash table entries: 2048 (order: 0, 16384 bytes)
TCP bind hash table entries: 2048 (order: 1, 32768 bytes)
TCP: Hash tables configured (established 2048 bind 2048)
UDP hash table entries: 512 (order: 0, 16384 bytes)
UDP-Lite hash table entries: 512 (order: 0, 16384 bytes)
NET: Registered protocol family 1
RPC: Registered named UNIX socket transport module.
RPC: Registered udp transport module.
RPC: Registered tcp transport module.
RPC: Registered tcp NFSv4.1 backchannel transport module.
futex hash table entries: 2048 (order: 3, 131072 bytes)
workingset: timestamp_bits=60 max_order=13 bucket_order=0
Installing knfsd (copyright (C) 1996 okir@monad.swb.de).
Block layer SCSI generic (bsg) driver version 0.4 loaded (major 253)
io scheduler noop registered
io scheduler deadline registered
io scheduler cfq registered (default)
PCI: Enabling device 0000:00:12.0 (0000 -> 0002)
cirrusfb 0000:00:12.0: Cirrus Logic chipset on PCI bus, RAM (4096 kB) at
0x10000000
Console: switching to colour frame buffer device 80x30
hrtimer: interrupt took 159860621 ns
Serial: 8250/16550 driver, 4 ports, IRQ sharing disabled
serial8250.0: ttyS0 at I/O 0x3f8 (irq = 4, base_baud = 115200) is a 16550A
serial8250.0: ttyS1 at I/O 0x2f8 (irq = 3, base_baud = 115200) is a 16550A
console [ttyS1] enabled
bootconsole [uart8250] disabled
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] hw/acpi: refactor acpi hp modules so that targets can just use what they need
2021-08-12 7:14 [PATCH] hw/acpi: refactor acpi hp modules so that targets can just use what they need Ani Sinha
2021-08-12 13:22 ` Ani Sinha
@ 2021-08-18 11:46 ` Ani Sinha
2021-08-19 11:06 ` Philippe Mathieu-Daudé
2021-09-01 12:19 ` Ani Sinha
3 siblings, 0 replies; 10+ messages in thread
From: Ani Sinha @ 2021-08-18 11:46 UTC (permalink / raw)
To: Ani Sinha
Cc: Aleksandar Rikalo, Michael S. Tsirkin, qemu-devel,
Philippe Mathieu-Daudé,
Igor Mammedov, philmd, Aurelien Jarno
Ping ...
On Thu, 12 Aug 2021, 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;
> +}
> +
> +void legacy_acpi_cpu_hotplug_init(MemoryRegion *parent, Object *owner,
> + AcpiCpuHotplug *gpe_cpu, uint16_t base)
> +{
> + return;
> +}
> +
> +void acpi_cpu_ospm_status(CPUHotplugState *cpu_st, ACPIOSTInfoList ***list)
> +{
> + return;
> +}
> +
> +void acpi_cpu_plug_cb(HotplugHandler *hotplug_dev,
> + CPUHotplugState *cpu_st, DeviceState *dev, Error **errp)
> +{
> + return;
> +}
> +
> +void legacy_acpi_cpu_plug_cb(HotplugHandler *hotplug_dev,
> + AcpiCpuHotplug *g, DeviceState *dev, Error **errp)
> +{
> + return;
> +}
> +
> +void acpi_cpu_unplug_cb(CPUHotplugState *cpu_st,
> + DeviceState *dev, Error **errp)
> +{
> + return;
> +}
> +
> +void acpi_cpu_unplug_request_cb(HotplugHandler *hotplug_dev,
> + CPUHotplugState *cpu_st,
> + DeviceState *dev, Error **errp)
> +{
> + return;
> +}
> diff --git a/hw/acpi/acpi-mem-hotplug-stub.c b/hw/acpi/acpi-mem-hotplug-stub.c
> new file mode 100644
> index 0000000000..73a076a265
> --- /dev/null
> +++ b/hw/acpi/acpi-mem-hotplug-stub.c
> @@ -0,0 +1,35 @@
> +#include "qemu/osdep.h"
> +#include "hw/acpi/memory_hotplug.h"
> +#include "migration/vmstate.h"
> +
> +const VMStateDescription vmstate_memory_hotplug;
> +
> +void acpi_memory_hotplug_init(MemoryRegion *as, Object *owner,
> + MemHotplugState *state, hwaddr io_base)
> +{
> + return;
> +}
> +
> +void acpi_memory_ospm_status(MemHotplugState *mem_st, ACPIOSTInfoList ***list)
> +{
> + return;
> +}
> +
> +void acpi_memory_plug_cb(HotplugHandler *hotplug_dev, MemHotplugState *mem_st,
> + DeviceState *dev, Error **errp)
> +{
> + return;
> +}
> +
> +void acpi_memory_unplug_cb(MemHotplugState *mem_st,
> + DeviceState *dev, Error **errp)
> +{
> + return;
> +}
> +
> +void acpi_memory_unplug_request_cb(HotplugHandler *hotplug_dev,
> + MemHotplugState *mem_st,
> + DeviceState *dev, Error **errp)
> +{
> + return;
> +}
> diff --git a/hw/acpi/acpi-nvdimm-stub.c b/hw/acpi/acpi-nvdimm-stub.c
> new file mode 100644
> index 0000000000..8baff9be6f
> --- /dev/null
> +++ b/hw/acpi/acpi-nvdimm-stub.c
> @@ -0,0 +1,8 @@
> +#include "qemu/osdep.h"
> +#include "hw/mem/nvdimm.h"
> +#include "hw/hotplug.h"
> +
> +void nvdimm_acpi_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev)
> +{
> + return;
> +}
> diff --git a/hw/acpi/acpi-pci-hotplug-stub.c b/hw/acpi/acpi-pci-hotplug-stub.c
> new file mode 100644
> index 0000000000..734e4c5986
> --- /dev/null
> +++ b/hw/acpi/acpi-pci-hotplug-stub.c
> @@ -0,0 +1,47 @@
> +#include "qemu/osdep.h"
> +#include "hw/acpi/pcihp.h"
> +#include "migration/vmstate.h"
> +
> +const VMStateDescription vmstate_acpi_pcihp_pci_status;
> +
> +void acpi_pcihp_init(Object *owner, AcpiPciHpState *s, PCIBus *root_bus,
> + MemoryRegion *address_space_io, bool bridges_enabled,
> + uint16_t io_base)
> +{
> + return;
> +}
> +
> +void acpi_pcihp_device_plug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s,
> + DeviceState *dev, Error **errp)
> +{
> + return;
> +}
> +
> +void acpi_pcihp_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> + DeviceState *dev, Error **errp)
> +{
> + return;
> +}
> +
> +void acpi_pcihp_device_unplug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s,
> + DeviceState *dev, Error **errp)
> +{
> + return;
> +}
> +
> +void acpi_pcihp_device_unplug_request_cb(HotplugHandler *hotplug_dev,
> + AcpiPciHpState *s, DeviceState *dev,
> + Error **errp)
> +{
> + return;
> +}
> +
> +void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off)
> +{
> + return;
> +}
> +
> +bool vmstate_acpi_pcihp_use_acpi_index(void *opaque, int version_id)
> +{
> + return false;
> +}
> diff --git a/hw/acpi/meson.build b/hw/acpi/meson.build
> index 29f804d13e..7d8c0eb43e 100644
> --- a/hw/acpi/meson.build
> +++ b/hw/acpi/meson.build
> @@ -6,16 +6,20 @@ acpi_ss.add(files(
> 'core.c',
> 'utils.c',
> ))
> -acpi_ss.add(when: 'CONFIG_ACPI_CPU_HOTPLUG', if_true: files('cpu.c'))
> -acpi_ss.add(when: 'CONFIG_ACPI_CPU_HOTPLUG', if_true: files('cpu_hotplug.c'))
> +acpi_ss.add(when: 'CONFIG_ACPI_CPU_HOTPLUG', if_true: files('cpu.c', 'cpu_hotplug.c'))
> +acpi_ss.add(when: 'CONFIG_ACPI_CPU_HOTPLUG', if_false: files('acpi-cpu-hotplug-stub.c'))
> acpi_ss.add(when: 'CONFIG_ACPI_MEMORY_HOTPLUG', if_true: files('memory_hotplug.c'))
> +acpi_ss.add(when: 'CONFIG_ACPI_MEMORY_HOTPLUG', if_false: files('acpi-mem-hotplug-stub.c'))
> acpi_ss.add(when: 'CONFIG_ACPI_NVDIMM', if_true: files('nvdimm.c'))
> +acpi_ss.add(when: 'CONFIG_ACPI_NVDIMM', if_false: files('acpi-nvdimm-stub.c'))
> acpi_ss.add(when: 'CONFIG_ACPI_PCI', if_true: files('pci.c'))
> acpi_ss.add(when: 'CONFIG_ACPI_VMGENID', if_true: files('vmgenid.c'))
> acpi_ss.add(when: 'CONFIG_ACPI_HW_REDUCED', if_true: files('generic_event_device.c'))
> acpi_ss.add(when: 'CONFIG_ACPI_HMAT', if_true: files('hmat.c'))
> acpi_ss.add(when: 'CONFIG_ACPI_APEI', if_true: files('ghes.c'), if_false: files('ghes-stub.c'))
> -acpi_ss.add(when: 'CONFIG_ACPI_X86', if_true: files('piix4.c', 'pcihp.c'))
> +acpi_ss.add(when: 'CONFIG_ACPI_PIIX4', if_true: files('piix4.c'))
> +acpi_ss.add(when: 'CONFIG_ACPI_PCIHP', if_true: files('pcihp.c'))
> +acpi_ss.add(when: 'CONFIG_ACPI_PCIHP', if_false: files('acpi-pci-hotplug-stub.c'))
> acpi_ss.add(when: 'CONFIG_ACPI_X86_ICH', if_true: files('ich9.c', 'tco.c'))
> acpi_ss.add(when: 'CONFIG_IPMI', if_true: files('ipmi.c'), if_false: files('ipmi-stub.c'))
> acpi_ss.add(when: 'CONFIG_PC', if_false: files('acpi-x86-stub.c'))
> @@ -23,4 +27,6 @@ acpi_ss.add(when: 'CONFIG_TPM', if_true: files('tpm.c'))
> softmmu_ss.add(when: 'CONFIG_ACPI', if_false: files('acpi-stub.c', 'aml-build-stub.c', 'ghes-stub.c'))
> softmmu_ss.add_all(when: 'CONFIG_ACPI', if_true: acpi_ss)
> softmmu_ss.add(when: 'CONFIG_ALL', if_true: files('acpi-stub.c', 'aml-build-stub.c',
> - 'acpi-x86-stub.c', 'ipmi-stub.c', 'ghes-stub.c'))
> + 'acpi-x86-stub.c', 'ipmi-stub.c', 'ghes-stub.c',
> + 'acpi-mem-hotplug-stub.c', 'acpi-cpu-hotplug-stub.c',
> + 'acpi-pci-hotplug-stub.c', 'acpi-nvdimm-stub.c'))
> --
> 2.25.1
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] hw/acpi: refactor acpi hp modules so that targets can just use what they need
2021-08-12 7:14 [PATCH] hw/acpi: refactor acpi hp modules so that targets can just use what they need Ani Sinha
2021-08-12 13:22 ` Ani Sinha
2021-08-18 11:46 ` Ani Sinha
@ 2021-08-19 11:06 ` Philippe Mathieu-Daudé
2021-08-19 12:52 ` Ani Sinha
2021-08-20 15:43 ` Ani Sinha
2021-09-01 12:19 ` Ani Sinha
3 siblings, 2 replies; 10+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-08-19 11:06 UTC (permalink / raw)
To: Ani Sinha, qemu-devel
Cc: Aleksandar Rikalo, Michael S. Tsirkin,
Philippe Mathieu-Daudé,
Paolo Bonzini, Igor Mammedov, Aurelien Jarno
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?
Your patch is not incorrect, and indeed fixes the issues, but
I feel we are going backward now allowing call which should
never be there in the first place.
But x86 is a Frankenstein, you simply add more band aid patches
to hide its uglyness.
Up to the maintainer I guess.
> +}
> +
> +void legacy_acpi_cpu_hotplug_init(MemoryRegion *parent, Object *owner,
> + AcpiCpuHotplug *gpe_cpu, uint16_t base)
> +{
> + return;
> +}
> +
> +void acpi_cpu_ospm_status(CPUHotplugState *cpu_st, ACPIOSTInfoList ***list)
> +{
> + return;
> +}
> +
> +void acpi_cpu_plug_cb(HotplugHandler *hotplug_dev,
> + CPUHotplugState *cpu_st, DeviceState *dev, Error **errp)
> +{
> + return;
> +}
> +
> +void legacy_acpi_cpu_plug_cb(HotplugHandler *hotplug_dev,
> + AcpiCpuHotplug *g, DeviceState *dev, Error **errp)
> +{
> + return;
> +}
> +
> +void acpi_cpu_unplug_cb(CPUHotplugState *cpu_st,
> + DeviceState *dev, Error **errp)
> +{
> + return;
> +}
> +
> +void acpi_cpu_unplug_request_cb(HotplugHandler *hotplug_dev,
> + CPUHotplugState *cpu_st,
> + DeviceState *dev, Error **errp)
> +{
> + return;
> +}
> diff --git a/hw/acpi/acpi-mem-hotplug-stub.c b/hw/acpi/acpi-mem-hotplug-stub.c
> new file mode 100644
> index 0000000000..73a076a265
> --- /dev/null
> +++ b/hw/acpi/acpi-mem-hotplug-stub.c
> @@ -0,0 +1,35 @@
> +#include "qemu/osdep.h"
> +#include "hw/acpi/memory_hotplug.h"
> +#include "migration/vmstate.h"
> +
> +const VMStateDescription vmstate_memory_hotplug;
> +
> +void acpi_memory_hotplug_init(MemoryRegion *as, Object *owner,
> + MemHotplugState *state, hwaddr io_base)
> +{
> + return;
> +}
> +
> +void acpi_memory_ospm_status(MemHotplugState *mem_st, ACPIOSTInfoList ***list)
> +{
> + return;
> +}
> +
> +void acpi_memory_plug_cb(HotplugHandler *hotplug_dev, MemHotplugState *mem_st,
> + DeviceState *dev, Error **errp)
> +{
> + return;
> +}
> +
> +void acpi_memory_unplug_cb(MemHotplugState *mem_st,
> + DeviceState *dev, Error **errp)
> +{
> + return;
> +}
> +
> +void acpi_memory_unplug_request_cb(HotplugHandler *hotplug_dev,
> + MemHotplugState *mem_st,
> + DeviceState *dev, Error **errp)
> +{
> + return;
> +}
> diff --git a/hw/acpi/acpi-nvdimm-stub.c b/hw/acpi/acpi-nvdimm-stub.c
> new file mode 100644
> index 0000000000..8baff9be6f
> --- /dev/null
> +++ b/hw/acpi/acpi-nvdimm-stub.c
> @@ -0,0 +1,8 @@
> +#include "qemu/osdep.h"
> +#include "hw/mem/nvdimm.h"
> +#include "hw/hotplug.h"
> +
> +void nvdimm_acpi_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev)
> +{
> + return;
> +}
> diff --git a/hw/acpi/acpi-pci-hotplug-stub.c b/hw/acpi/acpi-pci-hotplug-stub.c
> new file mode 100644
> index 0000000000..734e4c5986
> --- /dev/null
> +++ b/hw/acpi/acpi-pci-hotplug-stub.c
> @@ -0,0 +1,47 @@
> +#include "qemu/osdep.h"
> +#include "hw/acpi/pcihp.h"
> +#include "migration/vmstate.h"
> +
> +const VMStateDescription vmstate_acpi_pcihp_pci_status;
> +
> +void acpi_pcihp_init(Object *owner, AcpiPciHpState *s, PCIBus *root_bus,
> + MemoryRegion *address_space_io, bool bridges_enabled,
> + uint16_t io_base)
> +{
> + return;
> +}
> +
> +void acpi_pcihp_device_plug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s,
> + DeviceState *dev, Error **errp)
> +{
> + return;
> +}
> +
> +void acpi_pcihp_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> + DeviceState *dev, Error **errp)
> +{
> + return;
> +}
> +
> +void acpi_pcihp_device_unplug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s,
> + DeviceState *dev, Error **errp)
> +{
> + return;
> +}
> +
> +void acpi_pcihp_device_unplug_request_cb(HotplugHandler *hotplug_dev,
> + AcpiPciHpState *s, DeviceState *dev,
> + Error **errp)
> +{
> + return;
> +}
> +
> +void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off)
> +{
> + return;
> +}
> +
> +bool vmstate_acpi_pcihp_use_acpi_index(void *opaque, int version_id)
> +{
> + return false;
> +}
> diff --git a/hw/acpi/meson.build b/hw/acpi/meson.build
> index 29f804d13e..7d8c0eb43e 100644
> --- a/hw/acpi/meson.build
> +++ b/hw/acpi/meson.build
> @@ -6,16 +6,20 @@ acpi_ss.add(files(
> 'core.c',
> 'utils.c',
> ))
> -acpi_ss.add(when: 'CONFIG_ACPI_CPU_HOTPLUG', if_true: files('cpu.c'))
> -acpi_ss.add(when: 'CONFIG_ACPI_CPU_HOTPLUG', if_true: files('cpu_hotplug.c'))
> +acpi_ss.add(when: 'CONFIG_ACPI_CPU_HOTPLUG', if_true: files('cpu.c', 'cpu_hotplug.c'))
> +acpi_ss.add(when: 'CONFIG_ACPI_CPU_HOTPLUG', if_false: files('acpi-cpu-hotplug-stub.c'))
> acpi_ss.add(when: 'CONFIG_ACPI_MEMORY_HOTPLUG', if_true: files('memory_hotplug.c'))
> +acpi_ss.add(when: 'CONFIG_ACPI_MEMORY_HOTPLUG', if_false: files('acpi-mem-hotplug-stub.c'))
> acpi_ss.add(when: 'CONFIG_ACPI_NVDIMM', if_true: files('nvdimm.c'))
> +acpi_ss.add(when: 'CONFIG_ACPI_NVDIMM', if_false: files('acpi-nvdimm-stub.c'))
> acpi_ss.add(when: 'CONFIG_ACPI_PCI', if_true: files('pci.c'))
> acpi_ss.add(when: 'CONFIG_ACPI_VMGENID', if_true: files('vmgenid.c'))
> acpi_ss.add(when: 'CONFIG_ACPI_HW_REDUCED', if_true: files('generic_event_device.c'))
> acpi_ss.add(when: 'CONFIG_ACPI_HMAT', if_true: files('hmat.c'))
> acpi_ss.add(when: 'CONFIG_ACPI_APEI', if_true: files('ghes.c'), if_false: files('ghes-stub.c'))
> -acpi_ss.add(when: 'CONFIG_ACPI_X86', if_true: files('piix4.c', 'pcihp.c'))
> +acpi_ss.add(when: 'CONFIG_ACPI_PIIX4', if_true: files('piix4.c'))
> +acpi_ss.add(when: 'CONFIG_ACPI_PCIHP', if_true: files('pcihp.c'))
> +acpi_ss.add(when: 'CONFIG_ACPI_PCIHP', if_false: files('acpi-pci-hotplug-stub.c'))
> acpi_ss.add(when: 'CONFIG_ACPI_X86_ICH', if_true: files('ich9.c', 'tco.c'))
> acpi_ss.add(when: 'CONFIG_IPMI', if_true: files('ipmi.c'), if_false: files('ipmi-stub.c'))
> acpi_ss.add(when: 'CONFIG_PC', if_false: files('acpi-x86-stub.c'))
> @@ -23,4 +27,6 @@ acpi_ss.add(when: 'CONFIG_TPM', if_true: files('tpm.c'))
> softmmu_ss.add(when: 'CONFIG_ACPI', if_false: files('acpi-stub.c', 'aml-build-stub.c', 'ghes-stub.c'))
> softmmu_ss.add_all(when: 'CONFIG_ACPI', if_true: acpi_ss)
> softmmu_ss.add(when: 'CONFIG_ALL', if_true: files('acpi-stub.c', 'aml-build-stub.c',
> - 'acpi-x86-stub.c', 'ipmi-stub.c', 'ghes-stub.c'))
> + 'acpi-x86-stub.c', 'ipmi-stub.c', 'ghes-stub.c',
> + 'acpi-mem-hotplug-stub.c', 'acpi-cpu-hotplug-stub.c',
> + 'acpi-pci-hotplug-stub.c', 'acpi-nvdimm-stub.c'))
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] hw/acpi: refactor acpi hp modules so that targets can just use what they need
2021-08-19 11:06 ` Philippe Mathieu-Daudé
@ 2021-08-19 12:52 ` Ani Sinha
2021-08-20 15:43 ` Ani Sinha
1 sibling, 0 replies; 10+ messages in thread
From: Ani Sinha @ 2021-08-19 12:52 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Aleksandar Rikalo, Michael S. Tsirkin, qemu-devel,
Philippe Mathieu-Daudé,
Paolo Bonzini, Ani Sinha, Igor Mammedov, Aurelien Jarno
[-- Attachment #1: Type: text/plain, Size: 5308 bytes --]
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)
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] hw/acpi: refactor acpi hp modules so that targets can just use what they need
2021-08-19 11:06 ` Philippe Mathieu-Daudé
2021-08-19 12:52 ` Ani Sinha
@ 2021-08-20 15:43 ` Ani Sinha
2021-08-20 15:59 ` Philippe Mathieu-Daudé
1 sibling, 1 reply; 10+ messages in thread
From: Ani Sinha @ 2021-08-20 15:43 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Aleksandar Rikalo, Michael S. Tsirkin, qemu-devel,
Philippe Mathieu-Daudé,
Paolo Bonzini, Ani Sinha, Igor Mammedov, Aurelien Jarno
[-- Attachment #1: Type: text/plain, Size: 714 bytes --]
On Thu, 19 Aug 2021, Philippe Mathieu-Daudé wrote:
> On 8/12/21 9:14 AM, Ani Sinha wrote:
> > + return;
>
> I suppose if you replace all 'return' by 'g_assert_not_reached()'
> both issues reproducers crash?
>
> Your patch is not incorrect, and indeed fixes the issues, but
> I feel we are going backward now allowing call which should
> never be there in the first place.
>
Linux kernel does something like this all over the place. They simply
replace functions with NOOPS when they are not allowed for a
configuration. They do this relying on preprocessor macros ofcourse!
It will be hard to do anythiung better without rearchitecting the modules.
That would have significant impact particularly on x86.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] hw/acpi: refactor acpi hp modules so that targets can just use what they need
2021-08-20 15:43 ` Ani Sinha
@ 2021-08-20 15:59 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 10+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-08-20 15:59 UTC (permalink / raw)
To: Ani Sinha
Cc: Aleksandar Rikalo, Michael S. Tsirkin, qemu-devel,
Philippe Mathieu-Daudé,
Paolo Bonzini, Igor Mammedov, Aurelien Jarno
On 8/20/21 5:43 PM, Ani Sinha wrote:
> On Thu, 19 Aug 2021, Philippe Mathieu-Daudé wrote:
>> On 8/12/21 9:14 AM, Ani Sinha wrote:
>
>>> + return;
>>
>> I suppose if you replace all 'return' by 'g_assert_not_reached()'
>> both issues reproducers crash?
>>
>> Your patch is not incorrect, and indeed fixes the issues, but
>> I feel we are going backward now allowing call which should
>> never be there in the first place.
>>
>
> Linux kernel does something like this all over the place. They simply
> replace functions with NOOPS when they are not allowed for a
> configuration. They do this relying on preprocessor macros ofcourse!
>
> It will be hard to do anythiung better without rearchitecting the modules.
Which is why this situation is unsolved since various years <:)
> That would have significant impact particularly on x86.
I don't believe so, the rework has to be done at the machine creation,
no change during runtime.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] hw/acpi: refactor acpi hp modules so that targets can just use what they need
2021-08-12 7:14 [PATCH] hw/acpi: refactor acpi hp modules so that targets can just use what they need Ani Sinha
` (2 preceding siblings ...)
2021-08-19 11:06 ` Philippe Mathieu-Daudé
@ 2021-09-01 12:19 ` Ani Sinha
3 siblings, 0 replies; 10+ messages in thread
From: Ani Sinha @ 2021-09-01 12:19 UTC (permalink / raw)
To: QEMU Developers
Cc: Aleksandar Rikalo, Michael S. Tsirkin,
Philippe Mathieu-Daudé,
Igor Mammedov, Philippe Mathieu-Daudé,
Aurelien Jarno
Ping ...
On Thu, Aug 12, 2021 at 12:44 PM Ani Sinha <ani@anisinha.ca> 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;
> +}
> +
> +void legacy_acpi_cpu_hotplug_init(MemoryRegion *parent, Object *owner,
> + AcpiCpuHotplug *gpe_cpu, uint16_t base)
> +{
> + return;
> +}
> +
> +void acpi_cpu_ospm_status(CPUHotplugState *cpu_st, ACPIOSTInfoList ***list)
> +{
> + return;
> +}
> +
> +void acpi_cpu_plug_cb(HotplugHandler *hotplug_dev,
> + CPUHotplugState *cpu_st, DeviceState *dev, Error **errp)
> +{
> + return;
> +}
> +
> +void legacy_acpi_cpu_plug_cb(HotplugHandler *hotplug_dev,
> + AcpiCpuHotplug *g, DeviceState *dev, Error **errp)
> +{
> + return;
> +}
> +
> +void acpi_cpu_unplug_cb(CPUHotplugState *cpu_st,
> + DeviceState *dev, Error **errp)
> +{
> + return;
> +}
> +
> +void acpi_cpu_unplug_request_cb(HotplugHandler *hotplug_dev,
> + CPUHotplugState *cpu_st,
> + DeviceState *dev, Error **errp)
> +{
> + return;
> +}
> diff --git a/hw/acpi/acpi-mem-hotplug-stub.c b/hw/acpi/acpi-mem-hotplug-stub.c
> new file mode 100644
> index 0000000000..73a076a265
> --- /dev/null
> +++ b/hw/acpi/acpi-mem-hotplug-stub.c
> @@ -0,0 +1,35 @@
> +#include "qemu/osdep.h"
> +#include "hw/acpi/memory_hotplug.h"
> +#include "migration/vmstate.h"
> +
> +const VMStateDescription vmstate_memory_hotplug;
> +
> +void acpi_memory_hotplug_init(MemoryRegion *as, Object *owner,
> + MemHotplugState *state, hwaddr io_base)
> +{
> + return;
> +}
> +
> +void acpi_memory_ospm_status(MemHotplugState *mem_st, ACPIOSTInfoList ***list)
> +{
> + return;
> +}
> +
> +void acpi_memory_plug_cb(HotplugHandler *hotplug_dev, MemHotplugState *mem_st,
> + DeviceState *dev, Error **errp)
> +{
> + return;
> +}
> +
> +void acpi_memory_unplug_cb(MemHotplugState *mem_st,
> + DeviceState *dev, Error **errp)
> +{
> + return;
> +}
> +
> +void acpi_memory_unplug_request_cb(HotplugHandler *hotplug_dev,
> + MemHotplugState *mem_st,
> + DeviceState *dev, Error **errp)
> +{
> + return;
> +}
> diff --git a/hw/acpi/acpi-nvdimm-stub.c b/hw/acpi/acpi-nvdimm-stub.c
> new file mode 100644
> index 0000000000..8baff9be6f
> --- /dev/null
> +++ b/hw/acpi/acpi-nvdimm-stub.c
> @@ -0,0 +1,8 @@
> +#include "qemu/osdep.h"
> +#include "hw/mem/nvdimm.h"
> +#include "hw/hotplug.h"
> +
> +void nvdimm_acpi_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev)
> +{
> + return;
> +}
> diff --git a/hw/acpi/acpi-pci-hotplug-stub.c b/hw/acpi/acpi-pci-hotplug-stub.c
> new file mode 100644
> index 0000000000..734e4c5986
> --- /dev/null
> +++ b/hw/acpi/acpi-pci-hotplug-stub.c
> @@ -0,0 +1,47 @@
> +#include "qemu/osdep.h"
> +#include "hw/acpi/pcihp.h"
> +#include "migration/vmstate.h"
> +
> +const VMStateDescription vmstate_acpi_pcihp_pci_status;
> +
> +void acpi_pcihp_init(Object *owner, AcpiPciHpState *s, PCIBus *root_bus,
> + MemoryRegion *address_space_io, bool bridges_enabled,
> + uint16_t io_base)
> +{
> + return;
> +}
> +
> +void acpi_pcihp_device_plug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s,
> + DeviceState *dev, Error **errp)
> +{
> + return;
> +}
> +
> +void acpi_pcihp_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> + DeviceState *dev, Error **errp)
> +{
> + return;
> +}
> +
> +void acpi_pcihp_device_unplug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s,
> + DeviceState *dev, Error **errp)
> +{
> + return;
> +}
> +
> +void acpi_pcihp_device_unplug_request_cb(HotplugHandler *hotplug_dev,
> + AcpiPciHpState *s, DeviceState *dev,
> + Error **errp)
> +{
> + return;
> +}
> +
> +void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off)
> +{
> + return;
> +}
> +
> +bool vmstate_acpi_pcihp_use_acpi_index(void *opaque, int version_id)
> +{
> + return false;
> +}
> diff --git a/hw/acpi/meson.build b/hw/acpi/meson.build
> index 29f804d13e..7d8c0eb43e 100644
> --- a/hw/acpi/meson.build
> +++ b/hw/acpi/meson.build
> @@ -6,16 +6,20 @@ acpi_ss.add(files(
> 'core.c',
> 'utils.c',
> ))
> -acpi_ss.add(when: 'CONFIG_ACPI_CPU_HOTPLUG', if_true: files('cpu.c'))
> -acpi_ss.add(when: 'CONFIG_ACPI_CPU_HOTPLUG', if_true: files('cpu_hotplug.c'))
> +acpi_ss.add(when: 'CONFIG_ACPI_CPU_HOTPLUG', if_true: files('cpu.c', 'cpu_hotplug.c'))
> +acpi_ss.add(when: 'CONFIG_ACPI_CPU_HOTPLUG', if_false: files('acpi-cpu-hotplug-stub.c'))
> acpi_ss.add(when: 'CONFIG_ACPI_MEMORY_HOTPLUG', if_true: files('memory_hotplug.c'))
> +acpi_ss.add(when: 'CONFIG_ACPI_MEMORY_HOTPLUG', if_false: files('acpi-mem-hotplug-stub.c'))
> acpi_ss.add(when: 'CONFIG_ACPI_NVDIMM', if_true: files('nvdimm.c'))
> +acpi_ss.add(when: 'CONFIG_ACPI_NVDIMM', if_false: files('acpi-nvdimm-stub.c'))
> acpi_ss.add(when: 'CONFIG_ACPI_PCI', if_true: files('pci.c'))
> acpi_ss.add(when: 'CONFIG_ACPI_VMGENID', if_true: files('vmgenid.c'))
> acpi_ss.add(when: 'CONFIG_ACPI_HW_REDUCED', if_true: files('generic_event_device.c'))
> acpi_ss.add(when: 'CONFIG_ACPI_HMAT', if_true: files('hmat.c'))
> acpi_ss.add(when: 'CONFIG_ACPI_APEI', if_true: files('ghes.c'), if_false: files('ghes-stub.c'))
> -acpi_ss.add(when: 'CONFIG_ACPI_X86', if_true: files('piix4.c', 'pcihp.c'))
> +acpi_ss.add(when: 'CONFIG_ACPI_PIIX4', if_true: files('piix4.c'))
> +acpi_ss.add(when: 'CONFIG_ACPI_PCIHP', if_true: files('pcihp.c'))
> +acpi_ss.add(when: 'CONFIG_ACPI_PCIHP', if_false: files('acpi-pci-hotplug-stub.c'))
> acpi_ss.add(when: 'CONFIG_ACPI_X86_ICH', if_true: files('ich9.c', 'tco.c'))
> acpi_ss.add(when: 'CONFIG_IPMI', if_true: files('ipmi.c'), if_false: files('ipmi-stub.c'))
> acpi_ss.add(when: 'CONFIG_PC', if_false: files('acpi-x86-stub.c'))
> @@ -23,4 +27,6 @@ acpi_ss.add(when: 'CONFIG_TPM', if_true: files('tpm.c'))
> softmmu_ss.add(when: 'CONFIG_ACPI', if_false: files('acpi-stub.c', 'aml-build-stub.c', 'ghes-stub.c'))
> softmmu_ss.add_all(when: 'CONFIG_ACPI', if_true: acpi_ss)
> softmmu_ss.add(when: 'CONFIG_ALL', if_true: files('acpi-stub.c', 'aml-build-stub.c',
> - 'acpi-x86-stub.c', 'ipmi-stub.c', 'ghes-stub.c'))
> + 'acpi-x86-stub.c', 'ipmi-stub.c', 'ghes-stub.c',
> + 'acpi-mem-hotplug-stub.c', 'acpi-cpu-hotplug-stub.c',
> + 'acpi-pci-hotplug-stub.c', 'acpi-nvdimm-stub.c'))
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 10+ messages in thread