|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] RE: [PATCH v5 4/4] arm/acpi: Parse PPTT to initialize CPU topology
Hello,
> > --- a/xen/drivers/acpi/topology.c
> > +++ b/xen/drivers/acpi/topology.c
> > @@ -5,18 +5,90 @@
> > #include <xen/cpumask.h>
> > #include <xen/init.h>
> >
> > -/*
> > - * TODO: Populate the topology information by scanning the ACPI
> > - * PPTT (Processor Properties Topology Table).
> > - */
> > -void __init acpi_init_cpu_topology(void)
> > +uint32_t map_cpu_acpiid[NR_CPUS] __initdata =
> > + { [0 ... NR_CPUS - 1] = INVALID_ACPIID };
> > +uint32_t socket_map[NR_CPUS] __initdata;
> > +uint32_t cluster_map[NR_CPUS] __initdata;
> > +uint32_t core_map[NR_CPUS] __initdata;
> > +uint32_t thread_map[NR_CPUS] __initdata;
> > +unsigned int __initdata num_sockets;
> > +unsigned int __initdata num_clusters;
> > +unsigned int __initdata num_cores;
>
> static for almost all of these? And please place __initdata uniformly,
> between type and identifier.
Okay.
> For large NR_CPUS this also looks to be adding quite a bit of data. Is all
> of this really needed?
The map_cpu_acpiid[NR_CPUS] array is required. This is because
acpi_smp_init_cpus()
parses the MADT and populates map_cpu_acpiid[] with the discovered CPUs. At the
point when acpi_smp_init_cpus() is called, the total number of CPUs has not yet
been
determined.
The memory for socket_map[NR_CPUS], cluster_map[NR_CPUS], and core_map[NR_CPUS]
is automatically freed after Xen initialization completes, as they are marked
as __initdata.
Therefore, I am not sure if there is any clear advantage to switching them to
dynamic
allocation and deallocation.
> Finally please see ./CODING_STYLE as to the use of fixed-width types.
Okay, I will fix them.
> > +static unsigned int __init get_logical_id(uint32_t phys_offset,
> > + uint32_t *map,
> > + unsigned int *count)
> > +{
> > + unsigned int id;
> > +
> > + for ( id = 0; id < *count; id++ )
> > + if ( map[id] == phys_offset )
> > + return id;
> > +
> > + map[*count] = phys_offset;
> > + id = *count;
> > + (*count)++;
>
> Imo better as either
>
> id = (*count)++;
>
> (or yet more simply
>
> return (*count)++;
Okay.
> ) or
>
> id = *count;
> ++*count;
>
> > + return id;
> > +}
> > +
> > +static struct acpi_pptt_processor *__init find_pptt_node(
> > + const struct acpi_table_header *table_hdr, unsigned int acpi_id)
>
> Nit: Bad indentation; should be identical to ...
Okay.
> > +{
> > + const struct acpi_subtable_header *entry;
> > + unsigned long table_end;
> > + const char *ptr;
>
> ... that of function-scope local variables.
>
> > + if ( !table_hdr )
> > + return NULL;
>
> Isn't this dead code?
Indeed, this is redundant as the caller already validates the table status.
I will drop the code or replace it with BUG_ON(!table_hdr );
> > + table_end = (unsigned long)table_hdr + table_hdr->length;
> > +
> > + ptr = (const char *)table_hdr + sizeof(struct acpi_table_pptt);
>
> There's way too much casting and other type-unsafe code in the function. For
> example, if the caller passed the full const struct acpi_table_pptt * into
> here, the above (ptr being const void *) could become
>
> ptr = pptt + 1;
Okay.
> > + while ( (unsigned long)ptr + sizeof(struct acpi_subtable_header)
> > + <= table_end )
> > + {
> > + entry = (const struct acpi_subtable_header *)ptr;
>
> Then no cast would be needed here either.
>
> > + if ( entry->length == 0 )
> > + {
> > + printk(XENLOG_ERR
> > + "ACPI: PPTT has an invalid zero-length subtable.\n");
> > + break;
> > + }
> > +
> > + if ( (unsigned long)ptr + entry->length > table_end )
> > + {
> > + printk(XENLOG_ERR
> > + "ACPI: PPTT subtable extends beyond table end.\n");
> > + break;
> > + }
> > +
> > + if ( entry->type == ACPI_PPTT_TYPE_PROCESSOR )
> > + if ( entry->length >= sizeof(struct acpi_pptt_processor) )
>
> Please fold two if()s like these ones. Then again - isn't there an "else"
> wanted for the inner if()? It doesn't look appropriate to continue the
> loop when the length doesn't fit the type.
Okay.
> > + {
> > + struct acpi_pptt_processor *proc =
> > + (struct acpi_pptt_processor *)entry;
>
> Please use container_of(). That'll (I think) also avoid you casting away
> const-ness (which Misra objects to for a good reason).
Okay.
> > @@ -30,6 +102,148 @@ void __init acpi_init_cpu_topology(void)
> > }
> > }
> >
> > +/*
> > + * Populate the topology information by scanning the ACPI PPTT
> > + * (Processor Properties Topology Table).
> > + */
> > +void __init acpi_init_cpu_topology(void)
> > +{
> > + acpi_status status;
> > + struct acpi_table_header *header;
> > + const struct acpi_table_pptt *pptt;
> > + unsigned int cpu;
> > +
> > + status = acpi_get_table(ACPI_SIG_PPTT, 0, &header);
> > + if ( ACPI_FAILURE(status) )
> > + {
> > + printk(XENLOG_WARNING
> > + "ACPI: PPTT table not found. Topology fallback will be
> > used.\n");
> > + setup_fake_topology();
> > + return;
> > + }
> > +
> > + pptt = (struct acpi_table_pptt *)header;
>
> Again container_of() please.
Okay.
> > + for_each_possible_cpu(cpu)
> > + {
> > + unsigned int acpi_id = map_cpu_acpiid[cpu];
> > + struct cpu_topology *topo = &cpu_topology[cpu];
> > + const struct acpi_pptt_processor *proc;
> > + unsigned int level = 0;
> > + uint32_t thread_offset = 0;
> > + uint32_t core_offset = 0;
> > + uint32_t cluster_offset = 0;
> > + uint32_t socket_offset = 0;
> > + bool threading = true;
> > +
> > + proc = find_pptt_node(&pptt->header, acpi_id);
> > + if ( !proc )
> > + {
> > + printk(XENLOG_WARNING
> > + "ACPI: No PPTT leaf node for CPU %u (ACPI ID 0x%u)\n",
> > + cpu, acpi_id);
> > + continue;
> > + }
> > +
> > + while ( proc )
> > + {
> > + if ( proc->flags & ACPI_PPTT_PHYSICAL_PACKAGE )
> > + {
> > + socket_offset = (char *)proc - (char *)pptt;
> > + break;
> > + }
> > + else if ( level == 0 )
> > + /*
> > + * ACPI_PPTT_PROCESSOR_IS_THREAD is supported in PPTT
> > + * revision 2 and later.
> > + */
> > + if ( proc->flags & ACPI_PPTT_ACPI_PROCESSOR_IS_THREAD )
> > + thread_offset = (char *)proc - (char *)pptt;
>
> This variable is only ever set, never read.
>
> Also I think casts to const void * are to be preferred for code like this
> (if such offset calculations are needed in the first place). Or casts to
> unsigned long.
Okay.
> > + else
> > + {
> > + /* Assume no threading support when PPTT revision is
> > 1. */
> > + threading = false;
> > + core_offset = (char *)proc - (char *)pptt;
> > + }
> > + else if ( level == 1 )
> > + if ( threading )
> > + core_offset = (char *)proc - (char *)pptt;
> > + else
> > + cluster_offset = (char *)proc - (char *)pptt;
> > + else if ( level == 2 )
> > + if ( threading )
> > + cluster_offset = (char *)proc - (char *)pptt;
>
> PLease add braces to separate inner if/else from outer ones.
Okay.
> > + if ( proc->parent )
> > + {
> > + proc = (const struct acpi_pptt_processor *)
> > + ((char *)pptt + proc->parent);
> > + level++;
> > + }
> > + else
> > + break;
> > + }
> > +
> > + topo->phys_socket_id =
> > + get_logical_id(socket_offset, socket_map, &num_sockets);
> > + topo->phys_cluster_id =
> > + get_logical_id(cluster_offset, cluster_map, &num_clusters);
> > + topo->phys_core_id =
> > + get_logical_id(core_offset, core_map, &num_cores);
>
> What if any of the ..._offset is still 0?
A value of `cluster_offset == 0` does not mean that cluster information is
absent; rather,
it happens when the PPTT is malformed or corrupted.
The same applies to `socket_offset == 0` and `core_offset == 0`. It would be
much
better to treat all of these as error cases and reject them. I will add proper
validation
and error handling for these zero offsets.
> > + /* Fall back to socket ID if PPTT lacks cluster information. */
> > + if ( topo->phys_cluster_id == 0 )
> > + topo->phys_cluster_id = topo->phys_socket_id;
>
> Why would 0 indicate the absence of cluster information? Isn't it
> cluster_offset being 0 which does so?
If the PPTT lacks cluster description, all CPUs reuse the same offset, causing
get_logical_id() to always return 0.
Even when clusters do exist and the actual cluster ID 0 is assigned, the socket
ID
will also be 0, so the fallback assignment still results in the correct
topology.
I will add a comment to explain this tricky part.
> > + }
> > +
> > + for_each_possible_cpu(cpu)
> > + {
> > + struct cpu_topology *topo = &cpu_topology[cpu];
> > + unsigned int tcpu;
> > +
> > + for_each_possible_cpu(tcpu)
> > + {
> > + struct cpu_topology *ttopo = &cpu_topology[tcpu];
> > +
> > + if ( cpu > tcpu )
> > + continue;
> > +
> > + if ( topo->phys_core_id == ttopo->phys_core_id )
> > + {
> > + cpumask_set_cpu(tcpu, topo->thread_sibling);
> > + cpumask_set_cpu(cpu, ttopo->thread_sibling);
> > + }
> > +
> > + if ( topo->phys_cluster_id == ttopo->phys_cluster_id )
> > + {
> > + cpumask_set_cpu(tcpu, topo->cluster_sibling);
> > + cpumask_set_cpu(cpu, ttopo->cluster_sibling);
> > + }
> > +
> > + if ( topo->phys_socket_id == ttopo->phys_socket_id )
> > + {
> > + cpumask_set_cpu(tcpu, topo->core_sibling);
> > + cpumask_set_cpu(cpu, ttopo->core_sibling);
> > + }
> > + }
> > +
> > + topo->num_siblings = cpumask_weight(topo->thread_sibling);
> > + }
> > +
> > + for_each_possible_cpu(cpu)
> > + {
> > + const struct cpu_topology *topo = &cpu_topology[cpu];
> > +
> > + printk(XENLOG_DEBUG
> > + "ACPI: acpi_id[%u] CPU-%u Socket-%u Cluster-%u Core-%u\n",
> > + map_cpu_acpiid[cpu],
> > + cpu,
> > + topo->phys_socket_id,
> > + topo->phys_cluster_id,
> > + topo->phys_core_id);
> > + }
>
> Is this meant to stay? It can be a lot of output with many CPUs.
This is just for debugging.
I will delete it.
> > --- a/xen/include/acpi/actbl3.h
> > +++ b/xen/include/acpi/actbl3.h
> > @@ -72,6 +72,7 @@
> >
> > #define ACPI_SIG_S3PT "S3PT" /* S3 Performance (sub)Table */
> > #define ACPI_SIG_PCCS "PCC" /* PCC Shared Memory Region */
> > +#define ACPI_SIG_PPTT "PPTT" /* Processor Properties
> > Topology Table */
> >
> > /* Reserved table signatures */
> >
> > @@ -637,6 +638,35 @@ struct acpi_table_stao {
> > u8 ignore_uart;
> > };
> >
> >
> +/*************************************************************************
> ******
> > + *
> > + * PPTT - Processor Properties Topology Table - ACPI 6.3
> > + * Version 1
> > + *
> > +
> ***************************************************************************
> ***/
> > +struct acpi_table_pptt {
> > + struct acpi_table_header header;
> > +};
> > +
> > +#define ACPI_PPTT_TYPE_PROCESSOR 0
> > +#define ACPI_PPTT_TYPE_CACHE 1
> > +#define ACPI_PPTT_TYPE_ID 2
> > +
> > +struct acpi_pptt_processor {
> > + struct acpi_subtable_header header;
> > + u16 reserved;
> > + u32 flags;
> > + u32 parent;
> > + u32 acpi_processor_id;
> > + u32 number_of_priv_resources;
> > +};
> > +
> > +#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 */
> > +
> > /* Reset to default packing */
> >
> > #pragma pack()
>
> Linux, which presumably still takes it from ACPI CA, has this in actbl2.h.
> Please match placement as closely as possible. Ideally take (as a separate,
> prereq patch) the Linux commit(s) adding the definitions. See
> docs/process/sending-patches.pandoc for formal aspects of doing so.
The locations where macros and structures are defined in actbl1.h, actbl2.h, and
actbl3.h are completely different between the Linux kernel and Xen.
For example, linux's actbl2.h and Xen's actbl2.h have an entirely different set
of
table signatures. Given this mismatch, attempting to fully align with Linux's
placement might
require updating or reorganizing Xen's ACPI headers first.
Linux kernel: actbl2.h
/*
* Values for description table header signatures for tables defined in this
* file. Useful because they make it more difficult to inadvertently type in
* the wrong signature.
*/
#define ACPI_SIG_AGDI "AGDI" /* Arm Generic Diagnostic Dump and
Reset Device Interface */
#define ACPI_SIG_APMT "APMT" /* Arm Performance Monitoring Unit
table */
#define ACPI_SIG_BDAT "BDAT" /* BIOS Data ACPI Table */
#define ACPI_SIG_CCEL "CCEL" /* CC Event Log Table */
#define ACPI_SIG_CDAT "CDAT" /* Coherent Device Attribute Table */
#define ACPI_SIG_ERDT "ERDT" /* Enhanced Resource Director
Technology */
#define ACPI_SIG_IORT "IORT" /* IO Remapping Table */
#define ACPI_SIG_IOVT "IOVT" /* I/O Virtualization Table */
#define ACPI_SIG_IVRS "IVRS" /* I/O Virtualization Reporting
Structure */
#define ACPI_SIG_KEYP "KEYP" /* Key Programming Interface for IDE */
#define ACPI_SIG_LPIT "LPIT" /* Low Power Idle Table */
#define ACPI_SIG_MADT "APIC" /* Multiple APIC Description Table */
#define ACPI_SIG_MCFG "MCFG" /* PCI Memory Mapped Configuration
table */
#define ACPI_SIG_MCHI "MCHI" /* Management Controller Host Interface
table */
#define ACPI_SIG_MPAM "MPAM" /* Memory System Resource Partitioning
and Monitoring Table */
#define ACPI_SIG_MPST "MPST" /* Memory Power State Table */
#define ACPI_SIG_MRRM "MRRM" /* Memory Range and Region Mapping
table */
#define ACPI_SIG_MSDM "MSDM" /* Microsoft Data Management Table */
#define ACPI_SIG_NFIT "NFIT" /* NVDIMM Firmware Interface Table */
#define ACPI_SIG_NHLT "NHLT" /* Non HD Audio Link Table */
#define ACPI_SIG_PCCT "PCCT" /* Platform Communications Channel
Table */
#define ACPI_SIG_PDTT "PDTT" /* Platform Debug Trigger Table */
#define ACPI_SIG_PHAT "PHAT" /* Platform Health Assessment Table */
#define ACPI_SIG_PMTT "PMTT" /* Platform Memory Topology Table */
#define ACPI_SIG_PPTT "PPTT" /* Processor Properties Topology Table
*/
#define ACPI_SIG_PRMT "PRMT" /* Platform Runtime Mechanism Table */
#define ACPI_SIG_RASF "RASF" /* RAS Feature table */
#define ACPI_SIG_RAS2 "RAS2" /* RAS2 Feature table */
#define ACPI_SIG_RGRT "RGRT" /* Regulatory Graphics Resource Table */
#define ACPI_SIG_RHCT "RHCT" /* RISC-V Hart Capabilities Table */
#define ACPI_SIG_RIMT "RIMT" /* RISC-V IO Mapping Table */
#define ACPI_SIG_SBST "SBST" /* Smart Battery Specification Table */
#define ACPI_SIG_SDEI "SDEI" /* Software Delegated Exception
Interface Table */
#define ACPI_SIG_SDEV "SDEV" /* Secure Devices table */
#define ACPI_SIG_SVKL "SVKL" /* Storage Volume Key Location Table */
#define ACPI_SIG_SWFT "SWFT" /* SoundWire File Table */
#define ACPI_SIG_TDEL "TDEL" /* TD Event Log Table */
Xen: actbl2.h
/*
* Values for description table header signatures for tables defined in this
* file. Useful because they make it more difficult to inadvertently type in
* the wrong signature.
*/
#define ACPI_SIG_ASF "ASF!" /* Alert Standard Format table */
#define ACPI_SIG_BOOT "BOOT" /* Simple Boot Flag Table */
#define ACPI_SIG_DBG2 "DBG2" /* Debug Port table type 2 */
#define ACPI_SIG_DBGP "DBGP" /* Debug Port table */
#define ACPI_SIG_DMAR "DMAR" /* DMA Remapping table */
#define ACPI_SIG_HPET "HPET" /* High Precision Event Timer table */
#define ACPI_SIG_IBFT "IBFT" /* i_sCSI Boot Firmware Table */
#define ACPI_SIG_IORT "IORT" /* IO Remapping Table */
#define ACPI_SIG_IVRS "IVRS" /* I/O Virtualization Reporting
Structure */
#define ACPI_SIG_MCFG "MCFG" /* PCI Memory Mapped Configuration
table */
#define ACPI_SIG_MCHI "MCHI" /* Management Controller Host Interface
table */
#define ACPI_SIG_SLIC "SLIC" /* Software Licensing Description Table
*/
#define ACPI_SIG_SPCR "SPCR" /* Serial Port Console Redirection
table */
#define ACPI_SIG_SPMI "SPMI" /* Server Platform Management Interface
table */
#define ACPI_SIG_TCPA "TCPA" /* Trusted Computing Platform Alliance
table */
#define ACPI_SIG_UEFI "UEFI" /* Uefi Boot Optimization Table */
#define ACPI_SIG_WAET "WAET" /* Windows ACPI Emulated devices Table
*/
#define ACPI_SIG_WDAT "WDAT" /* Watchdog Action Table */
#define ACPI_SIG_WDDT "WDDT" /* Watchdog Timer Description Table */
#define ACPI_SIG_WDRT "WDRT" /* Watchdog Resource Table */
#ifdef ACPI_UNDEFINED_TABLES
/*
* These tables have been seen in the field, but no definition has been found
*/
#define ACPI_SIG_ATKG "ATKG"
#define ACPI_SIG_GSCI "GSCI" /* GMCH SCI table */
#define ACPI_SIG_IEIT "IEIT"
#endif
> > --- a/xen/include/xen/acpi.h
> > +++ b/xen/include/xen/acpi.h
> > @@ -139,8 +139,16 @@ static inline int acpi_boot_table_init(void)
> >
> > void acpi_init_cpu_topology(void);
> >
> > +extern uint32_t map_cpu_acpiid[NR_CPUS];
>
> Since this is __initdata, imo ...
>
> > +static inline void acpi_map_cpu_acpiid(unsigned int cpu, uint32_t acpi_id)
>
> ... this would better be annotated __init as well, even if for an inline
> function that's unlikely to take any effect. Other than the important one
> here: Documentation.
Okay.
Thank you,
Hirokazu Takahashi.
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |