| Message ID | 1530289077-2444-1-git-send-email-sudeep.holla@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | ACPI/PPTT: use ACPI ID whenever ACPI_PPTT_ACPI_PROCESSOR_ID_VALID is set | expand |
Hi, On 06/29/2018 11:17 AM, Sudeep Holla wrote: > Currently we use the ACPI processor ID only for the leaf/processor nodes > as the specification states it must match the value of ACPI processor ID > field in the processor’s entry in the MADT. > > However, if a PPTT structure represents processors group, it match a > processor container UID in the namespace and ACPI_PPTT_ACPI_PROCESSOR_ID_VALID > flag describe whether the ACPI processor ID is valid. > > Lets use UID whenever ACPI_PPTT_ACPI_PROCESSOR_ID_VALID is set to be > consistent instead of using table offset as it's currently done for non > leaf nodes. > > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > --- > drivers/acpi/pptt.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > Hi, > > There's ongoing discussion on assigning ID based in OS using simple > counters. It can never be consistent with firmware's view. So if the > firmware provides valid UID for non-processors node, we must use it. > > Regards, > Sudeep > > diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c > index e5ea1974d1e3..d1e26cb599bf 100644 > --- a/drivers/acpi/pptt.c > +++ b/drivers/acpi/pptt.c > @@ -481,8 +481,14 @@ static int topology_get_acpi_cpu_tag(struct acpi_table_header *table, > if (cpu_node) { > cpu_node = acpi_find_processor_package_id(table, cpu_node, > level, flag); > - /* Only the first level has a guaranteed id */ > - if (level == 0) > + /* > + * As per specification if the processor structure represents > + * an actual processor, then ACPI processor ID must be valid. > + * For processor containers ACPI_PPTT_ACPI_PROCESSOR_ID_VALID > + * should be set if the UID is valid > + */ > + if (level == 0 || > + cpu_node->flags & ACPI_PPTT_ACPI_PROCESSOR_ID_VALID) > return cpu_node->acpi_processor_id; While, for some machines this likely helps create more human readable ID's... What happens when the ID namespaces conflict with each other? AKA, I'm a little shy of this change because your going from something we can guarantee is unique to depending on an portion of the PPTT definition that has a couple different ways that it can be interpreted. OTOH the change is probably safe at the moment because i don't think anyone has partially marked nodes at a given PPTT "level" valid, or put structures that aren't part of the PE/cache's in the tree (outside of my juno test tree with the GPU's/etc). -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jun 29, 2018 at 05:17:57PM +0100, Sudeep Holla wrote: > Currently we use the ACPI processor ID only for the leaf/processor nodes > as the specification states it must match the value of ACPI processor ID > field in the processor’s entry in the MADT. > > However, if a PPTT structure represents processors group, it match a > processor container UID in the namespace and ACPI_PPTT_ACPI_PROCESSOR_ID_VALID > flag describe whether the ACPI processor ID is valid. > > Lets use UID whenever ACPI_PPTT_ACPI_PROCESSOR_ID_VALID is set to be > consistent instead of using table offset as it's currently done for non > leaf nodes. > > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > --- > drivers/acpi/pptt.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > Hi, > > There's ongoing discussion on assigning ID based in OS using simple > counters. It can never be consistent with firmware's view. So if the > firmware provides valid UID for non-processors node, we must use it. I agree with this. I've been so focused on the fact that the ACPI offsets are arbitrary, and thus counters can't be worse, that I nearly forgot how these IDs are actually defined: From Documentation/cputopology.txt """ 1) /sys/devices/system/cpu/cpuX/topology/physical_package_id: physical package id of cpuX. Typically corresponds to a physical socket number, but the actual value is architecture and platform dependent. 2) /sys/devices/system/cpu/cpuX/topology/core_id: the CPU core ID of cpuX. Typically it is the hardware platform's identifier (rather than the kernel's). The actual value is architecture and platform dependent. """ So all my consistency arguments are moot, as no user should expect consistency from architecture and platform dependent IDs. A comment on the patch below > > Regards, > Sudeep > > diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c > index e5ea1974d1e3..d1e26cb599bf 100644 > --- a/drivers/acpi/pptt.c > +++ b/drivers/acpi/pptt.c > @@ -481,8 +481,14 @@ static int topology_get_acpi_cpu_tag(struct acpi_table_header *table, > if (cpu_node) { > cpu_node = acpi_find_processor_package_id(table, cpu_node, > level, flag); > - /* Only the first level has a guaranteed id */ > - if (level == 0) > + /* > + * As per specification if the processor structure represents > + * an actual processor, then ACPI processor ID must be valid. > + * For processor containers ACPI_PPTT_ACPI_PROCESSOR_ID_VALID > + * should be set if the UID is valid > + */ > + if (level == 0 || > + cpu_node->flags & ACPI_PPTT_ACPI_PROCESSOR_ID_VALID) > return cpu_node->acpi_processor_id; > return ACPI_PTR_DIFF(cpu_node, table); > } > -- > 2.7.4 > When the valid flag is set we'll now return a [hopefully] correct platform dependent ID, but when it's not we'll return an ACPI table offset. How will users of the ID know? Also, it's possible to return -ENOENT for the ID when calling find_acpi_cpu_topology(). How can we distinguish that from an arbitrary platform dependent ID? Thanks, drew -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jun 29, 2018 at 6:17 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > Currently we use the ACPI processor ID only for the leaf/processor nodes > as the specification states it must match the value of ACPI processor ID > field in the processor’s entry in the MADT. > > However, if a PPTT structure represents processors group, it match a > processor container UID in the namespace and ACPI_PPTT_ACPI_PROCESSOR_ID_VALID > flag describe whether the ACPI processor ID is valid. > > Lets use UID whenever ACPI_PPTT_ACPI_PROCESSOR_ID_VALID is set to be > consistent instead of using table offset as it's currently done for non > leaf nodes. > > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > --- > drivers/acpi/pptt.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > Hi, > > There's ongoing discussion on assigning ID based in OS using simple > counters. It can never be consistent with firmware's view. So if the > firmware provides valid UID for non-processors node, we must use it. OK Do you regard this as a fix for the recently merged PPTT material? If so, I should apply it as a fix for 4.18. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 30/06/18 08:16, Andrew Jones wrote: > On Fri, Jun 29, 2018 at 05:17:57PM +0100, Sudeep Holla wrote: >> Currently we use the ACPI processor ID only for the leaf/processor nodes >> as the specification states it must match the value of ACPI processor ID >> field in the processor’s entry in the MADT. >> >> However, if a PPTT structure represents processors group, it match a >> processor container UID in the namespace and ACPI_PPTT_ACPI_PROCESSOR_ID_VALID >> flag describe whether the ACPI processor ID is valid. >> >> Lets use UID whenever ACPI_PPTT_ACPI_PROCESSOR_ID_VALID is set to be >> consistent instead of using table offset as it's currently done for non >> leaf nodes. >> >> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> >> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> >> --- >> drivers/acpi/pptt.c | 10 ++++++++-- >> 1 file changed, 8 insertions(+), 2 deletions(-) >> >> Hi, >> >> There's ongoing discussion on assigning ID based in OS using simple >> counters. It can never be consistent with firmware's view. So if the >> firmware provides valid UID for non-processors node, we must use it. > > I agree with this. I've been so focused on the fact that the ACPI offsets > are arbitrary, and thus counters can't be worse, that I nearly forgot how > these IDs are actually defined: > Yes, it's platform dependent and I now realize that I never explicitly mentioned that, let alone emphasize on that. I was for UID for the same reason. [...] > > When the valid flag is set we'll now return a [hopefully] correct platform > dependent ID, but when it's not we'll return an ACPI table offset. How > will users of the ID know? Also, it's possible to return -ENOENT for the > ID when calling find_acpi_cpu_topology(). How can we distinguish that from > an arbitrary platform dependent ID? > But why should one need to distinguish that ? Even offset is kind of sparsely distributed UID. -- Regards, Sudeep -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 29/06/18 19:18, Jeremy Linton wrote: > Hi, > > On 06/29/2018 11:17 AM, Sudeep Holla wrote: >> Currently we use the ACPI processor ID only for the leaf/processor nodes >> as the specification states it must match the value of ACPI processor ID >> field in the processor’s entry in the MADT. >> >> However, if a PPTT structure represents processors group, it match a >> processor container UID in the namespace and >> ACPI_PPTT_ACPI_PROCESSOR_ID_VALID >> flag describe whether the ACPI processor ID is valid. >> >> Lets use UID whenever ACPI_PPTT_ACPI_PROCESSOR_ID_VALID is set to be >> consistent instead of using table offset as it's currently done for non >> leaf nodes. >> >> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> >> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> >> --- >> drivers/acpi/pptt.c | 10 ++++++++-- >> 1 file changed, 8 insertions(+), 2 deletions(-) >> >> Hi, >> >> There's ongoing discussion on assigning ID based in OS using simple >> counters. It can never be consistent with firmware's view. So if the >> firmware provides valid UID for non-processors node, we must use it. >> >> Regards, >> Sudeep >> >> diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c >> index e5ea1974d1e3..d1e26cb599bf 100644 >> --- a/drivers/acpi/pptt.c >> +++ b/drivers/acpi/pptt.c >> @@ -481,8 +481,14 @@ static int topology_get_acpi_cpu_tag(struct >> acpi_table_header *table, >> if (cpu_node) { >> cpu_node = acpi_find_processor_package_id(table, cpu_node, >> level, flag); >> - /* Only the first level has a guaranteed id */ >> - if (level == 0) >> + /* >> + * As per specification if the processor structure represents >> + * an actual processor, then ACPI processor ID must be valid. >> + * For processor containers ACPI_PPTT_ACPI_PROCESSOR_ID_VALID >> + * should be set if the UID is valid >> + */ >> + if (level == 0 || >> + cpu_node->flags & ACPI_PPTT_ACPI_PROCESSOR_ID_VALID) >> return cpu_node->acpi_processor_id; > > While, for some machines this likely helps create more human readable > ID's... What happens when the ID namespaces conflict with each other? > That's entirely left to the platform firmware. It should help userspace to identify the topology in a way firmware is describing and no more than that. If users use them for anything more, it's at their own risk. > AKA, I'm a little shy of this change because your going from something > we can guarantee is unique to depending on an portion of the PPTT > definition that has a couple different ways that it can be interpreted. > No, I am not guaranteeing anything here. I am just passing valid UID if present to the caller. Interpretation is left to the caller and in ARM64 we should just use(at least my preference) the value as is for sysfs topology. > OTOH the change is probably safe at the moment because i don't think > anyone has partially marked nodes at a given PPTT "level" valid, or put > structures that aren't part of the PE/cache's in the tree (outside of my > juno test tree with the GPU's/etc). > Even if they are present, I don't see issue. If that's how firmware presents the CPU topology, that should be exactly the way we too need to. -- Regards, Sudeep -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 2, 2018 at 11:57 AM, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > On 02/07/18 10:06, Rafael J. Wysocki wrote: >> On Fri, Jun 29, 2018 at 6:17 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: >>> Currently we use the ACPI processor ID only for the leaf/processor nodes >>> as the specification states it must match the value of ACPI processor ID >>> field in the processor’s entry in the MADT. >>> >>> However, if a PPTT structure represents processors group, it match a >>> processor container UID in the namespace and ACPI_PPTT_ACPI_PROCESSOR_ID_VALID >>> flag describe whether the ACPI processor ID is valid. >>> >>> Lets use UID whenever ACPI_PPTT_ACPI_PROCESSOR_ID_VALID is set to be >>> consistent instead of using table offset as it's currently done for non >>> leaf nodes. >>> >>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> >>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> >>> --- >>> drivers/acpi/pptt.c | 10 ++++++++-- >>> 1 file changed, 8 insertions(+), 2 deletions(-) >>> >>> Hi, >>> >>> There's ongoing discussion on assigning ID based in OS using simple >>> counters. It can never be consistent with firmware's view. So if the >>> firmware provides valid UID for non-processors node, we must use it. >> >> OK >> >> Do you regard this as a fix for the recently merged PPTT material? If >> so, I should apply it as a fix for 4.18. >> > > Yes, it should be considered as fix IMO. So any chance to provide a Fixes: tag? -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/07/18 12:01, Rafael J. Wysocki wrote: > On Mon, Jul 2, 2018 at 11:57 AM, Sudeep Holla <sudeep.holla@arm.com> wrote: >> >> >> On 02/07/18 10:06, Rafael J. Wysocki wrote: >>> On Fri, Jun 29, 2018 at 6:17 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: >>>> Currently we use the ACPI processor ID only for the leaf/processor nodes >>>> as the specification states it must match the value of ACPI processor ID >>>> field in the processor’s entry in the MADT. >>>> >>>> However, if a PPTT structure represents processors group, it match a >>>> processor container UID in the namespace and ACPI_PPTT_ACPI_PROCESSOR_ID_VALID >>>> flag describe whether the ACPI processor ID is valid. >>>> >>>> Lets use UID whenever ACPI_PPTT_ACPI_PROCESSOR_ID_VALID is set to be >>>> consistent instead of using table offset as it's currently done for non >>>> leaf nodes. >>>> >>>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> >>>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> >>>> --- >>>> drivers/acpi/pptt.c | 10 ++++++++-- >>>> 1 file changed, 8 insertions(+), 2 deletions(-) >>>> >>>> Hi, >>>> >>>> There's ongoing discussion on assigning ID based in OS using simple >>>> counters. It can never be consistent with firmware's view. So if the >>>> firmware provides valid UID for non-processors node, we must use it. >>> >>> OK >>> >>> Do you regard this as a fix for the recently merged PPTT material? If >>> so, I should apply it as a fix for 4.18. >>> >> >> Yes, it should be considered as fix IMO. > > So any chance to provide a Fixes: tag? Sure, since it was in the same release, I didn't add it. Fixes: 2bd00bcd73e5 ("ACPI/PPTT: Add Processor Properties Topology Table parsing") -- Regards, Sudeep -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 06/29/2018 11:17 AM, Sudeep Holla wrote: > Currently we use the ACPI processor ID only for the leaf/processor nodes > as the specification states it must match the value of ACPI processor ID > field in the processor’s entry in the MADT. > > However, if a PPTT structure represents processors group, it match a > processor container UID in the namespace and ACPI_PPTT_ACPI_PROCESSOR_ID_VALID > flag describe whether the ACPI processor ID is valid. > > Lets use UID whenever ACPI_PPTT_ACPI_PROCESSOR_ID_VALID is set to be > consistent instead of using table offset as it's currently done for non > leaf nodes. > > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > --- > drivers/acpi/pptt.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > Hi, > > There's ongoing discussion on assigning ID based in OS using simple > counters. It can never be consistent with firmware's view. So if the > firmware provides valid UID for non-processors node, we must use it. > > Regards, > Sudeep > > diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c > index e5ea1974d1e3..d1e26cb599bf 100644 > --- a/drivers/acpi/pptt.c > +++ b/drivers/acpi/pptt.c > @@ -481,8 +481,14 @@ static int topology_get_acpi_cpu_tag(struct acpi_table_header *table, > if (cpu_node) { > cpu_node = acpi_find_processor_package_id(table, cpu_node, > level, flag); > - /* Only the first level has a guaranteed id */ > - if (level == 0) > + /* > + * As per specification if the processor structure represents > + * an actual processor, then ACPI processor ID must be valid. > + * For processor containers ACPI_PPTT_ACPI_PROCESSOR_ID_VALID > + * should be set if the UID is valid > + */ > + if (level == 0 || > + cpu_node->flags & ACPI_PPTT_ACPI_PROCESSOR_ID_VALID) > return cpu_node->acpi_processor_id; > return ACPI_PTR_DIFF(cpu_node, table); > } > Ok sure, Acked-by: Jeremy Linton <jeremy.linton@arm.com> PS: To table implementers, the spec today mandates that setting the valid flag on a non leaf node means there is a matching _UID processor container in DSDT/SSDT. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday, July 2, 2018 2:52:24 PM CEST Sudeep Holla wrote: > > On 02/07/18 12:01, Rafael J. Wysocki wrote: > > On Mon, Jul 2, 2018 at 11:57 AM, Sudeep Holla <sudeep.holla@arm.com> wrote: > >> > >> > >> On 02/07/18 10:06, Rafael J. Wysocki wrote: > >>> On Fri, Jun 29, 2018 at 6:17 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > >>>> Currently we use the ACPI processor ID only for the leaf/processor nodes > >>>> as the specification states it must match the value of ACPI processor ID > >>>> field in the processor’s entry in the MADT. > >>>> > >>>> However, if a PPTT structure represents processors group, it match a > >>>> processor container UID in the namespace and ACPI_PPTT_ACPI_PROCESSOR_ID_VALID > >>>> flag describe whether the ACPI processor ID is valid. > >>>> > >>>> Lets use UID whenever ACPI_PPTT_ACPI_PROCESSOR_ID_VALID is set to be > >>>> consistent instead of using table offset as it's currently done for non > >>>> leaf nodes. > >>>> > >>>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > >>>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > >>>> --- > >>>> drivers/acpi/pptt.c | 10 ++++++++-- > >>>> 1 file changed, 8 insertions(+), 2 deletions(-) > >>>> > >>>> Hi, > >>>> > >>>> There's ongoing discussion on assigning ID based in OS using simple > >>>> counters. It can never be consistent with firmware's view. So if the > >>>> firmware provides valid UID for non-processors node, we must use it. > >>> > >>> OK > >>> > >>> Do you regard this as a fix for the recently merged PPTT material? If > >>> so, I should apply it as a fix for 4.18. > >>> > >> > >> Yes, it should be considered as fix IMO. > > > > So any chance to provide a Fixes: tag? > > Sure, since it was in the same release, I didn't add it. > > Fixes: 2bd00bcd73e5 ("ACPI/PPTT: Add Processor Properties Topology Table > parsing") > > Applied now, thanks! -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c index e5ea1974d1e3..d1e26cb599bf 100644 --- a/drivers/acpi/pptt.c +++ b/drivers/acpi/pptt.c @@ -481,8 +481,14 @@ static int topology_get_acpi_cpu_tag(struct acpi_table_header *table, if (cpu_node) { cpu_node = acpi_find_processor_package_id(table, cpu_node, level, flag); - /* Only the first level has a guaranteed id */ - if (level == 0) + /* + * As per specification if the processor structure represents + * an actual processor, then ACPI processor ID must be valid. + * For processor containers ACPI_PPTT_ACPI_PROCESSOR_ID_VALID + * should be set if the UID is valid + */ + if (level == 0 || + cpu_node->flags & ACPI_PPTT_ACPI_PROCESSOR_ID_VALID) return cpu_node->acpi_processor_id; return ACPI_PTR_DIFF(cpu_node, table); }
Currently we use the ACPI processor ID only for the leaf/processor nodes as the specification states it must match the value of ACPI processor ID field in the processor’s entry in the MADT. However, if a PPTT structure represents processors group, it match a processor container UID in the namespace and ACPI_PPTT_ACPI_PROCESSOR_ID_VALID flag describe whether the ACPI processor ID is valid. Lets use UID whenever ACPI_PPTT_ACPI_PROCESSOR_ID_VALID is set to be consistent instead of using table offset as it's currently done for non leaf nodes. Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> --- drivers/acpi/pptt.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) Hi, There's ongoing discussion on assigning ID based in OS using simple counters. It can never be consistent with firmware's view. So if the firmware provides valid UID for non-processors node, we must use it. Regards, Sudeep -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html