Re: [PATCH v2 16/18] riscv: mm: Use physical memory aliases to apply PMAs

[Samuel Holland <samuel.holland@sifive.com>]

On 2025-10-10 12:04 PM, Emil Renner Berthing wrote:
> Samuel Holland wrote:
>> Hi Emil,
>>
>> Thanks for testing!
>>
>> On 2025-10-10 10:06 AM, Emil Renner Berthing wrote:
>>> Samuel Holland wrote:
> [ .. ]
>>>> +
>>>> +void __init riscv_init_memory_alias(void)
>>>> +{
>>>> +	int na = of_n_addr_cells(of_root);
>>>> +	int ns = of_n_size_cells(of_root);
>>>> +	int nc = na + ns + 2;
>>>> +	const __be32 *prop;
>>>> +	int pairs = 0;
>>>> +	int len;
>>>> +
>>>> +	prop = of_get_property(of_root, "riscv,physical-memory-regions", &len);
>>>> +	if (!prop)
>>>> +		return;
>>>> +
>>>> +	len /= sizeof(__be32);
>>>> +	for (int i = 0; len >= nc; i++, prop += nc, len -= nc) {
>>>> +		unsigned long base = of_read_ulong(prop, na);
>>>> +		unsigned long size = of_read_ulong(prop + na, ns);
>>>> +		unsigned long flags = be32_to_cpup(prop + na + ns);
>>>> +		struct memory_alias_pair *pair;
>>>> +		int alias;
>>>> +
>>>> +		/* We only care about non-coherent memory. */
>>>> +		if ((flags & PMA_ORDER_MASK) != PMA_ORDER_MEMORY || (flags & PMA_COHERENT))
>>>> +			continue;
>>>> +
>>>> +		/* The cacheable alias must be usable memory. */
>>>> +		if ((flags & PMA_CACHEABLE) &&
>>>> +		    !memblock_overlaps_region(&memblock.memory, base, size))
>>>> +			continue;
>>>> +
>>>> +		alias = FIELD_GET(PMR_ALIAS_MASK, flags);
>>>> +		if (alias) {
>>>> +			pair = NULL;
>>>> +			for (int j = 0; j < pairs; j++) {
>>>> +				if (alias == memory_alias_pairs[j].index) {
>>>> +					pair = &memory_alias_pairs[j];
>>>> +					break;
>>>> +				}
>>>> +			}
>>>> +			if (!pair)
>>>> +				continue;
>>>> +		} else {
>>>> +			/* Leave room for the null sentinel. */
>>>> +			if (pairs == ARRAY_SIZE(memory_alias_pairs) - 1)
>>>> +				continue;
>>>> +			pair = &memory_alias_pairs[pairs++];
>>>> +			pair->index = i;
>>>
>>> I think this needs to be pair->index = i + 1, so PMA_ALIAS(1) can refer to the
>>> first entry (i = 0).
>>
>> The code here is as intended. It's the PMA_ALIAS(1) in the DT that I should have
>> changed to PMA_ALIAS(0) after I removed the special first entry from the
>> riscv,physical-memory-regions property. Patch 18 also needs this fix.
> 
> Hmm.. that doesn't quite work for me though. Then the "if (alias)" above won't
> trigger with PMR_ALIAS(0) right?

Yes, you're right. My fault for trying to be clever last time, where the special
first entry meant PMR_ALIAS(0) would never be used. (And for not testing with
the same DT as I sent, since EIC7700 needs downstream DT changes to integrate
noncoherent peripherals.)

For v3, I plan to make PMR_ALIAS(0) set a flag so it will be distinct from lack
of PMR_ALIAS, and keep the indexes zero-based. For now, you should be able to
test by keeping PMR_ALIAS(1) and adding a dummy entry at the beginning (for
example by copying the first entry).

Regards,
Samuel