Re: [PATCH v3 6/7] arm64: topology: Enable ACPI/PPTT based CPU topology.

[Al Stone <ahs3@redhat.com>]

On 10/20/2017 03:22 AM, Lorenzo Pieralisi wrote:
> On Thu, Oct 19, 2017 at 11:54:22AM -0500, Jeremy Linton wrote:
> 
> [...]
> 
>>>> +			cpu_topology[cpu].core_id   = topology_id;
>>>> +			topology_id = setup_acpi_cpu_topology(cpu, 2);
>>>> +			cpu_topology[cpu].cluster_id = topology_id;
>>>> +			topology_id = setup_acpi_cpu_topology(cpu, max_topo);
>>>
>>> If you want a package id (that's just a package tag to group cores), you
>>> should not use a large level because you know how setup_acpi_cpu_topology()works, you should add an API that allows you to retrieve the package id
>>> (so that you can use th ACPI_PPTT_PHYSICAL_PACKAGE flag consistenly,
>>> whatever it represents).
>>
>> I don't think the spec requires the use of PHYSICAL_PACKAGE... Am I
>> misreading it? Which means we need to "pick" a node level to
>> represent the physical package if one doesn't exist...
> 
> The specs define a means to detect if a given PPTT node corresponds to a
> package (I am refraining from stating again that to me that's not clean
> cut what a package is _architecturally_, I think you know my POV by now)
> and that's what you need to use to retrieve a packageid for a given cpu,
> if I understand the aim of the physical package flag.
> 
> Either that or that flag is completely useless.
> 
> Lorenzo
> 
> ACPI 6.2 - Table 5-151 (page 248)
> Physical package
> -----------------
> Set to 1 if this node of the processor topology represents the boundary
> of a physical package, whether socketed or surface mounted.  Set to 0 if
> this instance of the processor topology does not represent the boundary
> of a physical package.
> 

I've been following the discussion and I'm not sure I understand what the
confusion is around having a physical package ID.  Since I'm the one that
insisted it be in the spec, I'd be glad to clarify anything.  My apologies
for not saying anything sooner but things IRL have been very complicated
of late.

What was intended was a simple flag that was meant to tell me if a CPU ID
(this could be a CPU, a cluster, a processor container -- I don't really
care which) is *also* an actual physical device on a motherboard.  That is
the only intent; there was no architectural meaning intended at all -- that
is what the PPTT structures are for, in conjunction with any DSDT information
uncovered later in the boot process.

However, in the broader server ecosystem, this can be incredibly useful.  There
are a significant number of software products sold or supported that base their
fees on the number of physical sockets in use.  There have been in the past (and
may be in the near future) machines where the cost of the lease on the machine
is determined by how many physical sockets (or even CPUs) are in use, even if
the machine has many more available.

Some vendors also include FRU (Field Replaceable Unit) location info in their
ACPI tables.  So, for example, one or more CPUs or caches might fail in one
physical package, which is then reported to a maintenance system of some sort
that tells some human which of the physical sockets on what motherboard needs a
replacement device, or it's simply noted and shut off until it's time to replace
the entire server, or perhaps it's logged and used in an algorithm to predict
when the server might fail completely.

So, that's why the flag exists in the spec.  It seems to make sense to me to
have a package ID as part of struct cpu_topology -- it might even be really
handy for CPU hotplug.  If you don't, it seems to me a whole separate struct
would be needed with more cpumasks to show who belongs to what physical package;
that might be okay but seems unnecessarily complicated to me.

You can also tell me that I have completely missed the point of the discussion
so far :-).  But if you do, you have to tell me what I missed.

Hope this helps clarify...

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------