[RFC,v3,4/9] hw/arm/virt: Initialize the present cpu members

Commit Message

Yanan Wang May 16, 2021, 10:28 a.m. UTC

We create and initialize a cpuobj for each present cpu in
machvirt_init(). Now we also initialize the cpu member of
structure CPUArchId for each present cpu in the function.

This will be used to determine whether a cpu is present
when generating ACPI tables in later patches.

Co-developed-by: Ying Fang <[e-mail]>
Signed-off-by: Ying Fang <[e-mail]>
Signed-off-by: Yanan Wang <[e-mail]>
---
 hw/arm/virt.c | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Andrew Jones May 17, 2021, 6:43 a.m. UTC | #1

On Sun, May 16, 2021 at 06:28:55PM +0800, Yanan Wang wrote:
> We create and initialize a cpuobj for each present cpu in
> machvirt_init(). Now we also initialize the cpu member of
> structure CPUArchId for each present cpu in the function.
> 
> This will be used to determine whether a cpu is present
> when generating ACPI tables in later patches.
> 
> Co-developed-by: Ying Fang <[e-mail]>
> Signed-off-by: Ying Fang <[e-mail]>
> Signed-off-by: Yanan Wang <[e-mail]>
> ---
>  hw/arm/virt.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index e5dcdebdbc..50e324975f 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -2061,6 +2061,13 @@ static void machvirt_init(MachineState *machine)
>          }
>  
>          qdev_realize(DEVICE(cpuobj), NULL, &error_fatal);
> +
> +        /*
> +         * As ARM cpu hotplug is not supported yet, we initialize
> +         * the present cpu members here.
> +         */
> +        machine->possible_cpus->cpus[n].cpu = cpuobj;
> +
>          object_unref(cpuobj);
>      }
>      fdt_add_timer_nodes(vms);
> -- 
> 2.19.1
>

Reviewed-by: Andrew Jones <[e-mail]>

Salil Mehta May 17, 2021, 8:48 p.m. UTC | #2

> From: Qemu-arm [mailto:qemu-arm-bounces+salil.mehta=[e-mail]]
> On Behalf Of Yanan Wang
> Sent: Sunday, May 16, 2021 11:29 AM
> To: Peter Maydell <[e-mail]>; Andrew Jones
> <[e-mail]>; Michael S . Tsirkin <[e-mail]>; Igor Mammedov
> <[e-mail]>; Shannon Zhao <[e-mail]>; Alistair
> Francis <[e-mail]>; David Gibson
> <[e-mail]>; [e-mail]; [e-mail]
> Cc: Song Bao Hua (Barry Song) <[e-mail]>; zhukeqian
> <[e-mail]>; yangyicong <[e-mail]>; Zengtao (B)
> <[e-mail]>; Wanghaibin (D) <[e-mail]>;
> yuzenghui <[e-mail]>; Paolo Bonzini <[e-mail]>;
> Philippe Mathieu-Daudé <[e-mail]>
> Subject: [RFC PATCH v3 4/9] hw/arm/virt: Initialize the present cpu members
> 
> We create and initialize a cpuobj for each present cpu in
> machvirt_init(). Now we also initialize the cpu member of
> structure CPUArchId for each present cpu in the function.

[...]

>          qdev_realize(DEVICE(cpuobj), NULL, &error_fatal);
> +
> +        /*
> +         * As ARM cpu hotplug is not supported yet, we initialize
> +         * the present cpu members here.
> +         */
> +        machine->possible_cpus->cpus[n].cpu = cpuobj;


when vcpu Hotplug is not supported yet, what necessitates this change now?

Yanan Wang May 18, 2021, 4:42 a.m. UTC | #3

Hi Salil,

On 2021/5/18 4:48, Salil Mehta wrote:
>> From: Qemu-arm [mailto:qemu-arm-bounces+salil.mehta=[e-mail]]
>> On Behalf Of Yanan Wang
>> Sent: Sunday, May 16, 2021 11:29 AM
>> To: Peter Maydell <[e-mail]>; Andrew Jones
>> <[e-mail]>; Michael S . Tsirkin <[e-mail]>; Igor Mammedov
>> <[e-mail]>; Shannon Zhao <[e-mail]>; Alistair
>> Francis <[e-mail]>; David Gibson
>> <[e-mail]>; [e-mail]; [e-mail]
>> Cc: Song Bao Hua (Barry Song) <[e-mail]>; zhukeqian
>> <[e-mail]>; yangyicong <[e-mail]>; Zengtao (B)
>> <[e-mail]>; Wanghaibin (D) <[e-mail]>;
>> yuzenghui <[e-mail]>; Paolo Bonzini <[e-mail]>;
>> Philippe Mathieu-Daudé <[e-mail]>
>> Subject: [RFC PATCH v3 4/9] hw/arm/virt: Initialize the present cpu members
>>
>> We create and initialize a cpuobj for each present cpu in
>> machvirt_init(). Now we also initialize the cpu member of
>> structure CPUArchId for each present cpu in the function.
> [...]
>
>>           qdev_realize(DEVICE(cpuobj), NULL, &error_fatal);
>> +
>> +        /*
>> +         * As ARM cpu hotplug is not supported yet, we initialize
>> +         * the present cpu members here.
>> +         */
>> +        machine->possible_cpus->cpus[n].cpu = cpuobj;
>
> when vcpu Hotplug is not supported yet, what necessitates this change now?
>
The initialization will gives a way to determine whether a CPU is 
present or not.
At least, for now it will be used when generating ACPI tables, e.g. 
DSDT, MADT.
See patch 5 and 6.

Thanks,
Yanan
> .

Salil Mehta May 18, 2021, 7:04 a.m. UTC | #4

> From: wangyanan (Y)
> Sent: Tuesday, May 18, 2021 5:43 AM
> 
> Hi Salil,
> 
> On 2021/5/18 4:48, Salil Mehta wrote:
> >> From: Qemu-arm
> [mailto:qemu-arm-bounces+salil.mehta=[e-mail]]
> >> On Behalf Of Yanan Wang
> >> Sent: Sunday, May 16, 2021 11:29 AM
> >> To: Peter Maydell <[e-mail]>; Andrew Jones
> >> <[e-mail]>; Michael S . Tsirkin <[e-mail]>; Igor Mammedov
> >> <[e-mail]>; Shannon Zhao <[e-mail]>; Alistair
> >> Francis <[e-mail]>; David Gibson
> >> <[e-mail]>; [e-mail]; [e-mail]
> >> Cc: Song Bao Hua (Barry Song) <[e-mail]>; zhukeqian
> >> <[e-mail]>; yangyicong <[e-mail]>; Zengtao (B)
> >> <[e-mail]>; Wanghaibin (D) <[e-mail]>;
> >> yuzenghui <[e-mail]>; Paolo Bonzini <[e-mail]>;
> >> Philippe Mathieu-Daudé <[e-mail]>
> >> Subject: [RFC PATCH v3 4/9] hw/arm/virt: Initialize the present cpu members
> >>
> >> We create and initialize a cpuobj for each present cpu in
> >> machvirt_init(). Now we also initialize the cpu member of
> >> structure CPUArchId for each present cpu in the function.
> > [...]
> >
> >>           qdev_realize(DEVICE(cpuobj), NULL, &error_fatal);
> >> +
> >> +        /*
> >> +         * As ARM cpu hotplug is not supported yet, we initialize
> >> +         * the present cpu members here.
> >> +         */
> >> +        machine->possible_cpus->cpus[n].cpu = cpuobj;
> >
> > when vcpu Hotplug is not supported yet, what necessitates this change now?
> >
> The initialization will gives a way to determine whether a CPU is
> present or not.
> At least, for now it will be used when generating ACPI tables, e.g.
> DSDT, MADT.
> See patch 5 and 6.

yes,  but why do you require it now as part of the vcpu topology change?

As-far-as-i-can-see, PPTT table changes(part of patch 5/9) do not require
this change. Change in Patch 5/9 has also been done in anticipation of
some future requirement(vcpu Hotplug?).

Please correct me here if I am wrong?

Thanks

Andrew Jones May 18, 2021, 7:50 a.m. UTC | #5

On Tue, May 18, 2021 at 07:04:51AM +0000, Salil Mehta wrote:
> > From: wangyanan (Y)
> > Sent: Tuesday, May 18, 2021 5:43 AM
> > 
> > Hi Salil,
> > 
> > On 2021/5/18 4:48, Salil Mehta wrote:
> > >> From: Qemu-arm
> > [mailto:qemu-arm-bounces+salil.mehta=[e-mail]]
> > >> On Behalf Of Yanan Wang
> > >> Sent: Sunday, May 16, 2021 11:29 AM
> > >> To: Peter Maydell <[e-mail]>; Andrew Jones
> > >> <[e-mail]>; Michael S . Tsirkin <[e-mail]>; Igor Mammedov
> > >> <[e-mail]>; Shannon Zhao <[e-mail]>; Alistair
> > >> Francis <[e-mail]>; David Gibson
> > >> <[e-mail]>; [e-mail]; [e-mail]
> > >> Cc: Song Bao Hua (Barry Song) <[e-mail]>; zhukeqian
> > >> <[e-mail]>; yangyicong <[e-mail]>; Zengtao (B)
> > >> <[e-mail]>; Wanghaibin (D) <[e-mail]>;
> > >> yuzenghui <[e-mail]>; Paolo Bonzini <[e-mail]>;
> > >> Philippe Mathieu-Daudé <[e-mail]>
> > >> Subject: [RFC PATCH v3 4/9] hw/arm/virt: Initialize the present cpu members
> > >>
> > >> We create and initialize a cpuobj for each present cpu in
> > >> machvirt_init(). Now we also initialize the cpu member of
> > >> structure CPUArchId for each present cpu in the function.
> > > [...]
> > >
> > >>           qdev_realize(DEVICE(cpuobj), NULL, &error_fatal);
> > >> +
> > >> +        /*
> > >> +         * As ARM cpu hotplug is not supported yet, we initialize
> > >> +         * the present cpu members here.
> > >> +         */
> > >> +        machine->possible_cpus->cpus[n].cpu = cpuobj;
> > >
> > > when vcpu Hotplug is not supported yet, what necessitates this change now?
> > >
> > The initialization will gives a way to determine whether a CPU is
> > present or not.
> > At least, for now it will be used when generating ACPI tables, e.g.
> > DSDT, MADT.
> > See patch 5 and 6.
> 
> yes,  but why do you require it now as part of the vcpu topology change?
> 
> As-far-as-i-can-see, PPTT table changes(part of patch 5/9) do not require
> this change. Change in Patch 5/9 has also been done in anticipation of
> some future requirement(vcpu Hotplug?).
> 
> Please correct me here if I am wrong?
>

Hi Salil,

The problem is that we've never required smp.cpus == smp.maxcpus, so
a user could have smp.cpus < smp.maxcpus. We want the topology to match
maxcpus, but only enable cpus. However, if you think we should just not
allow cpus < maxcpus until hot plug is sorted out, then we could discuss
a way of trying to enforce cpus == maxcpus, but I'm not sure how we can
without breaking existing command lines.

Thanks,
drew

Salil Mehta May 18, 2021, 6:50 p.m. UTC | #6

> From: Andrew Jones [mailto:[e-mail]]
> Sent: Tuesday, May 18, 2021 8:50 AM
> 
> On Tue, May 18, 2021 at 07:04:51AM +0000, Salil Mehta wrote:
> > > From: wangyanan (Y)
> > > Sent: Tuesday, May 18, 2021 5:43 AM
> > >
> > > Hi Salil,
> > >
> > > On 2021/5/18 4:48, Salil Mehta wrote:
> > > >> From: Qemu-arm
> > > [mailto:qemu-arm-bounces+salil.mehta=[e-mail]]
> > > >> On Behalf Of Yanan Wang
> > > >> Sent: Sunday, May 16, 2021 11:29 AM
> > > >> To: Peter Maydell <[e-mail]>; Andrew Jones
> > > >> <[e-mail]>; Michael S . Tsirkin <[e-mail]>; Igor Mammedov
> > > >> <[e-mail]>; Shannon Zhao <[e-mail]>; Alistair
> > > >> Francis <[e-mail]>; David Gibson
> > > >> <[e-mail]>; [e-mail];
> [e-mail]
> > > >> Cc: Song Bao Hua (Barry Song) <[e-mail]>; zhukeqian
> > > >> <[e-mail]>; yangyicong <[e-mail]>; Zengtao (B)
> > > >> <[e-mail]>; Wanghaibin (D)
> <[e-mail]>;
> > > >> yuzenghui <[e-mail]>; Paolo Bonzini <[e-mail]>;
> > > >> Philippe Mathieu-Daudé <[e-mail]>
> > > >> Subject: [RFC PATCH v3 4/9] hw/arm/virt: Initialize the present cpu members
> > > >>
> > > >> We create and initialize a cpuobj for each present cpu in
> > > >> machvirt_init(). Now we also initialize the cpu member of
> > > >> structure CPUArchId for each present cpu in the function.
> > > > [...]
> > > >
> > > >>           qdev_realize(DEVICE(cpuobj), NULL, &error_fatal);
> > > >> +
> > > >> +        /*
> > > >> +         * As ARM cpu hotplug is not supported yet, we initialize
> > > >> +         * the present cpu members here.
> > > >> +         */
> > > >> +        machine->possible_cpus->cpus[n].cpu = cpuobj;
> > > >
> > > > when vcpu Hotplug is not supported yet, what necessitates this change now?
> > > >
> > > The initialization will gives a way to determine whether a CPU is
> > > present or not.
> > > At least, for now it will be used when generating ACPI tables, e.g.
> > > DSDT, MADT.
> > > See patch 5 and 6.
> >
> > yes,  but why do you require it now as part of the vcpu topology change?
> >
> > As-far-as-i-can-see, PPTT table changes(part of patch 5/9) do not require
> > this change. Change in Patch 5/9 has also been done in anticipation of
> > some future requirement(vcpu Hotplug?).
> >
> > Please correct me here if I am wrong?
> >
> 
> Hi Salil,
> 
> The problem is that we've never required smp.cpus == smp.maxcpus, so
> a user could have smp.cpus < smp.maxcpus. We want the topology to match
> maxcpus, but only enable cpus. However, if you think we should just not
> allow cpus < maxcpus until hot plug is sorted out, then we could discuss
> a way of trying to enforce cpus == maxcpus, but I'm not sure how we can
> without breaking existing command lines.


Hi Andrew,
Ideally, if the vcpu Hotplug is not supported the check in the smp_parse()
should impose (cpus == maxcpus). This as of now is just a warning of invalid
configuration I think. Beside this does not breaks any prior usages which you
suggested might happen?

Again, this is not a blocking issue from my side but just a humble suggestion.
You might want to take a call on this :)


Thanks
Salil.

Patch

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index e5dcdebdbc..50e324975f 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2061,6 +2061,13 @@  static void machvirt_init(MachineState *machine)
         }
 
         qdev_realize(DEVICE(cpuobj), NULL, &error_fatal);
+
+        /*
+         * As ARM cpu hotplug is not supported yet, we initialize
+         * the present cpu members here.
+         */
+        machine->possible_cpus->cpus[n].cpu = cpuobj;
+
         object_unref(cpuobj);
     }
     fdt_add_timer_nodes(vms);