qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: "wangyanan (Y)" <wangyanan55@huawei.com>
To: Andrew Jones <drjones@redhat.com>
Cc: Peter Maydell <peter.maydell@linaro.org>,
	"Michael S . Tsirkin" <mst@redhat.com>,
	wanghaibin.wang@huawei.com, qemu-devel@nongnu.org,
	Shannon Zhao <shannon.zhaosl@gmail.com>,
	qemu-arm@nongnu.org, Alistair Francis <alistair.francis@wdc.com>,
	prime.zeng@hisilicon.com, yangyicong@huawei.com,
	yuzenghui@huawei.com, Igor Mammedov <imammedo@redhat.com>,
	zhukeqian1@huawei.com, Jiajie Li <lijiajie11@huawei.com>,
	David Gibson <david@gibson.dropbear.id.au>
Subject: Re: [RFC PATCH v2 5/6] hw/arm/virt-acpi-build: Add PPTT table
Date: Thu, 13 May 2021 13:10:10 +0800	[thread overview]
Message-ID: <1551b7d6-e010-e5c7-47e1-c347ca78a1db@huawei.com> (raw)
In-Reply-To: <20210413080745.33004-6-wangyanan55@huawei.com>

Hi Drew,

I got a question below, and hope your reply. Thanks!
On 2021/4/13 16:07, Yanan Wang wrote:
> Add the Processor Properties Topology Table (PPTT) to present
> CPU topology information to ACPI guests. Note, while a DT boot
> Linux guest with a non-flat CPU topology will see socket and
> core IDs being sequential integers starting from zero, e.g.
> with -smp 4,sockets=2,cores=2,threads=1
>
> a DT boot produces
>
>   cpu:  0 package_id:  0 core_id:  0
>   cpu:  1 package_id:  0 core_id:  1
>   cpu:  2 package_id:  1 core_id:  0
>   cpu:  3 package_id:  1 core_id:  1
>
> an ACPI boot produces
>
>   cpu:  0 package_id: 36 core_id:  0
>   cpu:  1 package_id: 36 core_id:  1
>   cpu:  2 package_id: 96 core_id:  2
>   cpu:  3 package_id: 96 core_id:  3
>
> This is due to several reasons:
>
>   1) DT cpu nodes do not have an equivalent field to what the PPTT
>      ACPI Processor ID must be, i.e. something equal to the MADT CPU
>      UID or equal to the UID of an ACPI processor container. In both
>      ACPI cases those are platform dependant IDs assigned by the
>      vendor.
>
>   2) While QEMU is the vendor for a guest, if the topology specifies
>      SMT (> 1 thread), then, with ACPI, it is impossible to assign a
>      core-id the same value as a package-id, thus it is not possible
>      to have package-id=0 and core-id=0. This is because package and
>      core containers must be in the same ACPI namespace and therefore
>      must have unique UIDs.
>
>   3) ACPI processor containers are not required for PPTT tables to
>      be used and, due to the limitations of which IDs are selected
>      described above in (2), they are not helpful for QEMU, so we
>      don't build them with this patch. In the absence of them, Linux
>      assigns its own unique IDs. The maintainers have chosen not to use
>      counters from zero, but rather ACPI table offsets, which explains
>      why the numbers are so much larger than with DT.
>
>   4) When there is no SMT (threads=1) the core IDs for ACPI boot guests
>      match the logical CPU IDs, because these IDs must be equal to the
>      MADT CPU UID (as no processor containers are present), and QEMU
>      uses the logical CPU ID for these MADT IDs.
>
> Tested-by: Jiajie Li <lijiajie11@huawei.com>
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Ying Fang <fangying1@huawei.com>
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
>   hw/arm/virt-acpi-build.c | 63 ++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 63 insertions(+)
>
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 2ad5dad1bf..03fd812d5a 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -436,6 +436,64 @@ build_srat(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>                    vms->oem_table_id);
>   }
>   
> +/* PPTT */
> +static void
> +build_pptt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> +{
> +    int pptt_start = table_data->len;
> +    int uid = 0, cpus = 0, socket = 0;
> +    MachineState *ms = MACHINE(vms);
> +    unsigned int smp_cores = ms->smp.cores;
> +    unsigned int smp_threads = ms->smp.threads;
> +
> +    acpi_data_push(table_data, sizeof(AcpiTableHeader));
> +
> +    for (socket = 0; cpus < ms->possible_cpus->len; socket++) {
> +        uint32_t socket_offset = table_data->len - pptt_start;
> +        int core;
> +
> +        build_processor_hierarchy_node(
> +            table_data, 1, /* Physical package */
> +            0, socket, /* No parent */
> +            NULL, 0);  /* No private resources */
> +
> +        for (core = 0; core < smp_cores; core++) {
> +            uint32_t core_offset = table_data->len - pptt_start;
> +            int thread;
> +
> +            if (smp_threads <= 1) {
> +                build_processor_hierarchy_node(
> +                    table_data,
> +                    (1 << 1) | /* ACPI Processor ID valid */
> +                    (1 << 3),  /* ACPI 6.3 - Node is a Leaf */
> +                    socket_offset, uid++, /* Parent is a Socket */
> +                    NULL, 0);  /* No private resources */
> +            } else {
> +                build_processor_hierarchy_node(
> +                    table_data, 0,
> +                    socket_offset, core, /* Parent is a Socket */
> +                    NULL, 0); /* No private resources */
> +
> +                for (thread = 0; thread < smp_threads; thread++) {
> +                    build_processor_hierarchy_node(
> +                        table_data,
> +                        (1 << 1) | /* ACPI Processor ID valid */
> +                        (1 << 2) | /* ACPI 6.3 - Processor is a Thread */
> +                        (1 << 3),  /* ACPI 6.3 - Node is a Leaf */
> +                        core_offset, uid++, /* Parent is a Core */
> +                        NULL, 0);  /* No private resources */
> +                }
> +            }
> +        }
> +        cpus += smp_cores * smp_threads;
> +    }
> +
> +    build_header(linker, table_data,
> +                 (void *)(table_data->data + pptt_start), "PPTT",
> +                 table_data->len - pptt_start, 2,
> +                 vms->oem_id, vms->oem_table_id);
> +}
> +
>   /* GTDT */
>   static void
>   build_gtdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> @@ -707,6 +765,11 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
>       acpi_add_table(table_offsets, tables_blob);
>       build_madt(tables_blob, tables->linker, vms);
>   
> +    if (ms->smp.cpus > 1 && !vmc->no_cpu_topology) {
I'm not really sure why we need to care about "ms->smp.cpus > 1" here?

IMO, just like MADT in which we create both ENABLED and DISABLED
gicc nodes no matter of number of ENABLED nodes is one or not, we
should create PPTT table for all the possible cpus and not care about
number of smp cpus, too. This will be more consistent with the ACPI
specification and the PPTT table will be used for ACPI cpu hotplug in
the future even with  "smp.cpus == 1".

Care of "smp.cpus > 1" in the DT cpu-map part makes sense to me,
because we are required to only add present cpu nodes to the DT and
Linux Doc says that a cpu-map is not needed for uniprocessor systems.

Thanks,
Yanan
> +        acpi_add_table(table_offsets, tables_blob);
> +        build_pptt(tables_blob, tables->linker, vms);
> +    }
> +
>       acpi_add_table(table_offsets, tables_blob);
>       build_gtdt(tables_blob, tables->linker, vms);
>   


  parent reply	other threads:[~2021-05-13  5:11 UTC|newest]

Thread overview: 48+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-13  8:07 [RFC PATCH v2 0/6] hw/arm/virt: Introduce cpu topology support Yanan Wang
2021-04-13  8:07 ` [RFC PATCH v2 1/6] device_tree: Add qemu_fdt_add_path Yanan Wang
2021-04-16  4:52   ` David Gibson
2021-04-17  2:36     ` wangyanan (Y)
2021-04-19  1:13       ` David Gibson
2021-04-19  7:02         ` wangyanan (Y)
2021-04-13  8:07 ` [RFC PATCH v2 2/6] hw/arm/virt: DT: Add cpu-map Yanan Wang
2021-04-27  9:47   ` Philippe Mathieu-Daudé
2021-04-27 10:04     ` Andrew Jones
2021-04-27 12:36       ` Philippe Mathieu-Daudé
2021-04-28  6:36         ` wangyanan (Y)
2021-05-13  6:58   ` Andrew Jones
2021-05-13  7:15     ` wangyanan (Y)
2021-04-13  8:07 ` [RFC PATCH v2 3/6] hw/arm/virt-acpi-build: Distinguish possible and present cpus Yanan Wang
2021-04-27 13:18   ` Andrew Jones
2021-04-28  6:42     ` wangyanan (Y)
2021-04-27 14:50   ` Andrew Jones
2021-04-28  6:47     ` wangyanan (Y)
2021-04-13  8:07 ` [RFC PATCH v2 4/6] hw/acpi/aml-build: Add processor hierarchy node structure Yanan Wang
2021-04-27 13:37   ` Andrew Jones
2021-04-28  6:59     ` wangyanan (Y)
2021-04-13  8:07 ` [RFC PATCH v2 5/6] hw/arm/virt-acpi-build: Add PPTT table Yanan Wang
2021-04-27 14:16   ` Andrew Jones
2021-04-28  7:30     ` wangyanan (Y)
2021-05-13  5:10   ` wangyanan (Y) [this message]
2021-05-13  6:55     ` Andrew Jones
2021-05-18  7:17     ` Salil Mehta
2021-05-18  7:42       ` Andrew Jones
2021-05-18 18:34         ` Salil Mehta
2021-05-18 19:05           ` Andrew Jones
2021-05-18 19:22             ` Salil Mehta
2021-05-19  3:18               ` wangyanan (Y)
2021-05-19  7:54                 ` Salil Mehta
2021-05-19  8:15                   ` Andrew Jones
2021-05-19  8:42                     ` wangyanan (Y)
2021-05-19 10:00                     ` Salil Mehta
2021-05-19  8:27             ` Andrew Jones
2021-05-19 13:26               ` wangyanan (Y)
2021-05-19 13:40                 ` wangyanan (Y)
2021-05-18  9:16       ` wangyanan (Y)
2021-04-13  8:07 ` [RFC PATCH v2 6/6] hw/arm/virt: Replace smp_parse with one that prefers cores Yanan Wang
2021-04-27 14:58   ` Andrew Jones
2021-04-28  8:04     ` wangyanan (Y)
2021-04-28  9:36     ` wangyanan (Y)
2021-04-28 10:13       ` Andrew Jones
2021-04-29  2:21         ` wangyanan (Y)
2021-04-21  7:40 ` [RFC PATCH v2 0/6] hw/arm/virt: Introduce cpu topology support wangyanan (Y)
2021-04-21  9:31 ` wangyanan (Y)

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1551b7d6-e010-e5c7-47e1-c347ca78a1db@huawei.com \
    --to=wangyanan55@huawei.com \
    --cc=alistair.francis@wdc.com \
    --cc=david@gibson.dropbear.id.au \
    --cc=drjones@redhat.com \
    --cc=imammedo@redhat.com \
    --cc=lijiajie11@huawei.com \
    --cc=mst@redhat.com \
    --cc=peter.maydell@linaro.org \
    --cc=prime.zeng@hisilicon.com \
    --cc=qemu-arm@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    --cc=shannon.zhaosl@gmail.com \
    --cc=wanghaibin.wang@huawei.com \
    --cc=yangyicong@huawei.com \
    --cc=yuzenghui@huawei.com \
    --cc=zhukeqian1@huawei.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).