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

[Emil Renner Berthing <emil.renner.berthing@canonical.com>]

Samuel Holland wrote:
> 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).

Great that also works. To be clear the JH7100 also boots with the pair->index =
i + 1 solution above.