[RFC 12/52] hw/acpi: Replace MachineState.smp access with topology helpers

Zhao Liu posted 52 patches 2 years, 12 months ago
[RFC 12/52] hw/acpi: Replace MachineState.smp access with topology helpers
Posted by Zhao Liu 2 years, 12 months ago
From: Zhao Liu <zhao1.liu@intel.com>

At present, in QEMU only arm needs PPTT table to build cpu topology.

Before QEMU's arm supports hybrid architectures, it's enough to limit
the cpu topology of PPTT to smp type through the explicit smp interface
(machine_topo_get_smp_threads()).

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Ani Sinha <ani@anisinha.ca>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 hw/acpi/aml-build.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index ea331a20d131..693bd8833d10 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -2044,7 +2044,7 @@ void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState *ms,
             cluster_offset = socket_offset;
         }
 
-        if (ms->smp.threads == 1) {
+        if (machine_topo_get_smp_threads(ms) == 1) {
             build_processor_hierarchy_node(table_data,
                 (1 << 1) | /* ACPI Processor ID valid */
                 (1 << 3),  /* Node is a Leaf */
-- 
2.34.1
Re: [RFC 12/52] hw/acpi: Replace MachineState.smp access with topology helpers
Posted by wangyanan (Y) via 2 years, 11 months ago
Hi Zhao,

在 2023/2/13 17:49, Zhao Liu 写道:
> From: Zhao Liu <zhao1.liu@intel.com>
>
> At present, in QEMU only arm needs PPTT table to build cpu topology.
>
> Before QEMU's arm supports hybrid architectures, it's enough to limit
> the cpu topology of PPTT to smp type through the explicit smp interface
> (machine_topo_get_smp_threads()).
>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Ani Sinha <ani@anisinha.ca>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
>   hw/acpi/aml-build.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index ea331a20d131..693bd8833d10 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -2044,7 +2044,7 @@ void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState *ms,
>               cluster_offset = socket_offset;
>           }
>   
> -        if (ms->smp.threads == 1) {
> +        if (machine_topo_get_smp_threads(ms) == 1) {
>               build_processor_hierarchy_node(table_data,
>                   (1 << 1) | /* ACPI Processor ID valid */
>                   (1 << 3),  /* Node is a Leaf */
ACPI PPTT table is designed to also support the hybrid CPU topology
case where nodes on the same CPU topology level can have different
number of child nodes.

So to be general, the diff should be:
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index ea331a20d1..dfded95bbc 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -2044,7 +2044,7 @@ void build_pptt(GArray *table_data, BIOSLinker 
*linker, MachineState *ms,
              cluster_offset = socket_offset;
          }

-        if (ms->smp.threads == 1) {
+        if (machine_topo_get_threads_by_idx(n) == 1) {
              build_processor_hierarchy_node(table_data,
                  (1 << 1) | /* ACPI Processor ID valid */
                  (1 << 3),  /* Node is a Leaf */

Actually I'm recently working on ARM hmp virtualization which relys on
PPTT for topology representation, so we will also need PPTT to be general
for hybrid case anyway.

Thanks,
Yanan

Re: [RFC 12/52] hw/acpi: Replace MachineState.smp access with topology helpers
Posted by Zhao Liu 2 years, 11 months ago
On Thu, Feb 16, 2023 at 05:31:11PM +0800, wangyanan (Y) wrote:
> Date: Thu, 16 Feb 2023 17:31:11 +0800
> From: "wangyanan (Y)" <wangyanan55@huawei.com>
> Subject: Re: [RFC 12/52] hw/acpi: Replace MachineState.smp access with
>  topology helpers
> 
> Hi Zhao,
> 
> 在 2023/2/13 17:49, Zhao Liu 写道:
> > From: Zhao Liu <zhao1.liu@intel.com>
> > 
> > At present, in QEMU only arm needs PPTT table to build cpu topology.
> > 
> > Before QEMU's arm supports hybrid architectures, it's enough to limit
> > the cpu topology of PPTT to smp type through the explicit smp interface
> > (machine_topo_get_smp_threads()).
> > 
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: Igor Mammedov <imammedo@redhat.com>
> > Cc: Ani Sinha <ani@anisinha.ca>
> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> > ---
> >   hw/acpi/aml-build.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> > index ea331a20d131..693bd8833d10 100644
> > --- a/hw/acpi/aml-build.c
> > +++ b/hw/acpi/aml-build.c
> > @@ -2044,7 +2044,7 @@ void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState *ms,
> >               cluster_offset = socket_offset;
> >           }
> > -        if (ms->smp.threads == 1) {
> > +        if (machine_topo_get_smp_threads(ms) == 1) {
> >               build_processor_hierarchy_node(table_data,
> >                   (1 << 1) | /* ACPI Processor ID valid */
> >                   (1 << 3),  /* Node is a Leaf */
> ACPI PPTT table is designed to also support the hybrid CPU topology
> case where nodes on the same CPU topology level can have different
> number of child nodes.
> 
> So to be general, the diff should be:
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index ea331a20d1..dfded95bbc 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -2044,7 +2044,7 @@ void build_pptt(GArray *table_data, BIOSLinker
> *linker, MachineState *ms,
>              cluster_offset = socket_offset;
>          }
> 
> -        if (ms->smp.threads == 1) {
> +        if (machine_topo_get_threads_by_idx(n) == 1) {
>              build_processor_hierarchy_node(table_data,
>                  (1 << 1) | /* ACPI Processor ID valid */
>                  (1 << 3),  /* Node is a Leaf */

Nice! I'll replace that.

> 
> Actually I'm recently working on ARM hmp virtualization which relys on
> PPTT for topology representation, so we will also need PPTT to be general
> for hybrid case anyway.

Good to know that you are considering hybrid support for arm.
BTW, I explained the difference between arm and x86's hybrid in previous
email [1] [2], mainly about whether the cpm model is the same.

I tentatively think that this difference can be solved by arch-specific
coretype(). Do you have any comments on this? Thanks!

[1]: https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg03884.html
[2]: https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg03789.html

> 
> Thanks,
> Yanan

Re: [RFC 12/52] hw/acpi: Replace MachineState.smp access with topology helpers
Posted by wangyanan (Y) via 2 years, 11 months ago
在 2023/2/17 11:14, Zhao Liu 写道:
> On Thu, Feb 16, 2023 at 05:31:11PM +0800, wangyanan (Y) wrote:
>> Date: Thu, 16 Feb 2023 17:31:11 +0800
>> From: "wangyanan (Y)" <wangyanan55@huawei.com>
>> Subject: Re: [RFC 12/52] hw/acpi: Replace MachineState.smp access with
>>   topology helpers
>>
>> Hi Zhao,
>>
>> 在 2023/2/13 17:49, Zhao Liu 写道:
>>> From: Zhao Liu <zhao1.liu@intel.com>
>>>
>>> At present, in QEMU only arm needs PPTT table to build cpu topology.
>>>
>>> Before QEMU's arm supports hybrid architectures, it's enough to limit
>>> the cpu topology of PPTT to smp type through the explicit smp interface
>>> (machine_topo_get_smp_threads()).
>>>
>>> Cc: Michael S. Tsirkin <mst@redhat.com>
>>> Cc: Igor Mammedov <imammedo@redhat.com>
>>> Cc: Ani Sinha <ani@anisinha.ca>
>>> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
>>> ---
>>>    hw/acpi/aml-build.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>>> index ea331a20d131..693bd8833d10 100644
>>> --- a/hw/acpi/aml-build.c
>>> +++ b/hw/acpi/aml-build.c
>>> @@ -2044,7 +2044,7 @@ void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState *ms,
>>>                cluster_offset = socket_offset;
>>>            }
>>> -        if (ms->smp.threads == 1) {
>>> +        if (machine_topo_get_smp_threads(ms) == 1) {
>>>                build_processor_hierarchy_node(table_data,
>>>                    (1 << 1) | /* ACPI Processor ID valid */
>>>                    (1 << 3),  /* Node is a Leaf */
>> ACPI PPTT table is designed to also support the hybrid CPU topology
>> case where nodes on the same CPU topology level can have different
>> number of child nodes.
>>
>> So to be general, the diff should be:
>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>> index ea331a20d1..dfded95bbc 100644
>> --- a/hw/acpi/aml-build.c
>> +++ b/hw/acpi/aml-build.c
>> @@ -2044,7 +2044,7 @@ void build_pptt(GArray *table_data, BIOSLinker
>> *linker, MachineState *ms,
>>               cluster_offset = socket_offset;
>>           }
>>
>> -        if (ms->smp.threads == 1) {
>> +        if (machine_topo_get_threads_by_idx(n) == 1) {
>>               build_processor_hierarchy_node(table_data,
>>                   (1 << 1) | /* ACPI Processor ID valid */
>>                   (1 << 3),  /* Node is a Leaf */
> Nice! I'll replace that.
>
>> Actually I'm recently working on ARM hmp virtualization which relys on
>> PPTT for topology representation, so we will also need PPTT to be general
>> for hybrid case anyway.
> Good to know that you are considering hybrid support for arm.
> BTW, I explained the difference between arm and x86's hybrid in previous
> email [1] [2], mainly about whether the cpm model is the same.
>
> I tentatively think that this difference can be solved by arch-specific
> coretype(). Do you have any comments on this? Thanks!
Will look at that. Thanks.
>
> [1]: https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg03884.html
> [2]: https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg03789.html
>
>> Thanks,
>> Yanan