diff mbox series

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

Message ID 20220329011301.1166265-2-oupton@google.com (mailing list archive)
State New, archived
Headers show
Series KVM: arm64: Limit feature register reads from AArch32 | expand

Commit Message

Oliver Upton March 29, 2022, 1:12 a.m. UTC 
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(+)

Comments

Reiji Watanabe March 31, 2022, 5:45 a.m. UTC | #1 
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,
Reiji


> + */
> +static inline bool kvm_is_cp15_id_reg(struct sys_reg_params *params)
> +{
> +       return params->CRn == 0 && params->Op1 == 0 && params->CRm != 0;
> +}
> +
>  /**
>   * kvm_handle_cp_32 -- handles a mrc/mcr trap on a guest CP14/CP15 access
>   * @vcpu: The VCPU pointer
> @@ -2360,6 +2419,13 @@ static int kvm_handle_cp_32(struct kvm_vcpu *vcpu,
>         params.Op1 = (esr >> 14) & 0x7;
>         params.Op2 = (esr >> 17) & 0x7;
>
> +       /*
> +        * Certain AArch32 ID registers are handled by rerouting to the AArch64
> +        * system register table.
> +        */
> +       if (ESR_ELx_EC(esr) == ESR_ELx_EC_CP15_32 && kvm_is_cp15_id_reg(&params))
> +               return kvm_emulate_cp15_id_reg(vcpu, &params);
> +
>         if (!emulate_cp(vcpu, &params, global, nr_global)) {
>                 if (!params.is_write)
>                         vcpu_set_reg(vcpu, Rt, params.regval);
> --
> 2.35.1.1021.g381101b075-goog
>
Oliver Upton March 31, 2022, 3:34 p.m. UTC | #2 
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
diff mbox series

Patch

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;
+
+	/*
+	 * 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)
+		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.
+ */
+static inline bool kvm_is_cp15_id_reg(struct sys_reg_params *params)
+{
+	return params->CRn == 0 && params->Op1 == 0 && params->CRm != 0;
+}
+
 /**
  * kvm_handle_cp_32 -- handles a mrc/mcr trap on a guest CP14/CP15 access
  * @vcpu: The VCPU pointer
@@ -2360,6 +2419,13 @@  static int kvm_handle_cp_32(struct kvm_vcpu *vcpu,
 	params.Op1 = (esr >> 14) & 0x7;
 	params.Op2 = (esr >> 17) & 0x7;
 
+	/*
+	 * Certain AArch32 ID registers are handled by rerouting to the AArch64
+	 * system register table.
+	 */
+	if (ESR_ELx_EC(esr) == ESR_ELx_EC_CP15_32 && kvm_is_cp15_id_reg(&params))
+		return kvm_emulate_cp15_id_reg(vcpu, &params);
+
 	if (!emulate_cp(vcpu, &params, global, nr_global)) {
 		if (!params.is_write)
 			vcpu_set_reg(vcpu, Rt, params.regval);