| Message ID | 20211014132206.19220-6-wangyanan55@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/arm/virt: Introduce cpu topology support | expand |
Hi, On 10/14/21 3:22 PM, Yanan Wang wrote: > From: Andrew Jones <drjones@redhat.com> > > Add the Processor Properties Topology Table (PPTT) used to > describe CPU topology information to ACPI guests. > > Note, a DT-boot Linux guest with a non-flat CPU topology will > see socket and core IDs being sequential integers starting > from zero, which is different from ACPI-boot Linux guest, > 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 mandatorily 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. > > So in summary, with QEMU as the vendor for the guests, we simply > use sequential integers starting from zero for the non-leaf nodes > but with ID-valid flag unset, so that guest will ignore them and > use table offsets as unique container IDs. And we use logical CPU > IDs for the leaf nodes with the ID-valid flag set, which will be > consistent with MADT. > > Signed-off-by: Andrew Jones <drjones@redhat.com> > Co-developed-by: Yanan Wang <wangyanan55@huawei.com> > Signed-off-by: Yanan Wang <wangyanan55@huawei.com> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > --- > hw/acpi/aml-build.c | 60 +++++++++++++++++++++++++++++++++++++ > include/hw/acpi/aml-build.h | 3 ++ > 2 files changed, 63 insertions(+) > > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c > index b7b9db6888..0d50e88e9d 100644 > --- a/hw/acpi/aml-build.c > +++ b/hw/acpi/aml-build.c > @@ -1990,6 +1990,66 @@ void build_processor_hierarchy_node(GArray *tbl, uint32_t flags, > } > } > > +/* ACPI 6.2: 5.2.29 Processor Properties Topology Table (PPTT) */ > +void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState *ms, > + const char *oem_id, const char *oem_table_id) > +{ > + int pptt_start = table_data->len; > + int uid = 0; > + int socket; > + AcpiTable table = { .sig = "PPTT", .rev = 2, > + .oem_id = oem_id, .oem_table_id = oem_table_id }; Table 5-149 of 6.2 spec (6.2 May 2017) tells the rev shall be 1. Or is it an erratum somewhere I did miss? I would also add the spec version in the commit msg. > + > + acpi_table_begin(&table, table_data); > + > + for (socket = 0; socket < ms->smp.sockets; socket++) { > + uint32_t socket_offset = table_data->len - pptt_start; > + int core; > + > + build_processor_hierarchy_node( > + table_data, > + /* > + * ACPI 6.2 - Physical package > + * represents the boundary of a physical package > + */ > + (1 << 0), > + 0, socket, NULL, 0); I see we set an ACPI process ID but in the meantime the ACPI processor ID valid flag is not set. I am not sure I fully catch the meaning of this latter but just to double check if this is done on purpose. Maybe wort a general comment as this also happens below. > + > + for (core = 0; core < ms->smp.cores; core++) { > + uint32_t core_offset = table_data->len - pptt_start; > + int thread; > + > + if (ms->smp.threads > 1) { > + build_processor_hierarchy_node( > + table_data, > + /* > + * ACPI 6.2 - Physical package > + * doesn't represent the boundary of a physical package > + */ > + (0 << 0), would rather say (0 << 0) /* not a physical package */ and same elsewhere > + socket_offset, core, NULL, 0); > + > + for (thread = 0; thread < ms->smp.threads; thread++) { > + build_processor_hierarchy_node( > + table_data, > + (1 << 1) | /* ACPI 6.2 - ACPI Processor ID valid */ > + (1 << 2) | /* ACPI 6.3 - Processor is a Thread */ So the references look globaly confusing to me. Either it complies to 6.2 or to 6.3. Looks ir rather complies with 6.3. To me, this needs to be clarified. I would also add the reference it complies to in the commit msg. > + (1 << 3), /* ACPI 6.3 - Node is a Leaf */ > + core_offset, uid++, NULL, 0); > + } > + } else { > + build_processor_hierarchy_node( > + table_data, > + (1 << 1) | /* ACPI 6.2 - ACPI Processor ID valid */ > + (1 << 3), /* ACPI 6.3 - Node is a Leaf */ > + socket_offset, uid++, NULL, 0); > + } > + } > + } > + > + acpi_table_end(linker, &table); > +} > + > /* 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/aml-build.h b/include/hw/acpi/aml-build.h > index 2c457c8f17..b92706388c 100644 > --- a/include/hw/acpi/aml-build.h > +++ b/include/hw/acpi/aml-build.h > @@ -493,6 +493,9 @@ void build_processor_hierarchy_node(GArray *tbl, uint32_t flags, > uint32_t parent, uint32_t id, > uint32_t *priv_rsrc, uint32_t priv_num); > > +void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState *ms, > + const char *oem_id, const char *oem_table_id); > + > void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f, > const char *oem_id, const char *oem_table_id); > > Thanks Eric
Hi Eric, On 2021/10/20 16:02, Eric Auger wrote: > Hi, > > On 10/14/21 3:22 PM, Yanan Wang wrote: >> From: Andrew Jones <drjones@redhat.com> >> >> Add the Processor Properties Topology Table (PPTT) used to >> describe CPU topology information to ACPI guests. >> >> Note, a DT-boot Linux guest with a non-flat CPU topology will >> see socket and core IDs being sequential integers starting >> from zero, which is different from ACPI-boot Linux guest, >> 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 mandatorily 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. >> >> So in summary, with QEMU as the vendor for the guests, we simply >> use sequential integers starting from zero for the non-leaf nodes >> but with ID-valid flag unset, so that guest will ignore them and >> use table offsets as unique container IDs. And we use logical CPU >> IDs for the leaf nodes with the ID-valid flag set, which will be >> consistent with MADT. >> >> Signed-off-by: Andrew Jones <drjones@redhat.com> >> Co-developed-by: Yanan Wang <wangyanan55@huawei.com> >> Signed-off-by: Yanan Wang <wangyanan55@huawei.com> >> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> >> --- >> hw/acpi/aml-build.c | 60 +++++++++++++++++++++++++++++++++++++ >> include/hw/acpi/aml-build.h | 3 ++ >> 2 files changed, 63 insertions(+) >> >> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c >> index b7b9db6888..0d50e88e9d 100644 >> --- a/hw/acpi/aml-build.c >> +++ b/hw/acpi/aml-build.c >> @@ -1990,6 +1990,66 @@ void build_processor_hierarchy_node(GArray *tbl, uint32_t flags, >> } >> } >> >> +/* ACPI 6.2: 5.2.29 Processor Properties Topology Table (PPTT) */ >> +void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState *ms, >> + const char *oem_id, const char *oem_table_id) >> +{ >> + int pptt_start = table_data->len; >> + int uid = 0; >> + int socket; >> + AcpiTable table = { .sig = "PPTT", .rev = 2, >> + .oem_id = oem_id, .oem_table_id = oem_table_id }; > Table 5-149 of 6.2 spec (6.2 May 2017) tells the rev shall be 1. Or is > it an erratum somewhere I did miss? Yes, the revision in 6.2 spec is 1. And it's 2 in spec 6.3. So just to be sure, should I use the oldest revision ? > I would also add the spec version in the commit msg. >> + >> + acpi_table_begin(&table, table_data); >> + >> + for (socket = 0; socket < ms->smp.sockets; socket++) { >> + uint32_t socket_offset = table_data->len - pptt_start; >> + int core; >> + >> + build_processor_hierarchy_node( >> + table_data, >> + /* >> + * ACPI 6.2 - Physical package >> + * represents the boundary of a physical package >> + */ >> + (1 << 0), >> + 0, socket, NULL, 0); > I see we set an ACPI process ID but in the meantime the ACPI processor > ID valid flag is not set. I am not sure I fully catch the meaning of > this latter but just to double check if this is done on purpose. Yes, it's on purpose. > Maybe > wort a general comment as this also happens below. The ID of the container node is invalid and ID of the leaf node is valid. The commit message by Andrew has explained why (reason 3). I think it may be clear enough to explain there why we don't need a valid ID for the container node. >> + >> + for (core = 0; core < ms->smp.cores; core++) { >> + uint32_t core_offset = table_data->len - pptt_start; >> + int thread; >> + >> + if (ms->smp.threads > 1) { >> + build_processor_hierarchy_node( >> + table_data, >> + /* >> + * ACPI 6.2 - Physical package >> + * doesn't represent the boundary of a physical package >> + */ >> + (0 << 0), > would rather say (0 << 0) /* not a physical package */ and same elsewhere Ok, thanks. >> + socket_offset, core, NULL, 0); >> + >> + for (thread = 0; thread < ms->smp.threads; thread++) { >> + build_processor_hierarchy_node( >> + table_data, >> + (1 << 1) | /* ACPI 6.2 - ACPI Processor ID valid */ >> + (1 << 2) | /* ACPI 6.3 - Processor is a Thread */ > So the references look globaly confusing to me. Either it complies to > 6.2 or to 6.3. Looks ir rather complies with 6.3. To me, this needs to > be clarified. ACPI 6.2 in the comment means the flag is introduced in the spec since 6.2. The same, ACPI 6.3 means the flag is introduced since 6.3. Maybe I should just drop all the version-prefix in the comment ? > I would also add the reference it complies to in the > commit msg. Ok, sure. Thanks, Yanan . >> + (1 << 3), /* ACPI 6.3 - Node is a Leaf */ >> + core_offset, uid++, NULL, 0); >> + } >> + } else { >> + build_processor_hierarchy_node( >> + table_data, >> + (1 << 1) | /* ACPI 6.2 - ACPI Processor ID valid */ >> + (1 << 3), /* ACPI 6.3 - Node is a Leaf */ >> + socket_offset, uid++, NULL, 0); >> + } >> + } >> + } >> + >> + acpi_table_end(linker, &table); >> +} >> + >> /* 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/aml-build.h b/include/hw/acpi/aml-build.h >> index 2c457c8f17..b92706388c 100644 >> --- a/include/hw/acpi/aml-build.h >> +++ b/include/hw/acpi/aml-build.h >> @@ -493,6 +493,9 @@ void build_processor_hierarchy_node(GArray *tbl, uint32_t flags, >> uint32_t parent, uint32_t id, >> uint32_t *priv_rsrc, uint32_t priv_num); >> >> +void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState *ms, >> + const char *oem_id, const char *oem_table_id); >> + >> void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f, >> const char *oem_id, const char *oem_table_id); >> >> > Thanks > > Eric > > .
Hi Yanan, On 10/20/21 11:51 AM, wangyanan (Y) wrote: > Hi Eric, > > On 2021/10/20 16:02, Eric Auger wrote: >> Hi, >> >> On 10/14/21 3:22 PM, Yanan Wang wrote: >>> From: Andrew Jones <drjones@redhat.com> >>> >>> Add the Processor Properties Topology Table (PPTT) used to >>> describe CPU topology information to ACPI guests. >>> >>> Note, a DT-boot Linux guest with a non-flat CPU topology will >>> see socket and core IDs being sequential integers starting >>> from zero, which is different from ACPI-boot Linux guest, >>> 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 mandatorily 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. >>> >>> So in summary, with QEMU as the vendor for the guests, we simply >>> use sequential integers starting from zero for the non-leaf nodes >>> but with ID-valid flag unset, so that guest will ignore them and >>> use table offsets as unique container IDs. And we use logical CPU >>> IDs for the leaf nodes with the ID-valid flag set, which will be >>> consistent with MADT. >>> >>> Signed-off-by: Andrew Jones <drjones@redhat.com> >>> Co-developed-by: Yanan Wang <wangyanan55@huawei.com> >>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com> >>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> >>> --- >>> hw/acpi/aml-build.c | 60 +++++++++++++++++++++++++++++++++++++ >>> include/hw/acpi/aml-build.h | 3 ++ >>> 2 files changed, 63 insertions(+) >>> >>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c >>> index b7b9db6888..0d50e88e9d 100644 >>> --- a/hw/acpi/aml-build.c >>> +++ b/hw/acpi/aml-build.c >>> @@ -1990,6 +1990,66 @@ void build_processor_hierarchy_node(GArray >>> *tbl, uint32_t flags, >>> } >>> } >>> +/* ACPI 6.2: 5.2.29 Processor Properties Topology Table (PPTT) */ >>> +void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState >>> *ms, >>> + const char *oem_id, const char *oem_table_id) >>> +{ >>> + int pptt_start = table_data->len; >>> + int uid = 0; >>> + int socket; >>> + AcpiTable table = { .sig = "PPTT", .rev = 2, >>> + .oem_id = oem_id, .oem_table_id = >>> oem_table_id }; >> Table 5-149 of 6.2 spec (6.2 May 2017) tells the rev shall be 1. Or is >> it an erratum somewhere I did miss? > Yes, the revision in 6.2 spec is 1. And it's 2 in spec 6.3. > So just to be sure, should I use the oldest revision ? If you need (and use) features (such as flags) introduced in 6.3 then you should say the code complies with 6.3 and update the above comment. >> I would also add the spec version in the commit msg. >>> + >>> + acpi_table_begin(&table, table_data); >>> + >>> + for (socket = 0; socket < ms->smp.sockets; socket++) { >>> + uint32_t socket_offset = table_data->len - pptt_start; >>> + int core; >>> + >>> + build_processor_hierarchy_node( >>> + table_data, >>> + /* >>> + * ACPI 6.2 - Physical package >>> + * represents the boundary of a physical package >>> + */ >>> + (1 << 0), >>> + 0, socket, NULL, 0); >> I see we set an ACPI process ID but in the meantime the ACPI processor >> ID valid flag is not set. I am not sure I fully catch the meaning of >> this latter but just to double check if this is done on purpose. > Yes, it's on purpose. >> Maybe >> wort a general comment as this also happens below. > The ID of the container node is invalid and ID of the leaf node is valid. > The commit message by Andrew has explained why (reason 3). I think > it may be clear enough to explain there why we don't need a valid ID > for the container node. >>> + >>> + for (core = 0; core < ms->smp.cores; core++) { >>> + uint32_t core_offset = table_data->len - pptt_start; >>> + int thread; >>> + >>> + if (ms->smp.threads > 1) { >>> + build_processor_hierarchy_node( >>> + table_data, >>> + /* >>> + * ACPI 6.2 - Physical package >>> + * doesn't represent the boundary of a physical >>> package >>> + */ >>> + (0 << 0), >> would rather say (0 << 0) /* not a physical package */ and same elsewhere > Ok, thanks. >>> + socket_offset, core, NULL, 0); >>> + >>> + for (thread = 0; thread < ms->smp.threads; thread++) { >>> + build_processor_hierarchy_node( >>> + table_data, >>> + (1 << 1) | /* ACPI 6.2 - ACPI Processor ID >>> valid */ >>> + (1 << 2) | /* ACPI 6.3 - Processor is a >>> Thread */ >> So the references look globaly confusing to me. Either it complies to >> 6.2 or to 6.3. Looks ir rather complies with 6.3. To me, this needs to >> be clarified. > ACPI 6.2 in the comment means the flag is introduced in the spec since 6.2. > The same, ACPI 6.3 means the flag is introduced since 6.3. Maybe I should > just drop all the version-prefix in the comment ? Yes I think you can drop those comments and just upgrade the global compliance with 6.3 Thanks Eric >> I would also add the reference it complies to in the >> commit msg. > Ok, sure. > > Thanks, > Yanan > . >>> + (1 << 3), /* ACPI 6.3 - Node is a Leaf */ >>> + core_offset, uid++, NULL, 0); >>> + } >>> + } else { >>> + build_processor_hierarchy_node( >>> + table_data, >>> + (1 << 1) | /* ACPI 6.2 - ACPI Processor ID valid */ >>> + (1 << 3), /* ACPI 6.3 - Node is a Leaf */ >>> + socket_offset, uid++, NULL, 0); >>> + } >>> + } >>> + } >>> + >>> + acpi_table_end(linker, &table); >>> +} >>> + >>> /* 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/aml-build.h b/include/hw/acpi/aml-build.h >>> index 2c457c8f17..b92706388c 100644 >>> --- a/include/hw/acpi/aml-build.h >>> +++ b/include/hw/acpi/aml-build.h >>> @@ -493,6 +493,9 @@ void build_processor_hierarchy_node(GArray *tbl, >>> uint32_t flags, >>> uint32_t parent, uint32_t id, >>> uint32_t *priv_rsrc, uint32_t >>> priv_num); >>> +void build_pptt(GArray *table_data, BIOSLinker *linker, >>> MachineState *ms, >>> + const char *oem_id, const char *oem_table_id); >>> + >>> void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData >>> *f, >>> const char *oem_id, const char *oem_table_id); >>> >> Thanks >> >> Eric >> >> . >
Hi Eric, On 2021/10/20 19:11, Eric Auger wrote: > Hi Yanan, > On 10/20/21 11:51 AM, wangyanan (Y) wrote: >> Hi Eric, >> >> On 2021/10/20 16:02, Eric Auger wrote: >>> Hi, >>> >>> On 10/14/21 3:22 PM, Yanan Wang wrote: >>>> From: Andrew Jones <drjones@redhat.com> >>>> >>>> Add the Processor Properties Topology Table (PPTT) used to >>>> describe CPU topology information to ACPI guests. >>>> >>>> Note, a DT-boot Linux guest with a non-flat CPU topology will >>>> see socket and core IDs being sequential integers starting >>>> from zero, which is different from ACPI-boot Linux guest, >>>> 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 mandatorily 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. >>>> >>>> So in summary, with QEMU as the vendor for the guests, we simply >>>> use sequential integers starting from zero for the non-leaf nodes >>>> but with ID-valid flag unset, so that guest will ignore them and >>>> use table offsets as unique container IDs. And we use logical CPU >>>> IDs for the leaf nodes with the ID-valid flag set, which will be >>>> consistent with MADT. >>>> >>>> Signed-off-by: Andrew Jones <drjones@redhat.com> >>>> Co-developed-by: Yanan Wang <wangyanan55@huawei.com> >>>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com> >>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> >>>> --- >>>> hw/acpi/aml-build.c | 60 +++++++++++++++++++++++++++++++++++++ >>>> include/hw/acpi/aml-build.h | 3 ++ >>>> 2 files changed, 63 insertions(+) >>>> >>>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c >>>> index b7b9db6888..0d50e88e9d 100644 >>>> --- a/hw/acpi/aml-build.c >>>> +++ b/hw/acpi/aml-build.c >>>> @@ -1990,6 +1990,66 @@ void build_processor_hierarchy_node(GArray >>>> *tbl, uint32_t flags, >>>> } >>>> } >>>> +/* ACPI 6.2: 5.2.29 Processor Properties Topology Table (PPTT) */ >>>> +void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState >>>> *ms, >>>> + const char *oem_id, const char *oem_table_id) >>>> +{ >>>> + int pptt_start = table_data->len; >>>> + int uid = 0; >>>> + int socket; >>>> + AcpiTable table = { .sig = "PPTT", .rev = 2, >>>> + .oem_id = oem_id, .oem_table_id = >>>> oem_table_id }; >>> Table 5-149 of 6.2 spec (6.2 May 2017) tells the rev shall be 1. Or is >>> it an erratum somewhere I did miss? >> Yes, the revision in 6.2 spec is 1. And it's 2 in spec 6.3. >> So just to be sure, should I use the oldest revision ? > If you need (and use) features (such as flags) introduced in 6.3 then > you should say the code complies with 6.3 and update the above comment. The comment /* ACPI 6.2: 5.2.29 Processor Properties Topology Table (PPTT) */ tried to explain the first spec version where PPTT is introduced is 6.2. But it may adds some confusion. I think it's better to replace it with: /* * ACPI spec 5.2.29 Processor Properties Topology Table (PPTT) * (Revision 6.2 or later) */ just like our build_slit(). >>> I would also add the spec version in the commit msg. >>>> + >>>> + acpi_table_begin(&table, table_data); >>>> + >>>> + for (socket = 0; socket < ms->smp.sockets; socket++) { >>>> + uint32_t socket_offset = table_data->len - pptt_start; >>>> + int core; >>>> + >>>> + build_processor_hierarchy_node( >>>> + table_data, >>>> + /* >>>> + * ACPI 6.2 - Physical package >>>> + * represents the boundary of a physical package >>>> + */ >>>> + (1 << 0), >>>> + 0, socket, NULL, 0); >>> I see we set an ACPI process ID but in the meantime the ACPI processor >>> ID valid flag is not set. I am not sure I fully catch the meaning of >>> this latter but just to double check if this is done on purpose. >> Yes, it's on purpose. >>> Maybe >>> wort a general comment as this also happens below. >> The ID of the container node is invalid and ID of the leaf node is valid. >> The commit message by Andrew has explained why (reason 3). I think >> it may be clear enough to explain there why we don't need a valid ID >> for the container node. >>>> + >>>> + for (core = 0; core < ms->smp.cores; core++) { >>>> + uint32_t core_offset = table_data->len - pptt_start; >>>> + int thread; >>>> + >>>> + if (ms->smp.threads > 1) { >>>> + build_processor_hierarchy_node( >>>> + table_data, >>>> + /* >>>> + * ACPI 6.2 - Physical package >>>> + * doesn't represent the boundary of a physical >>>> package >>>> + */ >>>> + (0 << 0), >>> would rather say (0 << 0) /* not a physical package */ and same elsewhere >> Ok, thanks. >>>> + socket_offset, core, NULL, 0); >>>> + >>>> + for (thread = 0; thread < ms->smp.threads; thread++) { >>>> + build_processor_hierarchy_node( >>>> + table_data, >>>> + (1 << 1) | /* ACPI 6.2 - ACPI Processor ID >>>> valid */ >>>> + (1 << 2) | /* ACPI 6.3 - Processor is a >>>> Thread */ >>> So the references look globaly confusing to me. Either it complies to >>> 6.2 or to 6.3. Looks ir rather complies with 6.3. To me, this needs to >>> be clarified. >> ACPI 6.2 in the comment means the flag is introduced in the spec since 6.2. >> The same, ACPI 6.3 means the flag is introduced since 6.3. Maybe I should >> just drop all the version-prefix in the comment ? > Yes I think you can drop those comments and just upgrade the global > compliance with 6.3 > I will drop the prefix and keep the rest. And add a generic comment on top of build_pptt() as I replied above. Thanks, Yanan >>> I would also add the reference it complies to in the >>> commit msg. >> Ok, sure. >> >> Thanks, >> Yanan >> . >>>> + (1 << 3), /* ACPI 6.3 - Node is a Leaf */ >>>> + core_offset, uid++, NULL, 0); >>>> + } >>>> + } else { >>>> + build_processor_hierarchy_node( >>>> + table_data, >>>> + (1 << 1) | /* ACPI 6.2 - ACPI Processor ID valid */ >>>> + (1 << 3), /* ACPI 6.3 - Node is a Leaf */ >>>> + socket_offset, uid++, NULL, 0); >>>> + } >>>> + } >>>> + } >>>> + >>>> + acpi_table_end(linker, &table); >>>> +} >>>> + >>>> /* 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/aml-build.h b/include/hw/acpi/aml-build.h >>>> index 2c457c8f17..b92706388c 100644 >>>> --- a/include/hw/acpi/aml-build.h >>>> +++ b/include/hw/acpi/aml-build.h >>>> @@ -493,6 +493,9 @@ void build_processor_hierarchy_node(GArray *tbl, >>>> uint32_t flags, >>>> uint32_t parent, uint32_t id, >>>> uint32_t *priv_rsrc, uint32_t >>>> priv_num); >>>> +void build_pptt(GArray *table_data, BIOSLinker *linker, >>>> MachineState *ms, >>>> + const char *oem_id, const char *oem_table_id); >>>> + >>>> void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData >>>> *f, >>>> const char *oem_id, const char *oem_table_id); >>>> >>> Thanks >>> >>> Eric >>> >>> . > .
Hi, On 10/20/21 2:27 PM, wangyanan (Y) wrote: > Hi Eric, > > On 2021/10/20 19:11, Eric Auger wrote: >> Hi Yanan, >> On 10/20/21 11:51 AM, wangyanan (Y) wrote: >>> Hi Eric, >>> >>> On 2021/10/20 16:02, Eric Auger wrote: >>>> Hi, >>>> >>>> On 10/14/21 3:22 PM, Yanan Wang wrote: >>>>> From: Andrew Jones <drjones@redhat.com> >>>>> >>>>> Add the Processor Properties Topology Table (PPTT) used to >>>>> describe CPU topology information to ACPI guests. >>>>> >>>>> Note, a DT-boot Linux guest with a non-flat CPU topology will >>>>> see socket and core IDs being sequential integers starting >>>>> from zero, which is different from ACPI-boot Linux guest, >>>>> 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 mandatorily 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. >>>>> >>>>> So in summary, with QEMU as the vendor for the guests, we simply >>>>> use sequential integers starting from zero for the non-leaf nodes >>>>> but with ID-valid flag unset, so that guest will ignore them and >>>>> use table offsets as unique container IDs. And we use logical CPU >>>>> IDs for the leaf nodes with the ID-valid flag set, which will be >>>>> consistent with MADT. >>>>> >>>>> Signed-off-by: Andrew Jones <drjones@redhat.com> >>>>> Co-developed-by: Yanan Wang <wangyanan55@huawei.com> >>>>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com> >>>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> >>>>> --- >>>>> hw/acpi/aml-build.c | 60 >>>>> +++++++++++++++++++++++++++++++++++++ >>>>> include/hw/acpi/aml-build.h | 3 ++ >>>>> 2 files changed, 63 insertions(+) >>>>> >>>>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c >>>>> index b7b9db6888..0d50e88e9d 100644 >>>>> --- a/hw/acpi/aml-build.c >>>>> +++ b/hw/acpi/aml-build.c >>>>> @@ -1990,6 +1990,66 @@ void build_processor_hierarchy_node(GArray >>>>> *tbl, uint32_t flags, >>>>> } >>>>> } >>>>> +/* ACPI 6.2: 5.2.29 Processor Properties Topology Table (PPTT) */ >>>>> +void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState >>>>> *ms, >>>>> + const char *oem_id, const char *oem_table_id) >>>>> +{ >>>>> + int pptt_start = table_data->len; >>>>> + int uid = 0; >>>>> + int socket; >>>>> + AcpiTable table = { .sig = "PPTT", .rev = 2, >>>>> + .oem_id = oem_id, .oem_table_id = >>>>> oem_table_id }; >>>> Table 5-149 of 6.2 spec (6.2 May 2017) tells the rev shall be 1. Or is >>>> it an erratum somewhere I did miss? >>> Yes, the revision in 6.2 spec is 1. And it's 2 in spec 6.3. >>> So just to be sure, should I use the oldest revision ? >> If you need (and use) features (such as flags) introduced in 6.3 then >> you should say the code complies with 6.3 and update the above comment. > The comment /* ACPI 6.2: 5.2.29 Processor Properties Topology Table > (PPTT) */ > tried to explain the first spec version where PPTT is introduced is 6.2. > But it may > adds some confusion. I think it's better to replace it with: > /* > * ACPI spec 5.2.29 Processor Properties Topology Table (PPTT) > * (Revision 6.2 or later) > */ > > just like our build_slit(). That's not my understanding. I think we usually point to the latest spec revision you comply with, ie. in that case 6.3 chapter ... Thanks Eric >>>> I would also add the spec version in the commit msg. >>>>> + >>>>> + acpi_table_begin(&table, table_data); >>>>> + >>>>> + for (socket = 0; socket < ms->smp.sockets; socket++) { >>>>> + uint32_t socket_offset = table_data->len - pptt_start; >>>>> + int core; >>>>> + >>>>> + build_processor_hierarchy_node( >>>>> + table_data, >>>>> + /* >>>>> + * ACPI 6.2 - Physical package >>>>> + * represents the boundary of a physical package >>>>> + */ >>>>> + (1 << 0), >>>>> + 0, socket, NULL, 0); >>>> I see we set an ACPI process ID but in the meantime the ACPI processor >>>> ID valid flag is not set. I am not sure I fully catch the meaning of >>>> this latter but just to double check if this is done on purpose. >>> Yes, it's on purpose. >>>> Maybe >>>> wort a general comment as this also happens below. >>> The ID of the container node is invalid and ID of the leaf node is >>> valid. >>> The commit message by Andrew has explained why (reason 3). I think >>> it may be clear enough to explain there why we don't need a valid ID >>> for the container node. >>>>> + >>>>> + for (core = 0; core < ms->smp.cores; core++) { >>>>> + uint32_t core_offset = table_data->len - pptt_start; >>>>> + int thread; >>>>> + >>>>> + if (ms->smp.threads > 1) { >>>>> + build_processor_hierarchy_node( >>>>> + table_data, >>>>> + /* >>>>> + * ACPI 6.2 - Physical package >>>>> + * doesn't represent the boundary of a physical >>>>> package >>>>> + */ >>>>> + (0 << 0), >>>> would rather say (0 << 0) /* not a physical package */ and same >>>> elsewhere >>> Ok, thanks. >>>>> + socket_offset, core, NULL, 0); >>>>> + >>>>> + for (thread = 0; thread < ms->smp.threads; >>>>> thread++) { >>>>> + build_processor_hierarchy_node( >>>>> + table_data, >>>>> + (1 << 1) | /* ACPI 6.2 - ACPI Processor ID >>>>> valid */ >>>>> + (1 << 2) | /* ACPI 6.3 - Processor is a >>>>> Thread */ >>>> So the references look globaly confusing to me. Either it complies to >>>> 6.2 or to 6.3. Looks ir rather complies with 6.3. To me, this needs to >>>> be clarified. >>> ACPI 6.2 in the comment means the flag is introduced in the spec >>> since 6.2. >>> The same, ACPI 6.3 means the flag is introduced since 6.3. Maybe I >>> should >>> just drop all the version-prefix in the comment ? >> Yes I think you can drop those comments and just upgrade the global >> compliance with 6.3 >> > I will drop the prefix and keep the rest. And add a generic comment on > top of build_pptt() as I replied above. > > Thanks, > Yanan >>>> I would also add the reference it complies to in the >>>> commit msg. >>> Ok, sure. >>> >>> Thanks, >>> Yanan >>> . >>>>> + (1 << 3), /* ACPI 6.3 - Node is a Leaf */ >>>>> + core_offset, uid++, NULL, 0); >>>>> + } >>>>> + } else { >>>>> + build_processor_hierarchy_node( >>>>> + table_data, >>>>> + (1 << 1) | /* ACPI 6.2 - ACPI Processor ID >>>>> valid */ >>>>> + (1 << 3), /* ACPI 6.3 - Node is a Leaf */ >>>>> + socket_offset, uid++, NULL, 0); >>>>> + } >>>>> + } >>>>> + } >>>>> + >>>>> + acpi_table_end(linker, &table); >>>>> +} >>>>> + >>>>> /* 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/aml-build.h b/include/hw/acpi/aml-build.h >>>>> index 2c457c8f17..b92706388c 100644 >>>>> --- a/include/hw/acpi/aml-build.h >>>>> +++ b/include/hw/acpi/aml-build.h >>>>> @@ -493,6 +493,9 @@ void build_processor_hierarchy_node(GArray *tbl, >>>>> uint32_t flags, >>>>> uint32_t parent, uint32_t id, >>>>> uint32_t *priv_rsrc, uint32_t >>>>> priv_num); >>>>> +void build_pptt(GArray *table_data, BIOSLinker *linker, >>>>> MachineState *ms, >>>>> + const char *oem_id, const char *oem_table_id); >>>>> + >>>>> void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData >>>>> *f, >>>>> const char *oem_id, const char *oem_table_id); >>>>> >>>> Thanks >>>> >>>> Eric >>>> >>>> . >> . >
On 2021/10/20 20:43, Eric Auger wrote: > Hi, > On 10/20/21 2:27 PM, wangyanan (Y) wrote: >> Hi Eric, >> >> On 2021/10/20 19:11, Eric Auger wrote: >>> Hi Yanan, >>> On 10/20/21 11:51 AM, wangyanan (Y) wrote: >>>> Hi Eric, >>>> >>>> On 2021/10/20 16:02, Eric Auger wrote: >>>>> Hi, >>>>> >>>>> On 10/14/21 3:22 PM, Yanan Wang wrote: >>>>>> From: Andrew Jones <drjones@redhat.com> >>>>>> >>>>>> Add the Processor Properties Topology Table (PPTT) used to >>>>>> describe CPU topology information to ACPI guests. >>>>>> >>>>>> Note, a DT-boot Linux guest with a non-flat CPU topology will >>>>>> see socket and core IDs being sequential integers starting >>>>>> from zero, which is different from ACPI-boot Linux guest, >>>>>> 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 mandatorily 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. >>>>>> >>>>>> So in summary, with QEMU as the vendor for the guests, we simply >>>>>> use sequential integers starting from zero for the non-leaf nodes >>>>>> but with ID-valid flag unset, so that guest will ignore them and >>>>>> use table offsets as unique container IDs. And we use logical CPU >>>>>> IDs for the leaf nodes with the ID-valid flag set, which will be >>>>>> consistent with MADT. >>>>>> >>>>>> Signed-off-by: Andrew Jones <drjones@redhat.com> >>>>>> Co-developed-by: Yanan Wang <wangyanan55@huawei.com> >>>>>> Signed-off-by: Yanan Wang <wangyanan55@huawei.com> >>>>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> >>>>>> --- >>>>>> hw/acpi/aml-build.c | 60 >>>>>> +++++++++++++++++++++++++++++++++++++ >>>>>> include/hw/acpi/aml-build.h | 3 ++ >>>>>> 2 files changed, 63 insertions(+) >>>>>> >>>>>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c >>>>>> index b7b9db6888..0d50e88e9d 100644 >>>>>> --- a/hw/acpi/aml-build.c >>>>>> +++ b/hw/acpi/aml-build.c >>>>>> @@ -1990,6 +1990,66 @@ void build_processor_hierarchy_node(GArray >>>>>> *tbl, uint32_t flags, >>>>>> } >>>>>> } >>>>>> +/* ACPI 6.2: 5.2.29 Processor Properties Topology Table (PPTT) */ >>>>>> +void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState >>>>>> *ms, >>>>>> + const char *oem_id, const char *oem_table_id) >>>>>> +{ >>>>>> + int pptt_start = table_data->len; >>>>>> + int uid = 0; >>>>>> + int socket; >>>>>> + AcpiTable table = { .sig = "PPTT", .rev = 2, >>>>>> + .oem_id = oem_id, .oem_table_id = >>>>>> oem_table_id }; >>>>> Table 5-149 of 6.2 spec (6.2 May 2017) tells the rev shall be 1. Or is >>>>> it an erratum somewhere I did miss? >>>> Yes, the revision in 6.2 spec is 1. And it's 2 in spec 6.3. >>>> So just to be sure, should I use the oldest revision ? >>> If you need (and use) features (such as flags) introduced in 6.3 then >>> you should say the code complies with 6.3 and update the above comment. >> The comment /* ACPI 6.2: 5.2.29 Processor Properties Topology Table >> (PPTT) */ >> tried to explain the first spec version where PPTT is introduced is 6.2. >> But it may >> adds some confusion. I think it's better to replace it with: >> /* >> * ACPI spec 5.2.29 Processor Properties Topology Table (PPTT) >> * (Revision 6.2 or later) >> */ >> >> just like our build_slit(). > That's not my understanding. I think we usually point to the latest spec > revision you comply with, ie. in that case 6.3 chapter ... Ok, I get it. Would /* * ACPI spec, Revision 6.3 * 5.2.29 Processor Properties Topology Table (PPTT) */ be ok ? Thanks, Yanan > Thanks > > Eric >>>>> I would also add the spec version in the commit msg. >>>>>> + >>>>>> + acpi_table_begin(&table, table_data); >>>>>> + >>>>>> + for (socket = 0; socket < ms->smp.sockets; socket++) { >>>>>> + uint32_t socket_offset = table_data->len - pptt_start; >>>>>> + int core; >>>>>> + >>>>>> + build_processor_hierarchy_node( >>>>>> + table_data, >>>>>> + /* >>>>>> + * ACPI 6.2 - Physical package >>>>>> + * represents the boundary of a physical package >>>>>> + */ >>>>>> + (1 << 0), >>>>>> + 0, socket, NULL, 0); >>>>> I see we set an ACPI process ID but in the meantime the ACPI processor >>>>> ID valid flag is not set. I am not sure I fully catch the meaning of >>>>> this latter but just to double check if this is done on purpose. >>>> Yes, it's on purpose. >>>>> Maybe >>>>> wort a general comment as this also happens below. >>>> The ID of the container node is invalid and ID of the leaf node is >>>> valid. >>>> The commit message by Andrew has explained why (reason 3). I think >>>> it may be clear enough to explain there why we don't need a valid ID >>>> for the container node. >>>>>> + >>>>>> + for (core = 0; core < ms->smp.cores; core++) { >>>>>> + uint32_t core_offset = table_data->len - pptt_start; >>>>>> + int thread; >>>>>> + >>>>>> + if (ms->smp.threads > 1) { >>>>>> + build_processor_hierarchy_node( >>>>>> + table_data, >>>>>> + /* >>>>>> + * ACPI 6.2 - Physical package >>>>>> + * doesn't represent the boundary of a physical >>>>>> package >>>>>> + */ >>>>>> + (0 << 0), >>>>> would rather say (0 << 0) /* not a physical package */ and same >>>>> elsewhere >>>> Ok, thanks. >>>>>> + socket_offset, core, NULL, 0); >>>>>> + >>>>>> + for (thread = 0; thread < ms->smp.threads; >>>>>> thread++) { >>>>>> + build_processor_hierarchy_node( >>>>>> + table_data, >>>>>> + (1 << 1) | /* ACPI 6.2 - ACPI Processor ID >>>>>> valid */ >>>>>> + (1 << 2) | /* ACPI 6.3 - Processor is a >>>>>> Thread */ >>>>> So the references look globaly confusing to me. Either it complies to >>>>> 6.2 or to 6.3. Looks ir rather complies with 6.3. To me, this needs to >>>>> be clarified. >>>> ACPI 6.2 in the comment means the flag is introduced in the spec >>>> since 6.2. >>>> The same, ACPI 6.3 means the flag is introduced since 6.3. Maybe I >>>> should >>>> just drop all the version-prefix in the comment ? >>> Yes I think you can drop those comments and just upgrade the global >>> compliance with 6.3 >>> >> I will drop the prefix and keep the rest. And add a generic comment on >> top of build_pptt() as I replied above. >> >> Thanks, >> Yanan >>>>> I would also add the reference it complies to in the >>>>> commit msg. >>>> Ok, sure. >>>> >>>> Thanks, >>>> Yanan >>>> . >>>>>> + (1 << 3), /* ACPI 6.3 - Node is a Leaf */ >>>>>> + core_offset, uid++, NULL, 0); >>>>>> + } >>>>>> + } else { >>>>>> + build_processor_hierarchy_node( >>>>>> + table_data, >>>>>> + (1 << 1) | /* ACPI 6.2 - ACPI Processor ID >>>>>> valid */ >>>>>> + (1 << 3), /* ACPI 6.3 - Node is a Leaf */ >>>>>> + socket_offset, uid++, NULL, 0); >>>>>> + } >>>>>> + } >>>>>> + } >>>>>> + >>>>>> + acpi_table_end(linker, &table); >>>>>> +} >>>>>> + >>>>>> /* 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/aml-build.h b/include/hw/acpi/aml-build.h >>>>>> index 2c457c8f17..b92706388c 100644 >>>>>> --- a/include/hw/acpi/aml-build.h >>>>>> +++ b/include/hw/acpi/aml-build.h >>>>>> @@ -493,6 +493,9 @@ void build_processor_hierarchy_node(GArray *tbl, >>>>>> uint32_t flags, >>>>>> uint32_t parent, uint32_t id, >>>>>> uint32_t *priv_rsrc, uint32_t >>>>>> priv_num); >>>>>> +void build_pptt(GArray *table_data, BIOSLinker *linker, >>>>>> MachineState *ms, >>>>>> + const char *oem_id, const char *oem_table_id); >>>>>> + >>>>>> void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData >>>>>> *f, >>>>>> const char *oem_id, const char *oem_table_id); >>>>>> >>>>> Thanks >>>>> >>>>> Eric >>>>> >>>>> . >>> . > .
On Wed, Oct 20, 2021 at 08:53:00PM +0800, wangyanan (Y) wrote: > > > > > > Table 5-149 of 6.2 spec (6.2 May 2017) tells the rev shall be 1. Or is > > > > > > it an erratum somewhere I did miss? > > > > > Yes, the revision in 6.2 spec is 1. And it's 2 in spec 6.3. > > > > > So just to be sure, should I use the oldest revision ? > > > > If you need (and use) features (such as flags) introduced in 6.3 then > > > > you should say the code complies with 6.3 and update the above comment. > > > The comment /* ACPI 6.2: 5.2.29 Processor Properties Topology Table > > > (PPTT) */ > > > tried to explain the first spec version where PPTT is introduced is 6.2. > > > But it may > > > adds some confusion. I think it's better to replace it with: > > > /* > > > * ACPI spec 5.2.29 Processor Properties Topology Table (PPTT) > > > * (Revision 6.2 or later) > > > */ > > > > > > just like our build_slit(). > > That's not my understanding. I think we usually point to the latest spec > > revision you comply with, ie. in that case 6.3 chapter ... > Ok, I get it. Would > /* > * ACPI spec, Revision 6.3 > * 5.2.29 Processor Properties Topology Table (PPTT) > */ > be ok ? > Yes, we need to call out the oldest spec we can for the functionality we're using. In this case, we're using flags first defined in the 6.3 spec, so we need to call out the 6.3 spec. Thanks, drew
On 2021/10/21 17:08, Andrew Jones wrote: > On Wed, Oct 20, 2021 at 08:53:00PM +0800, wangyanan (Y) wrote: >>>>>>> Table 5-149 of 6.2 spec (6.2 May 2017) tells the rev shall be 1. Or is >>>>>>> it an erratum somewhere I did miss? >>>>>> Yes, the revision in 6.2 spec is 1. And it's 2 in spec 6.3. >>>>>> So just to be sure, should I use the oldest revision ? >>>>> If you need (and use) features (such as flags) introduced in 6.3 then >>>>> you should say the code complies with 6.3 and update the above comment. >>>> The comment /* ACPI 6.2: 5.2.29 Processor Properties Topology Table >>>> (PPTT) */ >>>> tried to explain the first spec version where PPTT is introduced is 6.2. >>>> But it may >>>> adds some confusion. I think it's better to replace it with: >>>> /* >>>> * ACPI spec 5.2.29 Processor Properties Topology Table (PPTT) >>>> * (Revision 6.2 or later) >>>> */ >>>> >>>> just like our build_slit(). >>> That's not my understanding. I think we usually point to the latest spec >>> revision you comply with, ie. in that case 6.3 chapter ... >> Ok, I get it. Would >> /* >> * ACPI spec, Revision 6.3 >> * 5.2.29 Processor Properties Topology Table (PPTT) >> */ >> be ok ? >> > Yes, we need to call out the oldest spec we can for the functionality > we're using. In this case, we're using flags first defined in the 6.3 > spec, so we need to call out the 6.3 spec. > Agreed. I have updated these comments as suggested in v9. And I just noticed that Richard has taken that v9 into the target-arm tree. Thanks, Yanan
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c index b7b9db6888..0d50e88e9d 100644 --- a/hw/acpi/aml-build.c +++ b/hw/acpi/aml-build.c @@ -1990,6 +1990,66 @@ void build_processor_hierarchy_node(GArray *tbl, uint32_t flags, } } +/* ACPI 6.2: 5.2.29 Processor Properties Topology Table (PPTT) */ +void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState *ms, + const char *oem_id, const char *oem_table_id) +{ + int pptt_start = table_data->len; + int uid = 0; + int socket; + AcpiTable table = { .sig = "PPTT", .rev = 2, + .oem_id = oem_id, .oem_table_id = oem_table_id }; + + acpi_table_begin(&table, table_data); + + for (socket = 0; socket < ms->smp.sockets; socket++) { + uint32_t socket_offset = table_data->len - pptt_start; + int core; + + build_processor_hierarchy_node( + table_data, + /* + * ACPI 6.2 - Physical package + * represents the boundary of a physical package + */ + (1 << 0), + 0, socket, NULL, 0); + + for (core = 0; core < ms->smp.cores; core++) { + uint32_t core_offset = table_data->len - pptt_start; + int thread; + + if (ms->smp.threads > 1) { + build_processor_hierarchy_node( + table_data, + /* + * ACPI 6.2 - Physical package + * doesn't represent the boundary of a physical package + */ + (0 << 0), + socket_offset, core, NULL, 0); + + for (thread = 0; thread < ms->smp.threads; thread++) { + build_processor_hierarchy_node( + table_data, + (1 << 1) | /* ACPI 6.2 - 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++, NULL, 0); + } + } else { + build_processor_hierarchy_node( + table_data, + (1 << 1) | /* ACPI 6.2 - ACPI Processor ID valid */ + (1 << 3), /* ACPI 6.3 - Node is a Leaf */ + socket_offset, uid++, NULL, 0); + } + } + } + + acpi_table_end(linker, &table); +} + /* 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/aml-build.h b/include/hw/acpi/aml-build.h index 2c457c8f17..b92706388c 100644 --- a/include/hw/acpi/aml-build.h +++ b/include/hw/acpi/aml-build.h @@ -493,6 +493,9 @@ void build_processor_hierarchy_node(GArray *tbl, uint32_t flags, uint32_t parent, uint32_t id, uint32_t *priv_rsrc, uint32_t priv_num); +void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState *ms, + const char *oem_id, const char *oem_table_id); + void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f, const char *oem_id, const char *oem_table_id);