On Fri, 2008-10-24 at 13:36 +0800, Zhao Yakui wrote:
> On Thu, 2008-10-23 at 21:07 -0600, Myron Stowe wrote:
> > On Fri, 2008-10-24 at 09:16 +0800, Zhao Yakui wrote:
> > > On Wed, 2008-10-22 at 13:12 -0600, Myron Stowe wrote:
> > > > Declaring processors in ACPI namespace can be done using either a "Processor"
> > > > definition or a "Device" definition (see section 8.4 - Declaring Processors;
> > > > "Advanced Configuration and Power Interface Specification", Revision 3.0b).
> > > > Currently the two processor declaration types are conflated.
> > > In most cases there is no _UID object under the scope of processor
> > > namespace. So the current source can work well without adding the new
> > > HID("ACPI_CPU") for processor definition.
> > >
> > > If there exists the _UID object under the scope of processor namespace,
> > > IMO the ProcessorID returned by _UID will have higher priority. In such
> > > case the patch can't work well.
> >
> > According to section 5.2.11.13 "Local SAPIC Structure" - Local SAPIC to
> > processor object association uses the 'ProcessorID' for CPUs declared
> > with "Processor" statements and Local SAPIC to processor object
> > association for CPUs declared with "Device" statements use the '_UID'.
> > There is no "higher priority" - the association is fixed and must take
> > into account the type of CPU declaration - either "Processor" or
> > "Device" - to use the appropriate field - either 'ProcessorID' or '_UID'
> > - for the match.
> Agree with what you said. For the processor definition using Processor
> macro, we can get the ACPI ID by the processor block or _UID object (if
> it exists) and match it with the ACPI processor ID filed of LAPIC table
> or sapic table.
> If there is no _UID object under the scope of processor namespace, the
> current source code still can work well. In such case it is unnecessary
> to add a new HID. Of course it is harmless if you add it.
> > In example, the combinations of CPU declaration type used in combination
> > with whether or not the CPU declaration contains a _UID child object are
> > "Processor" without a _UID child object (which our systems have)
> > "Processor" with a _UID child object (which our systems have)
> It is very lucky that there exists such a system. Will you please
> confirm whether the ACPI ID returned by _UID object is consistent with
> the ACPI ID declared in the processor block?
> If they are the same value, any one can be used.
> If they are different, maybe we should confirm which one is correct.
>
> The remaining issue is that _UID can return the string according to ACPI
> spec. If the _UID string is returned, it should be matched with the ACPI
> UID_string field of slapic table to get the APIC ID of processor.
>
> If you can confirm that the integer is returned by the _UID object under
> the scope of processor using device definition on your system, I think
> that your patch is OK.
Yakui:
After I encountered the issue that this patch series addresses I wrote
up a synopsis in my notes. I find that trying to write something down
really helps uncover areas I think I am familiar with but may not really
be. Plus, I then have the notes for future reference.
Anyway, I really didn't plan on sending this out but here is my write up
from encountering this issue initially. Please take the time to read
this and follow the code. If you follow the examples carefully I
believe that you will then come to the same conclusion that I did and
thus then understand the need for this patch series.
Thanks for your questions as I'm sure others have had the exact same
ones.
Myron
SYNOPSIS (This is IA64 specific)
During early boot two tables are created which map Logical CPUs to the
CPUs Physical {S}APIC ID. These tables are 'smp_boot_data' and
'ia64_cpu_to_sapic[]'.
arch/ia64/include/asm/smp.h::
extern struct smp_boot_data {
int cpu_count;
int cpu_phys_id[NR_CPUS];
} smp_boot_data __initdata;
arch/ia64/kernel/smpboot.c::
/* which logical CPU number maps to which CPU (physical APIC ID) */
volatile int ia64_cpu_to_sapicid[NR_CPUS];
EXPORT_SYMBOL(ia64_cpu_to_sapicid);
A CPUs Physical {S}APIC ID, often referred to as the CPUs Physical ID,
is obtained from the CPUs corresponding LOCAL APIC entry in the
platform's MADT by concatinating the Logical ID and EID -
"lid.f.id << 8 | lid.f.eid;".
During initialization arch/ia64/kernel/acpi.c::acpi_boot_init() is
invoked which parses the MADT via acpi_parse_lsapic().
acpi_parse_lsapic() sets up 'available_cpus', 'total_cpus', and
'smp_boot_data' which contains the 'cpu_phys_id[]' table. Later, still
within the acpi_boot_init() routine, smp_build_cpu_map() is invoked.
smp_build_cpu_map() initializes the 'arch_cpu_to_apicid[]' (which for
IA64 is 'ia64_cpu_to_sapicid[]) table using information from
'smp_boot_data'.
acpi_boot_init()
{
acpi_parse_lsapic();
...
smp_build_cpu_map();
}
In the above initialization, 'boot_cpu_id' is handled specially -
arch/ia64/kernel/smpboot.c::smp_build_cpu_map()
int boot_cpu_id = hard_smp_processor_id();
arch/ia64/include/asm/smp.h::
static inline unsigned int ia64_get_lid (void)
{
union {
struct {
unsigned long reserved : 16;
unsigned long eid : 8;
unsigned long id : 8;
unsigned long ignored : 32;
} f;
unsigned long bits;
} lid;
lid.bits = ia64_getreg(_IA64_REG_CR_LID);
return lid.f.id << 8 | lid.f.eid;
}
#define hard_smp_processor_id() ia64_get_lid()
The result of this setup running on our new system yields:
smp_build_cpu_map
arch_cpu_to_apicid[00] 0x0000
arch_cpu_to_apicid[01] 0x0100
arch_cpu_to_apicid[02] 0x0200
arch_cpu_to_apicid[03] 0x0300
arch_cpu_to_apicid[04] 0x1000
arch_cpu_to_apicid[05] 0x1100
arch_cpu_to_apicid[06] 0x1200
arch_cpu_to_apicid[07] 0x1300
arch_cpu_to_apicid[08] 0x0002
arch_cpu_to_apicid[09] 0x0102
arch_cpu_to_apicid[10] 0x0202
arch_cpu_to_apicid[11] 0x0302
arch_cpu_to_apicid[12] 0x1002
arch_cpu_to_apicid[13] 0x1102
arch_cpu_to_apicid[14] 0x1202
arch_cpu_to_apicid[15] 0x1302
16 CPUs available, 16 CPUs total
Later in the kernel's boot processing 'acpi_processor_init()' is
invoked. Via object oriented obfuscation both 'acpi_processor_add()'
and 'acpi_processor_start()' get invoked. As part of
'acpi_processor_start()' processing 'acpi_processor_get_info()' is
called to get processor specific information (all of which is in
drivers/acpi/processor_core.c). 'acpi_processor_get_info()' is called
with two parameters; a pointer to an 'acpi_processor' struct and a flag
indicating whether or not the processor object has a _UID. The latter
_UID flag was set earlier in the boot process - presumably when the ACPI
namespace was parsed.
The processor's _UID flag indicates which mechanism was used in the
namespace for declaring the processor: the ASL Processor statement or
ASL Device statement. If the Device statement was used then the
processor's 'acpi_id' is set to it's _UID value: 'pr->acpi_id = value;'.
For Processor statement declarations the processor's 'acpi_id' is set to
the ProcessorID value: 'pr->acpi_id = object.processor.proc_id;'. With
the processor's acpi_id ('pr->acpi_id') value obtained, 'get_cpu_id()'
is called to get and set the corresponding Logical CPU ID
('cpu_index'/'pr->apic_id').
'get_cpu_id()' first maps the processor's 'acpi_id' to the corresponding
'apic_id' using either the processor's _MAT ACPI entry if one exists or
the platforms MADT ACPI table. Either way, the entry/table is parsed
for LOCAL_{S}APIC entries. The parsing - 'map_lapic_id()' or
'map_lsapic_id()' - first checks the 'lapic_flags' field to see if the
processor is ENABLED (non-enabled processors are ignored). Next the
'processor_id' member is checked against the 'acpi_id' for a match (this
checking takes into consideration _UID values for Local SAPIC entries:
/* First check against id */
if (lsapic->processor_id == acpi_id) {
*apic_id = (lsapic->id << 8) | lsapic->eid;
return 1;
/* Check against optional uid */
} else if (entry->length >= 16 && lsapic->uid == acpi_id) {
*apic_id = lsapic->uid;
return 1;
}
Note that precedence was given to matching ASL Processor statement based
declarations over ASL Device statement declarations (this precedence is
the opposite of what was used in 'acpi_processor_get_info()').
If a 'processor_id' member match to the 'acpi_id' occurs, it is assigned
to 'apic_id' and used to obtain the CPU's Logical ID. This mapping
utilizes the 'ia64_cpu_to_sapicid[]' table created earler in the
kernel's boot processing. Each possible CPU is checked to see if it's
Physical ID matches the 'apic_id'.
for_each_possible_cpu(i) {
if (cpu_physical_id(i) == apic_id)
return i;
}
'cpu_physical_id' is an alias for the ia64_cpu_to_sapic[]' table -
#define cpu_physical_id(i) ia64_cpu_to_sapicid[i]):
So, there are at least three issues here:
* Both Processor statement and Device statement processor
declarations get conflated to 'ACPI0007'. Code then later tries
to make the distinction bases solely on whether or not the
processor declaration's scope has a _UID object (i.e. Processor
statement declarations can have a _UID within the processor's
scope!). This logic is not strict enough - it is *not*
comparing the correct components which is dependent on the
processor's declaration type.
* When mapping the 'acpi_id' the precidence is reversed from
before (i.e. initially precidence is given to _UID, subsequently
in the mapping it is not. Actually this is pretty much a mute
point in that this type of logic is not strict enough as covered
above).
* In the mapping - map_lsapic_id() - the fall-through path
incorrectly returns the Local SAPIC _UID value and *not*
'(lsapic->id << 8) | lsapic->eid' and so the subsequent mapping
at the end of 'get_cpu_id' against the table set up at early
boot will *not* match (or worse - a false positive can occur).
Basically what is occuring is the table 'ia64_cpu_to_sapicid[]' was
correctly calculated using ID:EID and, in the case of Device statement
declarations, the processor's _UID value is being used trying to
match/map to the ID:EID value.
As a concrete example, using our new system's FW, the MADT - when
decoded - is basically:
LogicalID PhysicalID (ID:EID) _UID
----------------------------------------------------------
arch_cpu_to_apicid[00] 0x0000 0x00
arch_cpu_to_apicid[01] 0x0100 0x01
arch_cpu_to_apicid[02] 0x0200 0x02
arch_cpu_to_apicid[03] 0x0300 0x03
arch_cpu_to_apicid[04] 0x1000 0x04
arch_cpu_to_apicid[05] 0x1100 0x05
arch_cpu_to_apicid[06] 0x1200 0x06
arch_cpu_to_apicid[07] 0x1300 0x07
arch_cpu_to_apicid[08] 0x0002 0x10
arch_cpu_to_apicid[09] 0x0102 0x11
arch_cpu_to_apicid[0A] 0x0202 0x12
arch_cpu_to_apicid[0B] 0x0302 0x13
arch_cpu_to_apicid[0C] 0x1002 0x14
arch_cpu_to_apicid[0D] 0x1102 0x15
arch_cpu_to_apicid[0E] 0x1202 0x16
arch_cpu_to_apicid[0F] 0x1302 0x17
During early boot processing both the 'smp_boot_data' and
'ia64_cpu_to_sapic[]' table are initialized. Both tables end up with a
mapping of Logical Processor ID to Physical {S}APIC ID as shown above
(at this point ignore the _UID column). In example, Logical CPU 0x0B's
entry contains the processor's correspoding Local APIC ID of 0x0302.
Next, 'acpi_processor_get_info()' is called with it's second argument
indicating the processor object contains a _UID method (the implication
being that the ACPI namespace processor declaration used the AML Device
statement [is this necessarly true - i.e. can a processor statement have
a _UID method within it's scope?]). 'acpi_processor_get_info()' gives
precedence to the _UID and so calls 'get_cpu_id()' with the processor's
_UID as the 'acpi_id'. Continuing the example, 'get_cpu_id()' is called
on behalf of Logical processor 0xB with a _UID value of 0x13.
Device (P00B)
{
Name (_HID, "ACPI0007")
Name (_UID, 0x00000013)
...
Method (_MAT, 0, NotSerialized)
{
Return (Buffer (0x10)
{
0x07, 0x10, 0x00, 0x03, 0x02, 0x00, 0x00, 0x00,
0x01, 0x00, 0x00, 0x00, 0x13, 0x00, 0x00, 0x00
})
}
'get_cpu_id()' attempts to map the 'acpi_id' of 0x13 to the
corresponding _MAT entries PhysicalID - 0x0302 - which does not match.
Next an attempt to map the 'acpi_id' to the _MAT entries _UID - 0x13 -
which does match so 'apic_id' is set to 0x13. The final step of mapping
the 'apic_id' to a Logical CPU ID is attempted by parsing all the
possible CPUS via the 'arch_cpu_to_apicid[]' table entries is made but
no 'arch_cpu_to_apicid[]' entry value is 0x13!
Note that a "false positive" can be obtained - follow the same steps
using the values corresponding to Logical CPU 0x02 whose _UID value ix
0x2. The code will produce an incorrect Logical CPU value of 0x08!
>
> Of course we should add such enhancement.
>
> > "Device" without a _UID child object
> > "Device" with a _UID child object (which our systems now have)
> > In the "Processor" declarations the match to the Local SAPIC is based on
> > the 'ProcessorID' value regardless of whether or not there is a _UID
> > child object. For "Device" declarations, the match to the Local SAPIC
> > is based on the '_UID' of the child object - so the third case above
> > ("Device" without a _UID child object) would be illegal.
> >
> >
> > This patch separates the type of CPU declaration that was encountered in
> > the namespace (the current code conflated them into a single #define).
> > The separation enables the mapping logic, that is done later, know
> > explicitly which CPU declaration type was used so that it can use the
> > proper field - 'ProcessorID' or '_UID' for the association.
> >
> > If you do not agree with this interpretation of the spec then please let
> > me know where you believe I am wrong.
> >
> > Myron
> > >
> > > >
> > > > This patch disambiguates the processor declaration's definition type enabling
> > > > subsequent code to behave uniquely based explicitly on the declaration's type.
> > > >
> > > > Signed-off-by: Myron Stowe <myron.stowe@xxxxxx>
> > > > CC: Alexey Starikovskiy <aystarik@xxxxxxxxx>
> > > >
> > > > ---
> > > > drivers/acpi/processor_core.c | 1 +
> > > > drivers/acpi/scan.c | 2 +-
> > > > include/acpi/acpi_drivers.h | 1 +
> > > > 3 files changed, 3 insertions(+), 1 deletion(-)
> > > >
> > > > Index: linux-2.6/drivers/acpi/processor_core.c
> > > > ===================================================================
> > > > --- linux-2.6.orig/drivers/acpi/processor_core.c 2008-10-16 19:00:37.000000000 -0600
> > > > +++ linux-2.6/drivers/acpi/processor_core.c 2008-10-21 13:11:00.000000000 -0600
> > > > @@ -89,6 +89,7 @@ static int acpi_processor_handle_eject(s
> > > >
> > > >
> > > > static const struct acpi_device_id processor_device_ids[] = {
> > > > + {ACPI_PROCESSOR_OBJECT_HID, 0},
> > > > {ACPI_PROCESSOR_HID, 0},
> > > > {"", 0},
> > > > };
> > > > Index: linux-2.6/include/acpi/acpi_drivers.h
> > > > ===================================================================
> > > > --- linux-2.6.orig/include/acpi/acpi_drivers.h 2008-10-16 18:53:26.000000000 -0600
> > > > +++ linux-2.6/include/acpi/acpi_drivers.h 2008-10-20 13:23:28.000000000 -0600
> > > > @@ -41,6 +41,7 @@
> > > > */
> > > >
> > > > #define ACPI_POWER_HID "LNXPOWER"
> > > > +#define ACPI_PROCESSOR_OBJECT_HID "ACPI_CPU"
> > > > #define ACPI_PROCESSOR_HID "ACPI0007"
> > > > #define ACPI_SYSTEM_HID "LNXSYSTM"
> > > > #define ACPI_THERMAL_HID "LNXTHERM"
> > > > Index: linux-2.6/drivers/acpi/scan.c
> > > > ===================================================================
> > > > --- linux-2.6.orig/drivers/acpi/scan.c 2008-10-16 19:04:58.000000000 -0600
> > > > +++ linux-2.6/drivers/acpi/scan.c 2008-10-21 13:09:09.000000000 -0600
> > > > @@ -1025,7 +1025,7 @@ static void acpi_device_set_id(struct ac
> > > > hid = ACPI_POWER_HID;
> > > > break;
> > > > case ACPI_BUS_TYPE_PROCESSOR:
> > > > - hid = ACPI_PROCESSOR_HID;
> > > > + hid = ACPI_PROCESSOR_OBJECT_HID;
> > > > break;
> > > > case ACPI_BUS_TYPE_SYSTEM:
> > > > hid = ACPI_SYSTEM_HID;
> > > >
> > > >
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> > > > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > >
>
--
Myron Stowe HP Open Source & Linux Org
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
