Re: [RFC PATCH 4/6] acpi/hmat: Add helper functions to provide extended linear cache translation

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Fri, 27 Sep 2024 07:16:56 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> Add helper functions to help do address translation for either the address
> of the extended linear cache or its alias address. The translation function
> attempt to detect an I/O hole in the proximity domain and adjusts the address
> if the hole impacts the aliasing of the address. The range of the I/O hole
> is retrieved by walking through the associated memory target resources.

What does the I/O hole correspond to in the system?

> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/acpi/numa/hmat.c | 136 +++++++++++++++++++++++++++++++++++++++
>  include/linux/acpi.h     |  14 ++++
>  2 files changed, 150 insertions(+)
> 
> diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
> index d299f8d7af8c..834314582f4c 100644
> --- a/drivers/acpi/numa/hmat.c
> +++ b/drivers/acpi/numa/hmat.c
> @@ -152,6 +152,142 @@ int hmat_get_extended_linear_cache_size(struct resource *backing_res, int nid,
>  }
>  EXPORT_SYMBOL_NS_GPL(hmat_get_extended_linear_cache_size, CXL);
>  
> +static int alias_address_find_iohole(struct memory_target *target,
> +				     u64 address, u64 alias, struct range *hole)
> +{
> +	struct resource *alias_res = NULL;
> +	struct resource *res, *prev;
> +
> +	*hole = (struct range) {
> +		.start = 0,
> +		.end = -1,
> +	};
> +
> +	/* First find the resource that the address is in */
> +	prev = target->memregions.child;
> +	for (res = target->memregions.child; res; res = res->sibling) {
> +		if (alias >= res->start && alias <= res->end) {
> +			alias_res = res;
> +			break;
> +		}
> +		prev = res;
> +	}
> +	if (!alias_res)

	if (!res) and you can just use res instead of alias_res for the following
as you exit the loop with it set to the right value.



> +		return -EINVAL;
> +
> +	/* No memory hole */
> +	if (alias_res == prev)
> +		return 0;
> +
> +	/* If address is within the current resource, no need to deal with memory hole */
> +	if (address >= alias_res->start)
> +		return 0;
> +
> +	*hole = (struct range) {
> +		.start = prev->end + 1,
> +		.end = alias_res->start - 1,
> +	};
Ordering assumption should be avoided in such a generic
sounding function.  Can the hole be first?

or rename the function to include preceding_hole or something like that.
> +
> +	return 0;
> +}
> +
> +int hmat_extended_linear_cache_alias_xlat(u64 address, u64 *alias, int nid)
> +{
> +	unsigned int pxm = node_to_pxm(nid);
> +	struct memory_target *target;
> +	struct range iohole;
> +	int rc;
> +
> +	target = find_mem_target(pxm);
> +	if (!target)
> +		return -EINVAL;
> +
> +	rc = alias_address_find_iohole(target, address, *alias, &iohole);
> +	if (rc)
> +		return rc;
> +
> +	if (!range_len(&iohole))
> +		return 0;
> +
Maybe reformat like this and add comments on each condition.

	if (address >= iohole.start)
		return 0;

	if (*alias <= iohole.start)
		return 0;

	*alias += range_len(&iohole);

	return 0;

	
> +	if (address < iohole.start) {
> +		if (*alias > iohole.start) {
> +			*alias = *alias + range_len(&iohole);
> +			return 0;
> +		}
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(hmat_extended_linear_cache_alias_xlat, CXL);
> +
> +static int target_address_find_iohole(struct memory_target *target,
> +				      u64 address, u64 alias,
> +				      struct range *hole)
> +{
> +	struct resource *addr_res = NULL;
> +	struct resource *res, *next;
> +
> +	*hole = (struct range) {
> +		.start = 0,
> +		.end = -1,
> +	};
> +
> +	/* First find the resource that the address is in */
> +	for (res = target->memregions.child; res; res = res->sibling) {
> +		if (address >= res->start && address <= res->end) {
> +			addr_res = res;

Could just use res as it's scope is outside the loop.

> +			break;
> +		}
> +	}
> +	if (!addr_res)
> +		return -EINVAL;
> +
> +	next = res->sibling;
> +	/* No memory hole after the region */
> +	if (!next)
> +		return 0;
> +
> +	/* If alias is within the current resource, no need to deal with memory hole */
> +	if (alias <= addr_res->end)
> +		return 0;
> +
> +	*hole = (struct range) {
> +		.start = addr_res->end + 1,
> +		.end = next->start - 1,
> +	};
> +
> +	return 0;
> +}
> +
> +int hmat_extended_linear_cache_address_xlat(u64 *address, u64 alias, int nid)
> +{
> +	unsigned int pxm = node_to_pxm(nid);
> +	struct memory_target *target;
> +	struct range iohole;
> +	int rc;
> +
> +	target = find_mem_target(pxm);
> +	if (!target)
> +		return -EINVAL;
> +
> +	rc = target_address_find_iohole(target, *address, alias, &iohole);
> +	if (rc)
> +		return rc;
> +
> +	if (!range_len(&iohole))
> +		return 0;
> +

Similar to above, maybe break into multiple reasons to exit early.

> +	if (alias > iohole.end) {
> +		if (*address < iohole.end) {
> +			*address = *address - range_len(&iohole);
> +			return 0;
> +		}
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(hmat_extended_linear_cache_address_xlat, CXL);
>

Jonathan