| Message ID | 20210225085627.2263-5-fangying1@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/arm/virt: Introduce cpu topology support | expand |
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 */' > + 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. > + 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 */ > + > #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 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. [*] 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 > >
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 */ >> + >> #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. > > [*] 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.
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 */ > > > + > > > #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 > > > > > [*] 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. >
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? > > > > + > > > > #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. > > > > > > > > [*] 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. > >
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. >>> > > . >
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); + 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 */ + 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 */ + #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); + void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f, const char *oem_id, const char *oem_table_id);