On 10/17/24 9:33 AM, Jonathan Cameron wrote:
> On Fri, 27 Sep 2024 07:16:56 -0700
> Dave Jiang <dave.jiang@xxxxxxxxx> 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@xxxxxxxxx>
>> ---
>> 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.
>
Ok will do that
>
>
>> + 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?
Do you mean if the address mapping starts out with an MMIO range and then memory range? I'm not sure if such an implementation exists in the x86 world. And if the hole is behind all the ranges, then it shouldn't impact the address calculations.
>
> 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;
>
>
Will change to that and add comments.
>> + 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.
Will update.
>
>> + 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.
>
Will update.
>> + 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
