Re: [PATCH 1/3] KVM: arm64: Wire up CP15 feature registers to their AArch64 equivalents

[Oliver Upton <oupton@google.com>]

Hi Reiji,

On Wed, Mar 30, 2022 at 10:45:35PM -0700, Reiji Watanabe wrote:
> Hi Oliver,
> 
> On Mon, Mar 28, 2022 at 6:13 PM Oliver Upton <oupton@google.com> wrote:
> >
> > KVM currently does not trap ID register accesses from an AArch32 EL1.
> > This is painful for a couple of reasons. Certain unimplemented features
> > are visible to AArch32 EL1, as we limit PMU to version 3 and the debug
> > architecture to v8.0. Additionally, we attempt to paper over
> > heterogeneous systems by using register values that are safe
> > system-wide. All this hard work is completely sidestepped because KVM
> > does not set TID3 for AArch32 guests.
> >
> > Fix up handling of CP15 feature registers by simply rerouting to their
> > AArch64 aliases. Punt setting HCR_EL2.TID3 to a later change, as we need
> > to fix up the oddball CP10 feature registers still.
> >
> > Signed-off-by: Oliver Upton <oupton@google.com>
> > ---
> >  arch/arm64/kvm/sys_regs.c | 66 +++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 66 insertions(+)
> >
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index dd34b5ab51d4..30771f950027 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -2339,6 +2339,65 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu,
> >         return 1;
> >  }
> >
> > +static int emulate_sys_reg(struct kvm_vcpu *vcpu, struct sys_reg_params *params);
> > +
> > +/**
> > + * kvm_emulate_cp15_id_reg() - Handles an MRC trap on a guest CP15 access where
> > + *                            CRn=0, which corresponds to the AArch32 feature
> > + *                            registers.
> > + * @vcpu: the vCPU pointer
> > + * @params: the system register access parameters.
> > + *
> > + * Our cp15 system register tables do not enumerate the AArch32 feature
> > + * registers. Conveniently, our AArch64 table does, and the AArch32 system
> > + * register encoding can be trivially remapped into the AArch64 for the feature
> > + * registers: Append op0=3, leaving op1, CRn, CRm, and op2 the same.
> > + *
> > + * According to DDI0487G.b G7.3.1, paragraph "Behavior of VMSAv8-32 32-bit
> > + * System registers with (coproc=0b1111, CRn==c0)", read accesses from this
> > + * range are either UNKNOWN or RES0. Rerouting remains architectural as we
> > + * treat undefined registers in this range as RAZ.
> > + */
> > +static int kvm_emulate_cp15_id_reg(struct kvm_vcpu *vcpu,
> > +                                  struct sys_reg_params *params)
> > +{
> > +       int Rt = kvm_vcpu_sys_get_rt(vcpu);
> > +       int ret = 1;
> > +
> > +       params->Op0 = 3;
> 
> Nit: Shouldn't we restore the original Op0 after emulate_sys_reg() ?
> (unhandled_cp_access() prints Op0. Restoring the original one
>  would be more robust against future changes)
> 
> > +
> > +       /*
> > +        * All registers where CRm > 3 are known to be UNKNOWN/RAZ from AArch32.
> > +        * Avoid conflicting with future expansion of AArch64 feature registers
> > +        * and simply treat them as RAZ here.
> > +        */
> > +       if (params->CRm > 3)
> > +               params->regval = 0;
> > +       else
> > +               ret = emulate_sys_reg(vcpu, params);
> > +
> > +       /* Treat impossible writes to RO registers as UNDEFINED */
> > +       if (params->is_write)
> 
> This checking can be done even before calling emulate_sys_reg().
> BTW, __access_id_reg() also injects UNDEFINED when p->is_write is true.
> 
> > +               unhandled_cp_access(vcpu, params);
> > +       else
> > +               vcpu_set_reg(vcpu, Rt, params->regval);
> > +
> > +       return ret;
> > +}
> > +
> > +/**
> > + * kvm_is_cp15_id_reg() - Returns true if the specified CP15 register is an
> > + *                       AArch32 ID register.
> > + * @params: the system register access parameters
> > + *
> > + * Note that CP15 ID registers where CRm=0 are excluded from this check, as they
> > + * are already correctly handled in the CP15 register table.
> 
> I don't think this is true for all of the registers:)
> I think at least some of them are not trapped (TCMTR, TLBTR,
> REVIDR, etc), and I don't think they are handled in the CP15
> register table.

Thanks for the review! This patch was a bit sloppy and indeed skipped a
few steps in the comments. I believe the only register in CRm=0 that is
trapped is actually CTR, hence the others are not present in the table.

I'll send out a v2 soon that addresses your feedback and the other
embarrasing omissions on my end :)

--
Thanks,
Oliver
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm