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

[Marc Zyngier]

On 2019-11-25 16:21, Peter Maydell wrote:
> On Sat, 23 Nov 2019 at 11:56, Marc Zyngier <address@hidden> 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 <address@hidden>
> > Signed-off-by: Marc Zyngier <address@hidden>
> > ---
> > 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...