Re: [RFC PATCH 16/40] target/arm: Represent the entire MPIDR_EL1

[Peter Maydell <peter.maydell@linaro.org>]

On Fri, 6 Jan 2023 at 19:33, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 1/6/23 11:16, Peter Maydell wrote:
> > On Tue, 3 Jan 2023 at 18:24, Richard Henderson
> > <richard.henderson@linaro.org> wrote:
> >>
> >> Replace ARMCPU.mp_affinity with CPUARMState.cp15.mpidr_el1,
> >> setting the additional bits as required.  In particular,
> >> always set the U bit when there is only one cpu in the system.
> >> Remove the mp_is_up bit which attempted to do the same thing.
> >>
> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> >> ---
> >>   target/arm/cpu.h     |  7 ++--
> >>   target/arm/cpu.c     | 80 +++++++++++++++++++++++++++++++++++++-------
> >>   target/arm/cpu_tcg.c |  1 -
> >>   target/arm/helper.c  | 25 ++------------
> >>   target/arm/hvf/hvf.c |  2 +-
> >>   target/arm/kvm64.c   |  4 +--
> >>   6 files changed, 75 insertions(+), 44 deletions(-)
> >
> > Based purely on the diffstat it's not super-obvious why this
> > is an improvement. What's the rationale ?
>
> It gets rid of cpu->mp_is_up, set only by cortex-r5, and then generalizes.
>
> > Side note, we don't currently handle the MT bit, where some
> > CPUs end up putting the cpu number in the Aff1 field rather
> > than Aff0. We ideally ought to handle that too.
>
> Would that be set by the board as well?  I don't think we model cpu
> packages/clusters/whatchacallums at that level currently.

It's a fixed property of the CPU implementation, same as
mp_is_up. We currently mismodel Cortex-A55, Cortex-A76
and Neoverse-N1, which should all be setting the MT bit in
MPIDR and having the cpu number in Aff1. See this thread:
 https://lore.kernel.org/qemu-devel/CAFEAcA9P2-v94p8H8+ktnf-Yf-rucbGySXE6AGPdwvDxXfP=ZA@mail.gmail.com/

+    if (arm_feature(env, ARM_FEATURE_V7MP)) {
+        cpu->mpidr_el1 |= (1u << 31);   /* M */
+        if (cpu->core_count == 1) {
+            cpu->mpidr_el1 |= 1 << 30;  /* U */
+        }
+    }

This is wrong, incidentally -- a single Cortex A9, A53, etc does
not set the U bit. (It's "a cluster with 1 core in it", not
"a uniprocessor system".) The reason mp_is_up is set only
by the R5 model is because "we implement the MP extensions
but consider ourselves to be not part of a cluster" is a
behaviour of only this CPU. I forget why I didn't implement
it as an ARM_FEATURE_FOO bit.

thanks
-- PMM