diff mbox series

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

Message ID 20220401010832.3425787-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 April 1, 2022, 1:08 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 | 68 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)

Comments

Reiji Watanabe April 4, 2022, 1:51 a.m. UTC | #1 
On Thu, Mar 31, 2022 at 6:08 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>

Reviewed-by: Reiji Watanabe <reijiw@google.com>
Marc Zyngier April 6, 2022, 3:07 p.m. UTC | #2 
On Fri, 01 Apr 2022 02:08:30 +0100,
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 | 68 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 68 insertions(+)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index dd34b5ab51d4..8b791256a5b4 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -2339,6 +2339,67 @@ 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;
> +
> +	/* Treat impossible writes to RO registers as UNDEFINED */
> +	if (params->is_write) {
> +		unhandled_cp_access(vcpu, params);
> +		return 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);
> +
> +	vcpu_set_reg(vcpu, Rt, params->regval);

It feels odd to update Rt without checking whether the read has
succeeded. In your case, this is harmless, but would break with the
approach I'm outlining below.

> +	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. The
> + * only register trapped in the CRm=0 range is CTR, which is already handled in
> + * the cp15 register table.

There is also the fact that CTR_EL0 has Op1=3 while CTR has Op1=0,
which prevents it from fitting in your scheme.

> + */
> +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 +2421,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);

I think this is a bit ugly. We reach this point from a function that
was cp15-specific, and now we are reconstructing the context. I'd
rather this is moved to kvm_handle_cp15_32(), and treated there
(untested):

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 7b45c040cc27..a071d89ace92 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -2350,28 +2350,21 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu,
  * @run:  The kvm_run struct
  */
 static int kvm_handle_cp_32(struct kvm_vcpu *vcpu,
+			    struct sys_reg_params *params,
 			    const struct sys_reg_desc *global,
 			    size_t nr_global)
 {
-	struct sys_reg_params params;
-	u32 esr = kvm_vcpu_get_esr(vcpu);
 	int Rt  = kvm_vcpu_sys_get_rt(vcpu);
 
-	params.CRm = (esr >> 1) & 0xf;
-	params.regval = vcpu_get_reg(vcpu, Rt);
-	params.is_write = ((esr & 1) == 0);
-	params.CRn = (esr >> 10) & 0xf;
-	params.Op0 = 0;
-	params.Op1 = (esr >> 14) & 0x7;
-	params.Op2 = (esr >> 17) & 0x7;
+	params->regval = vcpu_get_reg(vcpu, Rt);
 
-	if (!emulate_cp(vcpu, &params, global, nr_global)) {
-		if (!params.is_write)
-			vcpu_set_reg(vcpu, Rt, params.regval);
+	if (!emulate_cp(vcpu, params, global, nr_global)) {
+		if (!params->is_write)
+			vcpu_set_reg(vcpu, Rt, params->regval);
 		return 1;
 	}
 
-	unhandled_cp_access(vcpu, &params);
+	unhandled_cp_access(vcpu, params);
 	return 1;
 }
 
@@ -2382,7 +2375,14 @@ int kvm_handle_cp15_64(struct kvm_vcpu *vcpu)
 
 int kvm_handle_cp15_32(struct kvm_vcpu *vcpu)
 {
-	return kvm_handle_cp_32(vcpu, cp15_regs, ARRAY_SIZE(cp15_regs));
+	struct sys_reg_params params;
+
+	params = esr_cp1x_32_to_params(kvm_vcpu_get_esr(vcpu));
+
+	if (params.Op1 == 0 && params.CRn == 0 && params.CRm)
+		return kvm_emulate_cp15_id_reg(vcpu, &params);
+
+	return kvm_handle_cp_32(vcpu, &params, cp15_regs, ARRAY_SIZE(cp15_regs));
 }
 
 int kvm_handle_cp14_64(struct kvm_vcpu *vcpu)
@@ -2392,7 +2392,11 @@ int kvm_handle_cp14_64(struct kvm_vcpu *vcpu)
 
 int kvm_handle_cp14_32(struct kvm_vcpu *vcpu)
 {
-	return kvm_handle_cp_32(vcpu, cp14_regs, ARRAY_SIZE(cp14_regs));
+	struct sys_reg_params params;
+
+	params = esr_cp1x_32_to_params(kvm_vcpu_get_esr(vcpu));
+
+	return kvm_handle_cp_32(vcpu, &params, cp14_regs, ARRAY_SIZE(cp14_regs));
 }
 
 static bool is_imp_def_sys_reg(struct sys_reg_params *params)
diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
index cc0cc95a0280..fd4b2bb8c782 100644
--- a/arch/arm64/kvm/sys_regs.h
+++ b/arch/arm64/kvm/sys_regs.h
@@ -35,6 +35,13 @@ struct sys_reg_params {
 				  .Op2 = ((esr) >> 17) & 0x7,                  \
 				  .is_write = !((esr) & 1) })
 
+#define esr_cp1x_32_to_params(esr)					       \
+	((struct sys_reg_params){ .Op1 = ((esr) >> 14) & 0x7,                  \
+				  .CRn = ((esr) >> 10) & 0xf,                  \
+				  .CRm = ((esr) >> 1) & 0xf,                   \
+				  .Op2 = ((esr) >> 17) & 0x7,                  \
+				  .is_write = !((esr) & 1) })
+
 struct sys_reg_desc {
 	/* Sysreg string for debug */
 	const char *name;


What do you think?

	M.
Oliver Upton April 7, 2022, 8:12 p.m. UTC | #3 
Hi Marc,

On Wed, Apr 06, 2022 at 04:07:28PM +0100, Marc Zyngier wrote:
> > +	/*
> > +	 * 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);
> > +
> > +	vcpu_set_reg(vcpu, Rt, params->regval);
> 
> It feels odd to update Rt without checking whether the read has
> succeeded. In your case, this is harmless, but would break with the
> approach I'm outlining below.
> 

A total kludge to avoid yet another level of indentation :) I'll go
about this the right way next spin.

> > +	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. The
> > + * only register trapped in the CRm=0 range is CTR, which is already handled in
> > + * the cp15 register table.
> 
> There is also the fact that CTR_EL0 has Op1=3 while CTR has Op1=0,
> which prevents it from fitting in your scheme.
> 
> > + */
> > +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 +2421,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);
> 
> I think this is a bit ugly. We reach this point from a function that
> was cp15-specific, and now we are reconstructing the context. I'd
> rather this is moved to kvm_handle_cp15_32(), and treated there
> (untested):
>

Completely agree, hoisting this would be much more elegant.

> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 7b45c040cc27..a071d89ace92 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -2350,28 +2350,21 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu,
>   * @run:  The kvm_run struct
>   */
>  static int kvm_handle_cp_32(struct kvm_vcpu *vcpu,
> +			    struct sys_reg_params *params,
>  			    const struct sys_reg_desc *global,
>  			    size_t nr_global)
>  {
> -	struct sys_reg_params params;
> -	u32 esr = kvm_vcpu_get_esr(vcpu);
>  	int Rt  = kvm_vcpu_sys_get_rt(vcpu);
>  
> -	params.CRm = (esr >> 1) & 0xf;
> -	params.regval = vcpu_get_reg(vcpu, Rt);
> -	params.is_write = ((esr & 1) == 0);
> -	params.CRn = (esr >> 10) & 0xf;
> -	params.Op0 = 0;
> -	params.Op1 = (esr >> 14) & 0x7;
> -	params.Op2 = (esr >> 17) & 0x7;
> +	params->regval = vcpu_get_reg(vcpu, Rt);
>  
> -	if (!emulate_cp(vcpu, &params, global, nr_global)) {
> -		if (!params.is_write)
> -			vcpu_set_reg(vcpu, Rt, params.regval);
> +	if (!emulate_cp(vcpu, params, global, nr_global)) {
> +		if (!params->is_write)
> +			vcpu_set_reg(vcpu, Rt, params->regval);
>  		return 1;
>  	}
>  
> -	unhandled_cp_access(vcpu, &params);
> +	unhandled_cp_access(vcpu, params);
>  	return 1;
>  }
>  
> @@ -2382,7 +2375,14 @@ int kvm_handle_cp15_64(struct kvm_vcpu *vcpu)
>  
>  int kvm_handle_cp15_32(struct kvm_vcpu *vcpu)
>  {
> -	return kvm_handle_cp_32(vcpu, cp15_regs, ARRAY_SIZE(cp15_regs));
> +	struct sys_reg_params params;
> +
> +	params = esr_cp1x_32_to_params(kvm_vcpu_get_esr(vcpu));
> +
> +	if (params.Op1 == 0 && params.CRn == 0 && params.CRm)
> +		return kvm_emulate_cp15_id_reg(vcpu, &params);
> +
> +	return kvm_handle_cp_32(vcpu, &params, cp15_regs, ARRAY_SIZE(cp15_regs));
>  }
>  
>  int kvm_handle_cp14_64(struct kvm_vcpu *vcpu)
> @@ -2392,7 +2392,11 @@ int kvm_handle_cp14_64(struct kvm_vcpu *vcpu)
>  
>  int kvm_handle_cp14_32(struct kvm_vcpu *vcpu)
>  {
> -	return kvm_handle_cp_32(vcpu, cp14_regs, ARRAY_SIZE(cp14_regs));
> +	struct sys_reg_params params;
> +
> +	params = esr_cp1x_32_to_params(kvm_vcpu_get_esr(vcpu));
> +
> +	return kvm_handle_cp_32(vcpu, &params, cp14_regs, ARRAY_SIZE(cp14_regs));
>  }
>  
>  static bool is_imp_def_sys_reg(struct sys_reg_params *params)
> diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
> index cc0cc95a0280..fd4b2bb8c782 100644
> --- a/arch/arm64/kvm/sys_regs.h
> +++ b/arch/arm64/kvm/sys_regs.h
> @@ -35,6 +35,13 @@ struct sys_reg_params {
>  				  .Op2 = ((esr) >> 17) & 0x7,                  \
>  				  .is_write = !((esr) & 1) })
>  
> +#define esr_cp1x_32_to_params(esr)					       \
> +	((struct sys_reg_params){ .Op1 = ((esr) >> 14) & 0x7,                  \
> +				  .CRn = ((esr) >> 10) & 0xf,                  \
> +				  .CRm = ((esr) >> 1) & 0xf,                   \
> +				  .Op2 = ((esr) >> 17) & 0x7,                  \
> +				  .is_write = !((esr) & 1) })
> +
>  struct sys_reg_desc {
>  	/* Sysreg string for debug */
>  	const char *name;
> 
> 
> What do you think?

Way better. Your suggested patch looks correct, I'll fold all of this
together and test it out. Thanks for the suggestions :)

--
Best,
Oliver
diff mbox series

Patch

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index dd34b5ab51d4..8b791256a5b4 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -2339,6 +2339,67 @@  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;
+
+	/* Treat impossible writes to RO registers as UNDEFINED */
+	if (params->is_write) {
+		unhandled_cp_access(vcpu, params);
+		return 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);
+
+	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. The
+ * only register trapped in the CRm=0 range is CTR, which is already 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 +2421,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);