diff mbox series

[3/4] acpi: numa: Add setting of generic port system locality attributes

Message ID 168333152832.2290593.17409054392013117865.stgit@djiang5-mobl3
State Superseded
Headers show
Series acpi: numa: add target support for generic port to HMAT parsing | expand

Commit Message

Dave Jiang May 6, 2023, 12:05 a.m. UTC 
Add generic port support for the parsing of HMAT system locality sub-table.
The attributes will be added to the third array member of the access
coordinates in order to not mix with the existing memory attributes it only
provides the system locality attributes from initator to the generic port
targets and is missing the rest of the data to the actual memory device.

The complete attributes will be updated when a memory device is
attached and the system locality information is calculated end to end.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/acpi/numa/hmat.c |    7 +++++++
 1 file changed, 7 insertions(+)

Comments

Jonathan Cameron May 12, 2023, 4:10 p.m. UTC | #1 
On Fri, 05 May 2023 17:05:28 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> Add generic port support for the parsing of HMAT system locality sub-table.
> The attributes will be added to the third array member of the access
> coordinates in order to not mix with the existing memory attributes it only

attributes. It only ?
Otherwise I'm having trouble following that sentence.

> provides the system locality attributes from initator to the generic port
> targets and is missing the rest of the data to the actual memory device.
> 
> The complete attributes will be updated when a memory device is
> attached and the system locality information is calculated end to end.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/acpi/numa/hmat.c |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
> index e2ab1cce0add..951579e903cf 100644
> --- a/drivers/acpi/numa/hmat.c
> +++ b/drivers/acpi/numa/hmat.c
> @@ -60,6 +60,7 @@ struct target_cache {
>  enum {
>  	NODE_ACCESS_CLASS_0 = 0,
>  	NODE_ACCESS_CLASS_1,
> +	NODE_ACCESS_CLASS_GENPORT,
>  	NODE_ACCESS_CLASS_MAX,
>  };
>  
> @@ -368,6 +369,12 @@ static __init int hmat_parse_locality(union acpi_subtable_headers *header,
>  			if (mem_hier == ACPI_HMAT_MEMORY) {
>  				target = find_mem_target(targs[targ]);
>  				if (target && target->processor_pxm == inits[init]) {
> +					if (*target->device_handle) {
> +						hmat_update_target_access(target, type, value,
> +								NODE_ACCESS_CLASS_GENPORT);
> +						continue;

There isn't anything that I know of that says a generic port has to be in it's own PXM.

In theory at least they could share one with CPUs or Memory.
Sure the access info will be inaccurate, but meh, it's a firmware table and if the spec
doesn't exclude it then someone might build it.

So rather than continuing here I think you need to add a check on there being memory
or not before deciding not to update the other access classes.

Jonathan


> +					}
> +
>  					hmat_update_target_access(target, type, value,
>  								  NODE_ACCESS_CLASS_0);
>  					/* If the node has a CPU, update access 1 */
> 
>
Jonathan Cameron May 12, 2023, 4:16 p.m. UTC | #2 
On Fri, 05 May 2023 17:05:28 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> Add generic port support for the parsing of HMAT system locality sub-table.
> The attributes will be added to the third array member of the access
> coordinates in order to not mix with the existing memory attributes it only
> provides the system locality attributes from initator to the generic port
> targets and is missing the rest of the data to the actual memory device.
> 
> The complete attributes will be updated when a memory device is
> attached and the system locality information is calculated end to end.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

Strange question for you.  For now we have it easy as we only have Type 3
CXL devices using this.  Later when we have accelerators, then the generic
port becomes both an initiator and a target (well proxy of one in both cases).
We'll probably want to map the linux view of a generic initiator (CPU less
node) on top of the accelerator using the access characteristics from HMAT +
other parts.

Any thoughts on how to extend this approach to that case?

I can see it might need another access class so that normal memory for example
can be the target of a generic port.  

Maybe it's worth predicting that and renaming CLASS_GENPORT to CLASS_GENPORT_SINK
or something along those lines?

Or just leave it until someone cares?  I'm fine with this answer if you agree ;)

Jonathan

> ---
>  drivers/acpi/numa/hmat.c |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
> index e2ab1cce0add..951579e903cf 100644
> --- a/drivers/acpi/numa/hmat.c
> +++ b/drivers/acpi/numa/hmat.c
> @@ -60,6 +60,7 @@ struct target_cache {
>  enum {
>  	NODE_ACCESS_CLASS_0 = 0,
>  	NODE_ACCESS_CLASS_1,
> +	NODE_ACCESS_CLASS_GENPORT,
>  	NODE_ACCESS_CLASS_MAX,
>  };
>  
> @@ -368,6 +369,12 @@ static __init int hmat_parse_locality(union acpi_subtable_headers *header,
>  			if (mem_hier == ACPI_HMAT_MEMORY) {
>  				target = find_mem_target(targs[targ]);
>  				if (target && target->processor_pxm == inits[init]) {
> +					if (*target->device_handle) {
> +						hmat_update_target_access(target, type, value,
> +								NODE_ACCESS_CLASS_GENPORT);
> +						continue;
> +					}
> +
>  					hmat_update_target_access(target, type, value,
>  								  NODE_ACCESS_CLASS_0);
>  					/* If the node has a CPU, update access 1 */
> 
>
Dave Jiang May 12, 2023, 4:22 p.m. UTC | #3 
On 5/12/23 9:16 AM, Jonathan Cameron wrote:
> On Fri, 05 May 2023 17:05:28 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> Add generic port support for the parsing of HMAT system locality sub-table.
>> The attributes will be added to the third array member of the access
>> coordinates in order to not mix with the existing memory attributes it only
>> provides the system locality attributes from initator to the generic port
>> targets and is missing the rest of the data to the actual memory device.
>>
>> The complete attributes will be updated when a memory device is
>> attached and the system locality information is calculated end to end.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> 
> Strange question for you.  For now we have it easy as we only have Type 3
> CXL devices using this.  Later when we have accelerators, then the generic
> port becomes both an initiator and a target (well proxy of one in both cases).
> We'll probably want to map the linux view of a generic initiator (CPU less
> node) on top of the accelerator using the access characteristics from HMAT +
> other parts.
> 
> Any thoughts on how to extend this approach to that case?

I have not given much thought on it. But I'll think on it.

> 
> I can see it might need another access class so that normal memory for example
> can be the target of a generic port.
> 
> Maybe it's worth predicting that and renaming CLASS_GENPORT to CLASS_GENPORT_SINK
> or something along those lines?

Sure. We can name it to something more specific with an eye on the 
future. But we can deal with the rest when the time comes.

> 
> Or just leave it until someone cares?  I'm fine with this answer if you agree ;)
> 
> Jonathan
> 
>> ---
>>   drivers/acpi/numa/hmat.c |    7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
>> index e2ab1cce0add..951579e903cf 100644
>> --- a/drivers/acpi/numa/hmat.c
>> +++ b/drivers/acpi/numa/hmat.c
>> @@ -60,6 +60,7 @@ struct target_cache {
>>   enum {
>>   	NODE_ACCESS_CLASS_0 = 0,
>>   	NODE_ACCESS_CLASS_1,
>> +	NODE_ACCESS_CLASS_GENPORT,
>>   	NODE_ACCESS_CLASS_MAX,
>>   };
>>   
>> @@ -368,6 +369,12 @@ static __init int hmat_parse_locality(union acpi_subtable_headers *header,
>>   			if (mem_hier == ACPI_HMAT_MEMORY) {
>>   				target = find_mem_target(targs[targ]);
>>   				if (target && target->processor_pxm == inits[init]) {
>> +					if (*target->device_handle) {
>> +						hmat_update_target_access(target, type, value,
>> +								NODE_ACCESS_CLASS_GENPORT);
>> +						continue;
>> +					}
>> +
>>   					hmat_update_target_access(target, type, value,
>>   								  NODE_ACCESS_CLASS_0);
>>   					/* If the node has a CPU, update access 1 */
>>
>>
>
diff mbox series

Patch

diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
index e2ab1cce0add..951579e903cf 100644
--- a/drivers/acpi/numa/hmat.c
+++ b/drivers/acpi/numa/hmat.c
@@ -60,6 +60,7 @@  struct target_cache {
 enum {
 	NODE_ACCESS_CLASS_0 = 0,
 	NODE_ACCESS_CLASS_1,
+	NODE_ACCESS_CLASS_GENPORT,
 	NODE_ACCESS_CLASS_MAX,
 };
 
@@ -368,6 +369,12 @@  static __init int hmat_parse_locality(union acpi_subtable_headers *header,
 			if (mem_hier == ACPI_HMAT_MEMORY) {
 				target = find_mem_target(targs[targ]);
 				if (target && target->processor_pxm == inits[init]) {
+					if (*target->device_handle) {
+						hmat_update_target_access(target, type, value,
+								NODE_ACCESS_CLASS_GENPORT);
+						continue;
+					}
+
 					hmat_update_target_access(target, type, value,
 								  NODE_ACCESS_CLASS_0);
 					/* If the node has a CPU, update access 1 */