Openamp cpu mask compat

[bentheredonethat]

update cpu mask handling for libmetal, openamp, gen-domain usage

[zeddii]

My review + AI is in here. The detailed questions about tests and the paths through the code are from AI, mine are the broader coments.

The design here is good. Most of the series is well scoped and genuinely bisectable.

Two things blocking:

1. Sign-off. All 16 commits are missing Signed-off-by, so DCO is red. You've signed off on everything else, so this looks like it got lost in a rebase rather than anything deliberate. Tests themselves are green on 3.12 and 3.13.

2. One real bug, inline: the cpu_refs call site passes propval("cluster_cpu") through unguarded, and that returns the [''] sentinel .. when the property is absent. The result is that the resolver can return an arbitrary CPU as a LEGACY_CLUSTER_CPU match, which is exactly what rule 5 of its own docstring promises never to happen. Interestingly you got this right in openamp_xlnx_common._openamp_legacy_domain_processor, which does guard value != [''] ... so it's an inconsistency between two call sites rather than a misunderstanding.

Smaller, non-blocking: two commits are self-described branch-history artifacts — 5cec7e70 ("This is a rebase adaptation and does not change selection semantics") and eddff10f — and would be better folded into the commits they fix up. The other 14 read well as a series and I'd keep them as-is.

One thing I checked and am happy with, so you don't have to defend it: eddff10f's "tolerate either baseline mask" wording made me expect a test that couldn't fail, but assert replacements == 1 pins the substitution, so fixture drift fails loudly. That's fine.

[zeddii]

propval() returns the [''] sentinel when the property is absent, and [''] is truthy. So when a domain-to-domain node exists but carries no cluster_cpu, this hands the resolver a non-empty label list rather than None.

Downstream that unwraps to "", and cpu.label is also "" for an unlabelled CPU node, so the membership test in resolve_domain_cpus matches on the first CPU child. Verified on a tree with an unlabelled cpu@1:

propval('cluster_cpu') when absent = ['']   truthy? True
cpu.label                          = ''
unwrapped label                    = ''
would it match a CPU?              -> cpu@1

So a domain with an out-of-range or zero mask and no cluster_cpu gets the cluster's first CPU returned as a LEGACY_CLUSTER_CPU resolution, complete with a migration warning suggesting it worked. That contradicts rule 5 in the resolver's docstring — "Otherwise report the selection as unresolved; never guess a CPU."

You already have the correct guard in the sibling helper:

# openamp_xlnx_common._openamp_legacy_domain_processor
value = dtd.propval("cluster_cpu") if dtd else [""]
return value[0] if value != [""] and value else None

Same treatment here would fix it:

legacy_cpu = dtd.propval("cluster_cpu") if dtd else None
if legacy_cpu == [""]:
    legacy_cpu = None

The new "zero and invalid masks remain unresolved" test won't catch this, because its fixture CPUs are labelled — the empty-string collision only happens when cpu.label is also ''.

[zeddii]

Related to the call-site comment, and worth fixing here too since this is the shared entry point that other callers will reach for.

The truth test on line 173 happens before the list is unwrapped on 174-175, so [''] passes the gate and only then becomes ''. By the time the membership test on 178 runs, an empty label is indistinguishable from a real one, and '' matches any node whose label is ''.

Unwrapping first and testing the unwrapped value closes it regardless of what a caller passes:

if isinstance(legacy_cpu_label, list):
    legacy_cpu_label = legacy_cpu_label[0] if legacy_cpu_label else None
if legacy_cpu_label:
    ...

That also removes the duplicated unwrap — the cluster-relative branch above does the same [0] if isinstance(..., list) dance separately, so both paths could share one normalization at the top of the function.

Given this function's contract is explicitly "never guess a CPU", I'd rather it defended itself than relied on every caller sanitizing first.

[bentheredonethat]

@zeddii Thanks for the detailed review. I’ve addressed all the comments and updated the series.

Every commit now has the required Signed-off-by trailer, and the branch-history/fixup commits have been folded into their corresponding functional commits.

I also fixed the cluster_cpu sentinel issue at both levels:

• cpu_refs() now converts the absent-property [""] value to None.
• resolve_domain_cpus() normalizes list-valued labels before checking them, so the shared resolver cannot accidentally match an unlabelled CPU.

I added a regression test covering the exact zero-mask, absent-cluster_cpu, unlabelled-CPU case. It confirms that the selection remains unresolved instead of selecting an arbitrary CPU.

The focused OpenAMP/Libmetal tests pass (26 passed, 1 skipped), as does the legacy OpenAMP sanity suite.

Thanks again—everything raised here should now be resolved.

[zeddii]

Everything from the last round is resolved:

• Sign-off — all five commits signed, DCO green.

• The [''] sentinel bug — fixed, and fixed in the right place. Normalizing at the top of resolve_domain_cpus (unwrap the list, then test the unwrapped value) means the shared entry point defends itself rather than depending on every caller. I re-ran the original failing case against the new logic:

  call site passes: ['']   truthy? True
  old unwrap -> ''    would match unlabelled cpu? True     <- the bug
  new unwrap -> None  reaches legacy branch? False        <- closed
  

• The series — down from 16 commits to 5, with the rebase-artifact commits folded and their branch-history wording gone. The five that remain read well.

_cpu_matches_legacy_label is a good addition beyond what I asked for — resolving through /__symbols__ when a parsed node hasn't kept its label closes a real gap, and since the caller only iterates the referenced cluster's own children, it can't widen selection across clusters.

One question on code that arrived after my last review, inline. Not blocking, and I may be reading the reachability wrong.

[zeddii]

This fallback is new since my last review, so flagging it rather than letting it through unremarked.

The cross-cluster constraint you describe in the comment is there and works. What I'm less sure about is selection within the cluster: the predicate doesn't test whether cpu is the one the legacy label names ... only that some legacy label exists and that cpu is a child of the referenced cluster. On a cluster with more than one CPU child and an unresolvable mask, that returns True for every CPU in it, not just the intended one.

For the split single-CPU R5/R52 clusters this is aimed at, the set is a single core and the result is exactly right. My question is what happens on a multi-core cluster reaching the same path: does the domain then match relations for every core, and is that the intended compatibility behaviour?

If it is, a line in the comment saying so would help, because the surrounding code is otherwise strict about never widening a selection — resolve_domain_cpus goes out of its way to return UNRESOLVED rather than guess, and this reads as the one place that relaxes it.

If it isn't, then matching cpu against _openamp_legacy_domain_processor(domain) through _cpu_matches_legacy_label (the helper you just added) would narrow it to the named core while keeping the psx_ prefix tolerance the comment is about.

[bentheredonethat]

@zeddii

Thanks for review and good catch. The fallback currently allows an unresolved selection to every CPU in the referenced cluster.

That happens to work for the split single-CPU R5/R52 topology, but it is not the intended behavior for a multi-core cluster.

I’ll narrow it by matching the CPU against the legacy cluster_cpu value through _cpu_matches_legacy_label, while retaining the referenced-cluster constraint, and add a multi-core regression test.