qemu-arm
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [RFC PATCH 4/5] hw/acpi/aml-build: add processor hierarchy node stru

From: Ying Fang
Subject: Re: [RFC PATCH 4/5] hw/acpi/aml-build: add processor hierarchy node structure
Date: Thu, 4 Mar 2021 15:09:41 +0800
User-agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 


On 3/1/2021 11:50 PM, Michael S. Tsirkin wrote:

On Mon, Mar 01, 2021 at 10:39:19AM +0100, Andrew Jones wrote:

On Fri, Feb 26, 2021 at 10:23:03AM +0800, Ying Fang wrote:



On 2/25/2021 7:47 PM, Andrew Jones wrote:

On Thu, Feb 25, 2021 at 04:56:26PM +0800, Ying Fang wrote:

Add the processor hierarchy node structures to build ACPI information
for CPU topology. Since the private resources may be used to describe
cache hierarchy and it is variable among different topology level,
three helpers are introduced to describe the hierarchy.

(1) build_socket_hierarchy for socket description
(2) build_processor_hierarchy for processor description
(3) build_smt_hierarchy for thread (logic processor) description

Signed-off-by: Ying Fang <fangying1@huawei.com>
Signed-off-by: Henglong Fan <fanhenglong@huawei.com>
---
   hw/acpi/aml-build.c         | 40 +++++++++++++++++++++++++++++++++++++
   include/hw/acpi/acpi-defs.h | 13 ++++++++++++
   include/hw/acpi/aml-build.h |  7 +++++++
   3 files changed, 60 insertions(+)

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index a2cd7a5830..a0af3e9d73 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -1888,6 +1888,46 @@ void build_slit(GArray *table_data, BIOSLinker *linker, 
MachineState *ms,
                    table_data->len - slit_start, 1, oem_id, oem_table_id);
   }
+/*
+ * ACPI 6.3: 5.2.29.1 Processor hierarchy node structure (Type 0)
+ */
+void build_socket_hierarchy(GArray *tbl, uint32_t parent, uint32_t id)
+{
+    build_append_byte(tbl, ACPI_PPTT_TYPE_PROCESSOR); /* Type 0 - processor */
+    build_append_byte(tbl, 20);         /* Length, no private resources */
+    build_append_int_noprefix(tbl, 0, 2);  /* Reserved */
+    build_append_int_noprefix(tbl, ACPI_PPTT_PHYSICAL_PACKAGE, 4);


Missing '/* Flags */'


Will fix.




+    build_append_int_noprefix(tbl, parent, 4); /* Parent */
+    build_append_int_noprefix(tbl, id, 4);     /* ACPI processor ID */
+    build_append_int_noprefix(tbl, 0, 4);  /* Number of private resources */
+}
+
+void build_processor_hierarchy(GArray *tbl, uint32_t flags,
+                               uint32_t parent, uint32_t id)
+{
+    build_append_byte(tbl, ACPI_PPTT_TYPE_PROCESSOR);  /* Type 0 - processor */
+    build_append_byte(tbl, 20);         /* Length, no private resources */
+    build_append_int_noprefix(tbl, 0, 2);      /* Reserved */
+    build_append_int_noprefix(tbl, flags, 4);  /* Flags */
+    build_append_int_noprefix(tbl, parent, 4); /* Parent */
+    build_append_int_noprefix(tbl, id, 4);     /* ACPI processor ID */
+    build_append_int_noprefix(tbl, 0, 4);  /* Number of private resources */
+}
+
+void build_thread_hierarchy(GArray *tbl, uint32_t parent, uint32_t id)
+{
+    build_append_byte(tbl, ACPI_PPTT_TYPE_PROCESSOR); /* Type 0 - processor */
+    build_append_byte(tbl, 20);           /* Length, no private resources */
+    build_append_int_noprefix(tbl, 0, 2); /* Reserved */
+    build_append_int_noprefix(tbl,
+                              ACPI_PPTT_ACPI_PROCESSOR_ID_VALID |
+                              ACPI_PPTT_ACPI_PROCESSOR_IS_THREAD |
+                              ACPI_PPTT_ACPI_LEAF_NODE, 4);  /* Flags */
+    build_append_int_noprefix(tbl, parent , 4); /* parent */


'parent' not capitalized. We want these comments to exactly match the text
in the spec.


Will fix.




+    build_append_int_noprefix(tbl, id, 4);      /* ACPI processor ID */
+    build_append_int_noprefix(tbl, 0, 4);       /* Num of private resources */
+}
+
   /* build rev1/rev3/rev5.1 FADT */
   void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
                   const char *oem_id, const char *oem_table_id)
diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
index cf9f44299c..45e10d886f 100644
--- a/include/hw/acpi/acpi-defs.h
+++ b/include/hw/acpi/acpi-defs.h
@@ -618,4 +618,17 @@ struct AcpiIortRC {
   } QEMU_PACKED;
   typedef struct AcpiIortRC AcpiIortRC;
+enum {
+    ACPI_PPTT_TYPE_PROCESSOR = 0,
+    ACPI_PPTT_TYPE_CACHE,
+    ACPI_PPTT_TYPE_ID,
+    ACPI_PPTT_TYPE_RESERVED
+};
+
+#define ACPI_PPTT_PHYSICAL_PACKAGE          (1)
+#define ACPI_PPTT_ACPI_PROCESSOR_ID_VALID   (1 << 1)
+#define ACPI_PPTT_ACPI_PROCESSOR_IS_THREAD  (1 << 2)      /* ACPI 6.3 */
+#define ACPI_PPTT_ACPI_LEAF_NODE            (1 << 3)      /* ACPI 6.3 */
+#define ACPI_PPTT_ACPI_IDENTICAL            (1 << 4)      /* ACPI 6.3 */


You need to quote specific place in spec where this appeared, not
just version. and what about previous ones?


Thanks, Will fix.





+
   #endif
diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index 380d3e3924..7f0ca1a198 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -462,6 +462,13 @@ void build_srat_memory(AcpiSratMemoryAffinity *numamem, 
uint64_t base,
   void build_slit(GArray *table_data, BIOSLinker *linker, MachineState *ms,
                   const char *oem_id, const char *oem_table_id);
+void build_socket_hierarchy(GArray *tbl, uint32_t parent, uint32_t id);
+
+void build_processor_hierarchy(GArray *tbl, uint32_t flags,
+                               uint32_t parent, uint32_t id);
+
+void build_thread_hierarchy(GArray *tbl, uint32_t parent, uint32_t id);


Why does build_processor_hierarchy() take a flags argument, but the
others don't? Why not just have a single 'flags' taking function,
like [*] that works for all of them? I think that answer to that is


Yes, you are right.


that when cache topology support is added it's better to break these
into separate functions, but should we do that now? It seems odd to
be introducing unused defines and this API before it's necessary.

So it is better for us to keep just one common build_processor_hierarchy
API here in your opinion.


Well, a consistent API without unused defines. Whether or not that's
a single common function or not isn't that important.

Thanks,
drew


Yes, the preferred way is code comments:
E.g.

     build_append_byte(tbl, ACPI_PPTT_TYPE_PROCESSOR);  /* Type 0 - processor */

should be

     build_append_byte(tbl, 0);  /* Type 0 - processor */


similar:


+    build_append_int_noprefix(tbl,
+                              ACPI_PPTT_ACPI_PROCESSOR_ID_VALID |
+                              ACPI_PPTT_ACPI_PROCESSOR_IS_THREAD |
+                              ACPI_PPTT_ACPI_LEAF_NODE, 4);  /* Flags */


should be

  +    build_append_int_noprefix(tbl, /* Processor Structure Flags */
  +                              (1 << 1)  /* ACPI Processor ID valid */|
  +                              (1 << 2) /* Processor is a Thread */) |
  +                              (1 << 3) /* Node is a Leaf */, 4);

where you would make sure the text matches the spec verbatim.

also note how for multi-line code comments precede the code.
For single-line they can come after the code.


Thanks, will fix it as your suggestions.







[*] 
https://github.com/rhdrjones/qemu/commit/439b38d67ca1f2cbfa5b9892a822b651ebd05c11

Thanks,
drew


+
   void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
                   const char *oem_id, const char *oem_table_id);
--
2.23.0




.



Thanks,
Ying.



.
reply via email to
[Prev in Thread] Current Thread [Next in Thread]