On 2014-2-22 19:30, Marc Zyngier wrote:
> On 2014-02-22 10:21, Hanjun Guo wrote:
>> On 2014-2-21 20:37, Sudeep Holla wrote:
>>> Hi Hanjun,
>>>
>>> (Adding MarcZ for his views on GIC)
>>>
>>> On 20/02/14 03:59, Hanjun Guo wrote:
>>>> Hi Sudeep,
>>>>
>>>> Thanks for your comments, please refer to the replies below. :)
>>>>
>>>> On 2014年02月19日 22:33, Sudeep Holla wrote:
>>>>> Hi Hanjun,
>>>>>
>>>>> On 18/02/14 16:23, Hanjun Guo wrote:
>>>>>> Get apic id from MADT or _MAT method is not implemented on arm/arm64,
>>>>>> and ACPI 5.0 introduces GIC Structure for it, so this patch introduces
>>>>>> map_gic_id() to get apic id followed the ACPI 5.0 spec.
>>>>>>
>>>>>> Signed-off-by: Hanjun Guo <hanjun.guo@xxxxxxxxxx>
>>>>>> ---
>>>>>> drivers/acpi/processor_core.c | 26 ++++++++++++++++++++++++++
>>>>>> 1 file changed, 26 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/acpi/processor_core.c b/drivers/acpi/processor_core.c
>>>>>> index 4dcf776..d316d9b 100644
>>>>>> --- a/drivers/acpi/processor_core.c
>>>>>> +++ b/drivers/acpi/processor_core.c
>>>>>> @@ -71,6 +71,27 @@ static int map_lsapic_id(struct acpi_subtable_header *entry,
>>>>>> return 0;
>>>>>> }
>>>>>>
>>>>>> +static int map_gic_id(struct acpi_subtable_header *entry,
>>>>>> + int device_declaration, u32 acpi_id, int *apic_id)
>>>>>> +{
>>>>>> + struct acpi_madt_generic_interrupt *gic =
>>>>>> + (struct acpi_madt_generic_interrupt *)entry;
>>>>>> +
>>>>>> + if (!(gic->flags & ACPI_MADT_ENABLED))
>>>>>> + return -ENODEV;
>>>>>> +
>>>>>> + /* In the GIC interrupt model, logical processors are
>>>>>> + * required to have a Processor Device object in the DSDT,
>>>>>> + * so we should check device_declaration here
>>>>>> + */
>>>>>> + if (device_declaration && (gic->uid == acpi_id)) {
>>>>>> + *apic_id = gic->gic_id;
>>>>> I have mentioned this earlier, it's not clear yet to me how does this work ?
>>>>> It needs more clarity in the form of comment here at-least as the ACPIv5.0 is
>>>>> also not so clear or explicit on how to handle this.
>>>>
>>>> Yes, I noticed your comments and had a reply for that, after a
>>>> long consideration for this, I would withdraw my previous comments
>>>> before, please refer to the comments below.
>>>>
>>>>>
>>>>> Here you are expecting gic->uid = acpi_id which is fine, while acpi_map_cpuid
>>>>> matches apic_id with cpu_physical_id(which must be MPIDR in ARM{32,64}). The
>>>>> latter imposes restriction that gic->gic_id has to be MPIDR. Does that mean we
>>>>> are imposing restriction on GIC ID to be MPIDR ? If so please document it here
>>>>> and please explain the reason behind that choice.
>>>>
>>>> On x86 and IA64, APIC/SAPIC ID is the hardware id of the logical
>>>> processor, and UID is just a unique ID to identify the processor in DSDT, it
>>>> can be any value, and even can be strings defined in ASL if I remember
>>>> that correctly.
>>>>
>>> OK, but that's not the case on ARM{32,64}. My main concern here is if we don't
>>> make this definitions clear enough, the vendors might produce ACPI tables with
>>> whatever suits them and we may end up supporting them. Since we are starting
>>> with clean slate, we can avoid getting into such situations. I will be to be
>>> more elaborate this time.
>>
>> I agree.
>>
>>>
>>> The GIC ID is referred as the local GIC’s hardware ID in ACPIv5.0.
>>> IIUC, since GICC is per-cpu entry, it has to GIC CPU interface ID.
>>>
>>> Now how does it differ from MPIDR ? e.g. ARM TC2(multi cluster system)
>>> GIC ID MPIDR Comment
>>> 0 0x000 CA15_0
>>> 1 0x001 CA15_1
>>> 2 0x100 CA7_0
>>> 3 0x101 CA7_1
>>> 4 0x102 CA7_2
>>
>> Yes, obvious different. I know GIC ID can matche the bit index of the
>> associated processor
>> in the distributor's GICD_ITARGETSR register, and it a clear
>> statement in GICv1/GICv2, my
>> question is that is this consistent in GICv3/v4 too? this will have
>> some impact on the
>> code implementation.
>
> For GICv3/v4, the only way you can match a CPU with its local redistributor is by using the CPU MPIDR. The GIC CPU ID is an implementation choice that may not exist (it doesn't in a distributed
> implementation), so anything that relies on a GIC CPU ID is broken for GICv3.
So that's the broken part, it seems that GIC ID is useless both
in GICv1/v2 and GICv3/v4 if it represents GIC CPU interface ID,
the only reliable one is the MPIDR.
I agree UID should be MPIDR for future usage, so how about this solution:
+static int map_gic_id(struct acpi_subtable_header *entry,
+ int device_declaration, u32 acpi_id, int *apic_id)
+{
+ struct acpi_madt_generic_interrupt *gic =
+ (struct acpi_madt_generic_interrupt *)entry;
+
+ if (!(gic->flags & ACPI_MADT_ENABLED))
+ return -ENODEV;
+
+ /* In the GIC interrupt model, logical processors are
+ * required to have a Processor Device object in the DSDT,
+ * so we should check device_declaration here
+ */
+ if (device_declaration && (gic->uid == acpi_id)) {
/*
* this is the special case for ARM/ARM64, gic->uid
* should be MPIDR which represents the CPU hardware
* id, so use gic->uid instead of gic->gic_id here
* to get the physical processor ID. (...)
*/
+ *apic_id = gic->uid;
+ return 0;
+ }
+
+ return -EINVAL;
+}
+
Thanks
Hanjun
--
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
