Re: [RFC PATCH 09/11] target/arm/hvf: Sync registers used at EL2

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 12/8/25 02:20, Richard Henderson wrote:
> On 8/12/25 03:06, Philippe Mathieu-Daudé wrote:
>> From: Mohamed Mediouni <mohamed@unpredictable.fr>
>>
>> When starting up the VM at EL2, more sysregs are available. Sync the 
>> state of those.
>>
>> Signed-off-by: Mohamed Mediouni <mohamed@unpredictable.fr>
>> [PMD: Adapted to host_cpu_feature_supported() API]
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   target/arm/hvf/hvf.c | 34 ++++++++++++++++++++++++++++++++++
>>   1 file changed, 34 insertions(+)
>>
>> diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
>> index 5174973991f..778dc3cedf7 100644
>> --- a/target/arm/hvf/hvf.c
>> +++ b/target/arm/hvf/hvf.c
>> @@ -400,6 +400,7 @@ static const struct hvf_reg_match 
>> hvf_fpreg_match[] = {
>>   struct hvf_sreg_match {
>>       int reg;
>>       uint32_t key;
>> +    bool el2;
>>       uint32_t cp_idx;
>>   };
>> @@ -545,6 +546,27 @@ static struct hvf_sreg_match hvf_sreg_match[] = {
>>       { HV_SYS_REG_CNTV_CTL_EL0, HVF_SYSREG(14, 3, 3, 3, 1) },
>>       { HV_SYS_REG_CNTV_CVAL_EL0, HVF_SYSREG(14, 3, 3, 3, 2) },
>>       { HV_SYS_REG_SP_EL1, HVF_SYSREG(4, 1, 3, 4, 0) },

[*]

>> +    /* EL2 */
>> +    { HV_SYS_REG_CPTR_EL2, HVF_SYSREG(1, 1, 3, 4, 2), .el2 = true },
>> +    { HV_SYS_REG_ELR_EL2, HVF_SYSREG(4, 0, 3, 4, 1), .el2 = true },
>> +    { HV_SYS_REG_ESR_EL2, HVF_SYSREG(5, 2, 3, 4, 0), .el2 = true },
>> +    { HV_SYS_REG_FAR_EL2, HVF_SYSREG(6, 0, 3, 4, 0), .el2 = true },
>> +    { HV_SYS_REG_HCR_EL2, HVF_SYSREG(1, 1, 3, 4, 0), .el2 = true },
>> +    { HV_SYS_REG_HPFAR_EL2, HVF_SYSREG(6, 0, 3, 4, 4), .el2 = true },
>> +    { HV_SYS_REG_MAIR_EL2, HVF_SYSREG(10, 2, 3, 4, 0), .el2 = true },
>> +    { HV_SYS_REG_MDCR_EL2, HVF_SYSREG(1, 1, 3, 4, 1), .el2 = true },
>> +    { HV_SYS_REG_SCTLR_EL2, HVF_SYSREG(1, 0, 3, 4, 0), .el2 = true },
>> +    { HV_SYS_REG_SPSR_EL2, HVF_SYSREG(4, 0, 3, 4, 0), .el2 = true },
>> +    { HV_SYS_REG_SP_EL2, HVF_SYSREG(4, 1, 3, 6, 0), .el2 = true},
>> +    { HV_SYS_REG_TCR_EL2, HVF_SYSREG(2, 0, 3, 4, 2), .el2 = true },
>> +    { HV_SYS_REG_TPIDR_EL2, HVF_SYSREG(13, 0, 3, 4, 2), .el2 = true },
>> +    { HV_SYS_REG_TTBR0_EL2, HVF_SYSREG(2, 0, 3, 4, 0), .el2 = true },
>> +    { HV_SYS_REG_TTBR1_EL2, HVF_SYSREG(2, 0, 3, 4, 1), .el2 = true },
>> +    { HV_SYS_REG_VBAR_EL2, HVF_SYSREG(12, 0, 3, 4, 0), .el2 = true },
>> +    { HV_SYS_REG_VMPIDR_EL2, HVF_SYSREG(0, 0, 3, 4, 5), .el2 = true },
>> +    { HV_SYS_REG_VPIDR_EL2, HVF_SYSREG(0, 0, 3, 4, 0), .el2 = true },
>> +    { HV_SYS_REG_VTCR_EL2, HVF_SYSREG(2, 1, 3, 4, 2), .el2 = true },
>> +    { HV_SYS_REG_VTTBR_EL2, HVF_SYSREG(2, 1, 3, 4, 0), .el2 = true },
> 
> Note that el2 == opc1 in {4,5}.

Does this also work with HV_SYS_REG_SP_EL1 (see [*])?
I suppose so, after reading commit 67d10fc4737 ("target/arm: Trap
sysreg accesses for FEAT_NV") addition:

/**
  * arm_cpreg_trap_in_nv: Return true if cpreg traps in nested
  * virtualization
  *
  * Return true if this cpreg is one which should be trapped to EL2
  * if it is executed at EL1 when nested virtualization is enabled
  * via HCR_EL2.NV.
  */
static inline bool arm_cpreg_traps_in_nv(const ARMCPRegInfo *ri)
{
     /*
      * The Arm ARM defines the registers to be trapped in terms of
      * their names (I_TZTZL). However the underlying principle is "if
      * it would UNDEF at EL1 but work at EL2 then it should trap", and
      * the way the encoding of sysregs and system instructions is done
      * means that the right set of registers is exactly those where
      * the opc1 field is 4 or 5. (You can see this also in the assert
      * we do that the opc1 field and the permissions mask line up in
      * define_one_arm_cp_reg_with_opaque().)
      * Checking the opc1 field is easier for us and avoids the problem
      * that we do not consistently use the right architectural names
      * for all sysregs, since we treat the name field as largely for
      * debug.
      *
      * However we do this check, it is going to be at least potentially
      * fragile to future new sysregs, but this seems the least likely
      * to break.
      *
      * In particular, note that the released sysreg XML defines that
      * the FEAT_MEC sysregs and instructions do not follow this FEAT_NV
      * trapping rule, so we will need to add an ARM_CP_* flag to
      * indicate "register does not trap on NV" to handle those if/when
      * we implement FEAT_MEC.
      */
     return ri->opc1 == 4 || ri->opc1 == 5;
}

 From v2:

 >> @@ -608,6 +630,11 @@ int hvf_get_registers(CPUState *cpu)
 >>               continue;
 >>           }
 >> +        if (hvf_sreg_match[i].el2
 >> +            && !host_cpu_feature_supported(ARM_FEATURE_EL2)) {
 >> +            continue;
 >> +        }
 >
 > I thought you were going to change this to arm_feature(&cpu->env,
 > ARM_FEATURE_EL2).

We want to use HVF to get these registers on M3 / M4, but not on
M1 / M2 when split-accel in use where we emulate them.

Expected logic:

     if (
         // this is an EL2 register ...
         hvf_reg_traps_in_nv(i)
         // ... and CPU doesn't provide EL2 ...
         && (!arm_feature(&cpu->env, ARM_FEATURE_EL2)
             // ... or we are emulating EL2 (split-accel) ...
             || tcg_enabled())
     ) {
         // ... then do not get the register with the HVF API ...
         continue;
     }
     // ... otherwise by default use HVF.
     hv_vcpu_get_sys_reg(cpu->accel->fd, hvf_sreg_match[i].reg, &val);