[RFC,V2,12/37] hw/acpi: Use qemu_present_cpu() API in ACPI CPU hotplug init

Commit Message

Salil Mehta Sept. 26, 2023, 10:04 a.m. UTC

ACPI CPU Hotplug code assumes a virtual CPU is unplugged if the CPUState object
is absent in the list of ths possible CPUs(CPUArchIdList *possible_cpus)
maintained on per-machine basis. Use the earlier introduced qemu_present_cpu()
API to check this state.

This change should have no bearing on the functionality of any architecture and
is mere a representational change.

Signed-off-by: Salil Mehta <[e-mail]>
---
 hw/acpi/cpu.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Gavin Shan Sept. 28, 2023, 12:40 a.m. UTC | #1

Hi Salil,

On 9/26/23 20:04, Salil Mehta wrote:
> ACPI CPU Hotplug code assumes a virtual CPU is unplugged if the CPUState object
> is absent in the list of ths possible CPUs(CPUArchIdList *possible_cpus)
> maintained on per-machine basis. Use the earlier introduced qemu_present_cpu()
> API to check this state.
> 
> This change should have no bearing on the functionality of any architecture and
> is mere a representational change.
> 
> Signed-off-by: Salil Mehta <[e-mail]>
> ---
>   hw/acpi/cpu.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
> index 45defdc0e2..d5ba37b209 100644
> --- a/hw/acpi/cpu.c
> +++ b/hw/acpi/cpu.c
> @@ -225,7 +225,10 @@ void cpu_hotplug_hw_init(MemoryRegion *as, Object *owner,
>       state->dev_count = id_list->len;
>       state->devs = g_new0(typeof(*state->devs), state->dev_count);
>       for (i = 0; i < id_list->len; i++) {
> -        state->devs[i].cpu =  CPU(id_list->cpus[i].cpu);
> +        struct CPUState *cpu = CPU(id_list->cpus[i].cpu);
> +        if (qemu_present_cpu(cpu)) {
> +            state->devs[i].cpu = cpu;
> +        }
>           state->devs[i].arch_id = id_list->cpus[i].arch_id;
>       }
>       memory_region_init_io(&state->ctrl_reg, owner, &cpu_hotplug_ops, state,

I don't think qemu_present_cpu() is needed because all possible vCPUs are present
for x86 and arm64 at this point? Besides, we have the assumption all hotpluggable
vCPUs are present, looking at James' kernel series where ACPI_HOTPLUG_PRESENT_CPU
exists in linux/drivers/acpi/Kconfig :)

Thanks,
Gavin

Salil Mehta Oct. 16, 2023, 9:41 p.m. UTC | #2

> From: Gavin Shan <[e-mail]>
> Sent: Thursday, September 28, 2023 1:41 AM
> To: Salil Mehta <[e-mail]>; [e-mail]; [e-mail]
> Cc: [e-mail]; [e-mail]; Jonathan Cameron
> <[e-mail]>; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> zhukeqian <[e-mail]>; wangxiongfeng (C)
> <[e-mail]>; wangyanan (Y) <[e-mail]>;
> [e-mail]; [e-mail]; [e-mail]
> Subject: Re: [PATCH RFC V2 12/37] hw/acpi: Use qemu_present_cpu() API in
> ACPI CPU hotplug init
> 
> Hi Salil,
> 
> On 9/26/23 20:04, Salil Mehta wrote:
> > ACPI CPU Hotplug code assumes a virtual CPU is unplugged if the CPUState object
> > is absent in the list of ths possible CPUs(CPUArchIdList *possible_cpus)
> > maintained on per-machine basis. Use the earlier introduced qemu_present_cpu()
> > API to check this state.
> >
> > This change should have no bearing on the functionality of any architecture and
> > is mere a representational change.
> >
> > Signed-off-by: Salil Mehta <[e-mail]>
> > ---
> >   hw/acpi/cpu.c | 5 ++++-
> >   1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
> > index 45defdc0e2..d5ba37b209 100644
> > --- a/hw/acpi/cpu.c
> > +++ b/hw/acpi/cpu.c
> > @@ -225,7 +225,10 @@ void cpu_hotplug_hw_init(MemoryRegion *as, Object *owner,
> >       state->dev_count = id_list->len;
> >       state->devs = g_new0(typeof(*state->devs), state->dev_count);
> >       for (i = 0; i < id_list->len; i++) {
> > -        state->devs[i].cpu =  CPU(id_list->cpus[i].cpu);
> > +        struct CPUState *cpu = CPU(id_list->cpus[i].cpu);
> > +        if (qemu_present_cpu(cpu)) {
> > +            state->devs[i].cpu = cpu;
> > +        }
> >           state->devs[i].arch_id = id_list->cpus[i].arch_id;
> >       }
> >       memory_region_init_io(&state->ctrl_reg, owner, &cpu_hotplug_ops, state,
> 
> I don't think qemu_present_cpu() is needed because all possible vCPUs are present
> for x86 and arm64 at this point? Besides, we have the assumption all hotpluggable
> vCPUs are present, looking at James' kernel series where ACPI_HOTPLUG_PRESENT_CPU
> exists in linux/drivers/acpi/Kconfig :)

No, for x86 not all possible vCPUs need to be present at VM init. IOAPIC has
got no such limitation like GIC. Hot-pluggable vCPUs can be deferred created.
Hence, QOM CPUState objects will not be present and ACPI will expose them as
NOT PRESENT to the Guest OS.

This is not the case with ARM. Not all possible vCPUs are present at QOM
level but QEMU *fakes* the presence of vCPUs i.e. the unplugged ones to
the Guest OS by exposing them as PRESENT in ACPI _STA method. Later is
required due to the architectural limitation of ARM. GIC CPU interfaces
need to be present in MADT for all possible vCPUs at guest boot time.

Hence, we require above change.


Thanks
Salil.

Patch

diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
index 45defdc0e2..d5ba37b209 100644
--- a/hw/acpi/cpu.c
+++ b/hw/acpi/cpu.c
@@ -225,7 +225,10 @@  void cpu_hotplug_hw_init(MemoryRegion *as, Object *owner,
     state->dev_count = id_list->len;
     state->devs = g_new0(typeof(*state->devs), state->dev_count);
     for (i = 0; i < id_list->len; i++) {
-        state->devs[i].cpu =  CPU(id_list->cpus[i].cpu);
+        struct CPUState *cpu = CPU(id_list->cpus[i].cpu);
+        if (qemu_present_cpu(cpu)) {
+            state->devs[i].cpu = cpu;
+        }
         state->devs[i].arch_id = id_list->cpus[i].arch_id;
     }
     memory_region_init_io(&state->ctrl_reg, owner, &cpu_hotplug_ops, state,