Re: [PATCH] target/arm: Honor HCR_EL2.TID3 trapping requirements

[Marc Zyngier <maz@kernel.org>]

On 2019-11-25 16:21, Peter Maydell wrote:
> On Sat, 23 Nov 2019 at 11:56, Marc Zyngier <maz@kernel.org> wrote:
>>
>> HCR_EL2.TID3 mandates that access from EL1 to a long list of id
>> registers traps to EL2, and QEMU has so far ignored this 
>> requirement.
>>
>> This breaks (among other things) KVM guests that have PtrAuth 
>> enabled,
>> while the hypervisor doesn't want to expose the feature to its 
>> guest.
>> To achieve this, KVM traps the ID registers (ID_AA64ISAR1_EL1 in 
>> this
>> case), and masks out the unsupported feature.
>>
>> QEMU not honoring the trap request means that the guest observes
>> that the feature is present in the HW, starts using it, and dies
>> a horrible death when KVM injects an UNDEF, because the feature
>> *really* isn't supported.
>>
>> Do the right thing by trapping to EL2 if HCR_EL2.TID3 is set.
>>
>> Reported-by: Will Deacon <will@kernel.org>
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>> ---
>> There is a number of other trap bits missing (TID[0-2], for 
>> example),
>> but this at least gets a mainline Linux going with cpu=max.
>
> I guess this ought to go into 4.2, given that it gets recent
> Linux guest kernels to work.
>
>
>> @@ -6185,11 +6221,13 @@ void register_cp_regs_for_features(ARMCPU 
>> *cpu)
>>              { .name = "ID_AA64PFR0_EL1", .state = 
>> ARM_CP_STATE_AA64,
>>                .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 0,
>>                .access = PL1_R, .type = ARM_CP_NO_RAW,
>> +              .accessfn = access_aa64idreg,
>>                .readfn = id_aa64pfr0_read,
>>                .writefn = arm_cp_write_ignore },
>>              { .name = "ID_AA64PFR1_EL1", .state = 
>> ARM_CP_STATE_AA64,
>>                .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 1,
>>                .access = PL1_R, .type = ARM_CP_CONST,
>> +              .accessfn = access_aa64idreg,
>>                .resetvalue = cpu->isar.id_aa64pfr1},
>>              { .name = "ID_AA64PFR2_EL1_RESERVED", .state = 
>> ARM_CP_STATE_AA64,
>>                .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 2,
>> @@ -6198,151 +6236,188 @@ void register_cp_regs_for_features(ARMCPU 
>> *cpu)
>>              { .name = "ID_AA64PFR3_EL1_RESERVED", .state = 
>> ARM_CP_STATE_AA64,
>>                .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 3,
>>                .access = PL1_R, .type = ARM_CP_CONST,
>> +              .accessfn = access_aa64idreg,
>>                .resetvalue = 0 },
>
> Missing .accessfn for ID_AA4PFR2_EL1_RESERVED ?

Indeed, I oversaw that one. I'll fix it and repost it together with
the extra patches to handle TID1 and TID2.

>
>>              { .name = "ID_AA64ZFR0_EL1", .state = 
>> ARM_CP_STATE_AA64,
>>                .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 4,
>>                .access = PL1_R, .type = ARM_CP_CONST,
>> +              .accessfn = access_aa64idreg,
>>                /* At present, only SVEver == 0 is defined anyway.  
>> */
>>                .resetvalue = 0 },
>
>>              { .name = "MVFR0_EL1", .state = ARM_CP_STATE_AA64,
>>                .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 3, .opc2 = 0,
>>                .access = PL1_R, .type = ARM_CP_CONST,
>> +              .accessfn = access_aa64idreg,
>>                .resetvalue = cpu->isar.mvfr0 },
>
> These are the AArch64 accessors for the aarch32 MVFR registers,
> but this trap bit is also supposed to trap aarch32 accesses to
> the MVFR registers, which are via the vmrs insn. That needs
> to be done in trans_VMSR_VMRS(); we can do that with a
> followup patch, since the mechanism there is completely different.

Holy cow! I'm afraid that my newly acquired QEMU-foo is missing
a few key options. Does it mean we need to generate a trapping
instruction, as opposed to take the exception on access?

I'd very much appreciate some guidance here.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...