[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC PATCH 16/40] target/arm: Represent the entire MPIDR_EL1
From: |
Peter Maydell |
Subject: |
Re: [RFC PATCH 16/40] target/arm: Represent the entire MPIDR_EL1 |
Date: |
Fri, 6 Jan 2023 22:14:17 +0000 |
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
[RFC PATCH 18/40] target/arm: Create cpreg definition functions with GHashTable arg, Richard Henderson, 2023/01/03
[RFC PATCH 05/40] target/arm: Create arm_cpu_register_parent, Richard Henderson, 2023/01/03
[RFC PATCH 07/40] target/arm: Create TYPE_ARM_V7M_CPU, Richard Henderson, 2023/01/03
[RFC PATCH 02/40] qom: Introduce class_late_init, Richard Henderson, 2023/01/03
[RFC PATCH 08/40] target/arm: Pass ARMCPUClass to ARMCPUInfo.class_init, Richard Henderson, 2023/01/03
[RFC PATCH 14/40] target/arm: Rename arm_cpu_mp_affinity, Richard Henderson, 2023/01/03