[PATCH v2 20/37] target/arm: Consolidate definitions of PAR

Richard Henderson posted 37 patches 1 month ago
Maintainers: Peter Xu <peterx@redhat.com>, Fabiano Rosas <farosas@suse.de>, Peter Maydell <peter.maydell@linaro.org>, Alexander Graf <agraf@csgraf.de>, Mads Ynddal <mads@ynddal.dk>, Paolo Bonzini <pbonzini@redhat.com>
[PATCH v2 20/37] target/arm: Consolidate definitions of PAR
Posted by Richard Henderson 1 month ago
Create a function define_par_register which handles the 3
distinct cases for PAR.  It is easier to understand with
the definitions all in one place.

Make the aarch64 to be the primary definition, when present,
rather than being an alias of the 64-bit non-secure aa32 reg.
Remove the unnecessary .writefn from the aarch64 defintion,
and drop it from the 32-bit definition with LPAE.

Remove the LPAE test from par_write, since it will no longer
be used in that situation.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/helper.c | 103 +++++++++++++++++++++++++++++++-------------
 1 file changed, 73 insertions(+), 30 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index d3a425e259..7800d83f48 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -2475,9 +2475,7 @@ static const ARMCPRegInfo gen_timer_ecv_cp_reginfo[] = {
 
 static void par_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
 {
-    if (arm_feature(env, ARM_FEATURE_LPAE)) {
-        raw_write(env, ri, value);
-    } else if (arm_feature(env, ARM_FEATURE_V7)) {
+    if (arm_feature(env, ARM_FEATURE_V7)) {
         raw_write(env, ri, value & 0xfffff6ff);
     } else {
         raw_write(env, ri, value & 0xfffff1ff);
@@ -3244,10 +3242,11 @@ static const ARMCPRegInfo lpae_cp_reginfo[] = {
     { .name = "AMAIR1", .cp = 15, .crn = 10, .crm = 3, .opc1 = 0, .opc2 = 1,
       .access = PL1_RW, .accessfn = access_tvm_trvm,
       .type = ARM_CP_CONST, .resetvalue = 0 },
-    { .name = "PAR", .cp = 15, .crm = 7, .opc1 = 0,
-      .access = PL1_RW, .type = ARM_CP_64BIT, .resetvalue = 0,
-      .bank_fieldoffsets = { offsetof(CPUARMState, cp15.par_s),
-                             offsetof(CPUARMState, cp15.par_ns)} },
+
+    /*
+     * The primary definitions of TTBR[01]_EL1 are in vmsa_cp_reginfo[].
+     * Here we need only provide the 64-bit views for AArch32.
+     */
     { .name = "TTBR0", .cp = 15, .crm = 2, .opc1 = 0,
       .access = PL1_RW, .accessfn = access_tvm_trvm,
       .type = ARM_CP_64BIT | ARM_CP_ALIAS,
@@ -3262,6 +3261,71 @@ static const ARMCPRegInfo lpae_cp_reginfo[] = {
       .writefn = vmsa_ttbr_write, .raw_writefn = raw_write },
 };
 
+static void define_par_register(ARMCPU *cpu)
+{
+    /*
+     * For v8:
+     * The aarch64 reg is primary, since it might be 128-bit.
+     * The aarch32 64-bit non-secure reg is secondary to aa64.
+     * The aarch32 64-bit secure reg is primary.
+     *
+     * For v7:
+     * The aarch32 64-bit s+ns regs are primary.
+     *
+     * The aarch32 32-bit regs are secondary to one of the above,
+     * and we also don't expose them to gdb.
+     */
+    static const ARMCPRegInfo parv8_reginfo = {
+        .name = "PAR_EL1", .state = ARM_CP_STATE_AA64,
+        .opc0 = 3, .opc1 = 0, .crn = 7, .crm = 4, .opc2 = 0,
+        .access = PL1_RW, .fgt = FGT_PAR_EL1,
+        .fieldoffset = offsetof(CPUARMState, cp15.par_el[1])
+    };
+
+    static ARMCPRegInfo par64_reginfo[2] = {
+        [0 ... 1] = {
+            .state = ARM_CP_STATE_AA32,
+            .cp = 15, .crm = 7, .opc1 = 0,
+            .type = ARM_CP_64BIT, .access = PL1_RW,
+        },
+        [0].name = "PAR",
+        [0].secure = ARM_CP_SECSTATE_NS,
+        [0].fieldoffset = offsetof(CPUARMState, cp15.par_ns),
+        [1].name = "PAR_S",
+        [1].secure = ARM_CP_SECSTATE_S,
+        [1].fieldoffset = offsetof(CPUARMState, cp15.par_s),
+    };
+
+    static ARMCPRegInfo par32_reginfo = {
+        .name = "PAR", .state = ARM_CP_STATE_AA32,
+        .cp = 15, .crn = 7, .crm = 4, .opc1 = 0, .opc2 = 0,
+        .access = PL1_RW, .resetvalue = 0,
+        .bank_fieldoffsets = { offsetoflow32(CPUARMState, cp15.par_s),
+                               offsetoflow32(CPUARMState, cp15.par_ns) },
+        .writefn = par_write,
+    };
+
+    CPUARMState *env = &cpu->env;
+
+    /* With only VAPA, define a 32-bit reg that filters bits from write. */
+    if (!arm_feature(env, ARM_FEATURE_LPAE)) {
+        define_one_arm_cp_reg(cpu, &par32_reginfo);
+        return;
+    }
+
+    /* With LPAE, the 32-bit regs are aliases of 64-bit regs. */
+    par32_reginfo.type = ARM_CP_ALIAS | ARM_CP_NO_GDB;
+    par32_reginfo.writefn = NULL;
+    define_one_arm_cp_reg(cpu, &par32_reginfo);
+
+    if (arm_feature(env, ARM_FEATURE_V8)) {
+        define_one_arm_cp_reg(cpu, &parv8_reginfo);
+        par64_reginfo[0].type |= ARM_CP_ALIAS;
+    }
+
+    define_arm_cp_regs(cpu, par64_reginfo);
+}
+
 static uint64_t aa64_fpcr_read(CPUARMState *env, const ARMCPRegInfo *ri)
 {
     return vfp_get_fpcr(env);
@@ -3765,13 +3829,6 @@ static const ARMCPRegInfo v8_cp_reginfo[] = {
       .opc0 = 1, .opc1 = 0, .crn = 7, .crm = 14, .opc2 = 2,
       .fgt = FGT_DCCISW,
       .access = PL1_W, .accessfn = access_tsw, .type = ARM_CP_NOP },
-    { .name = "PAR_EL1", .state = ARM_CP_STATE_AA64,
-      .type = ARM_CP_ALIAS,
-      .opc0 = 3, .opc1 = 0, .crn = 7, .crm = 4, .opc2 = 0,
-      .access = PL1_RW, .resetvalue = 0,
-      .fgt = FGT_PAR_EL1,
-      .fieldoffset = offsetof(CPUARMState, cp15.par_el[1]),
-      .writefn = par_write },
     /* 32 bit cache operations */
     { .name = "ICIALLUIS", .cp = 15, .opc1 = 0, .crn = 7, .crm = 1, .opc2 = 0,
       .type = ARM_CP_NOP, .access = PL1_W, .accessfn = access_ticab },
@@ -7120,23 +7177,9 @@ void register_cp_regs_for_features(ARMCPU *cpu)
         define_one_arm_cp_reg(cpu, &gen_timer_cntpoff_reginfo);
     }
 #endif
-    if (arm_feature(env, ARM_FEATURE_VAPA)) {
-        ARMCPRegInfo vapa_cp_reginfo[] = {
-            { .name = "PAR", .cp = 15, .crn = 7, .crm = 4, .opc1 = 0, .opc2 = 0,
-              .access = PL1_RW, .resetvalue = 0,
-              .bank_fieldoffsets = { offsetoflow32(CPUARMState, cp15.par_s),
-                                     offsetoflow32(CPUARMState, cp15.par_ns) },
-              .writefn = par_write},
-        };
 
-        /*
-         * When LPAE exists this 32-bit PAR register is an alias of the
-         * 64-bit AArch32 PAR register defined in lpae_cp_reginfo[]
-         */
-        if (arm_feature(env, ARM_FEATURE_LPAE)) {
-            vapa_cp_reginfo[0].type = ARM_CP_ALIAS | ARM_CP_NO_GDB;
-        }
-        define_arm_cp_regs(cpu, vapa_cp_reginfo);
+    if (arm_feature(env, ARM_FEATURE_VAPA)) {
+        define_par_register(cpu);
     }
     if (arm_feature(env, ARM_FEATURE_CACHE_TEST_CLEAN)) {
         define_arm_cp_regs(cpu, cache_test_clean_cp_reginfo);
-- 
2.43.0
Re: [PATCH v2 20/37] target/arm: Consolidate definitions of PAR
Posted by Peter Maydell 3 weeks, 4 days ago
On Tue, 14 Oct 2025 at 21:11, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Create a function define_par_register which handles the 3
> distinct cases for PAR.  It is easier to understand with
> the definitions all in one place.
>
> Make the aarch64 to be the primary definition, when present,
> rather than being an alias of the 64-bit non-secure aa32 reg.

Doesn't that break tcg-to-tcg migration across this commit?

thanks
-- PMM
Re: [PATCH v2 20/37] target/arm: Consolidate definitions of PAR
Posted by Richard Henderson 2 weeks, 3 days ago
On 10/20/25 15:31, Peter Maydell wrote:
> On Tue, 14 Oct 2025 at 21:11, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> Create a function define_par_register which handles the 3
>> distinct cases for PAR.  It is easier to understand with
>> the definitions all in one place.
>>
>> Make the aarch64 to be the primary definition, when present,
>> rather than being an alias of the 64-bit non-secure aa32 reg.
> 
> Doesn't that break tcg-to-tcg migration across this commit?

I don't know... possibly?
I guess we have a test for this somewhere, but it needs setup?

I'll experiment.


r~
Re: [PATCH v2 20/37] target/arm: Consolidate definitions of PAR
Posted by Peter Maydell 2 weeks, 3 days ago
On Tue, 28 Oct 2025 at 14:39, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 10/20/25 15:31, Peter Maydell wrote:
> > On Tue, 14 Oct 2025 at 21:11, Richard Henderson
> > <richard.henderson@linaro.org> wrote:
> >>
> >> Create a function define_par_register which handles the 3
> >> distinct cases for PAR.  It is easier to understand with
> >> the definitions all in one place.
> >>
> >> Make the aarch64 to be the primary definition, when present,
> >> rather than being an alias of the 64-bit non-secure aa32 reg.
> >
> > Doesn't that break tcg-to-tcg migration across this commit?
>
> I don't know... possibly?
> I guess we have a test for this somewhere, but it needs setup?

I don't have a test specifically -- I tend to use one
of my usual lying-around "boot linux" setups that has
a qcow2 disk file, and then connect the monitor and
use 'savevm foo' to save, and then check that -loadvm foo
with the new QEMU works.

thanks
-- PMM
Re: [PATCH v2 20/37] target/arm: Consolidate definitions of PAR
Posted by Richard Henderson 2 weeks, 3 days ago
On 10/28/25 15:41, Peter Maydell wrote:
> On Tue, 28 Oct 2025 at 14:39, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> On 10/20/25 15:31, Peter Maydell wrote:
>>> On Tue, 14 Oct 2025 at 21:11, Richard Henderson
>>> <richard.henderson@linaro.org> wrote:
>>>>
>>>> Create a function define_par_register which handles the 3
>>>> distinct cases for PAR.  It is easier to understand with
>>>> the definitions all in one place.
>>>>
>>>> Make the aarch64 to be the primary definition, when present,
>>>> rather than being an alias of the 64-bit non-secure aa32 reg.
>>>
>>> Doesn't that break tcg-to-tcg migration across this commit?
>>
>> I don't know... possibly?
>> I guess we have a test for this somewhere, but it needs setup?
> 
> I don't have a test specifically -- I tend to use one
> of my usual lying-around "boot linux" setups that has
> a qcow2 disk file, and then connect the monitor and
> use 'savevm foo' to save, and then check that -loadvm foo
> with the new QEMU works.

You're right, it doesn't work.  It'll make the 128-bit version more complicated, but I 
guess there's no avoiding it.


r~
Re: [PATCH v2 20/37] target/arm: Consolidate definitions of PAR
Posted by Peter Maydell 1 week, 4 days ago
On Tue, 28 Oct 2025 at 15:05, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 10/28/25 15:41, Peter Maydell wrote:
> > On Tue, 28 Oct 2025 at 14:39, Richard Henderson
> > <richard.henderson@linaro.org> wrote:
> >>
> >> On 10/20/25 15:31, Peter Maydell wrote:
> >>> On Tue, 14 Oct 2025 at 21:11, Richard Henderson
> >>> <richard.henderson@linaro.org> wrote:
> >>>>
> >>>> Create a function define_par_register which handles the 3
> >>>> distinct cases for PAR.  It is easier to understand with
> >>>> the definitions all in one place.
> >>>>
> >>>> Make the aarch64 to be the primary definition, when present,
> >>>> rather than being an alias of the 64-bit non-secure aa32 reg.
> >>>
> >>> Doesn't that break tcg-to-tcg migration across this commit?
> >>
> >> I don't know... possibly?
> >> I guess we have a test for this somewhere, but it needs setup?
> >
> > I don't have a test specifically -- I tend to use one
> > of my usual lying-around "boot linux" setups that has
> > a qcow2 disk file, and then connect the monitor and
> > use 'savevm foo' to save, and then check that -loadvm foo
> > with the new QEMU works.
>
> You're right, it doesn't work.  It'll make the 128-bit version more complicated, but I
> guess there's no avoiding it.

We in general do not have a good story for handling
migration compat for sysregs. See also the
"BOGUS_DBGDTR_EL0" sysreg which is an ad-hoc way to
deal with "if this is in the incoming data then ignore it",
and Eric Auger's series that tries to deal with the KVM
situations where the host kernel might remove sysregs and
break migration for us.

It's possible that there might be some unified way to
permit the sysreg migration code to handle "the underlying
way we represent the data has changed, we need to fix
things up on migration input" so that (like the VMState
handling) we have a mechanism for changing the representation
the rest of QEMU uses at runtime without breaking compat,
keeping the impedance-matching code confined to migration.

But that would be a big pile of work, so for the sake of
FEAT_SYS128 it's probably better to work around it...

thanks
-- PMM