[PATCH 0/3] KVM: arm64: Limit feature register reads from AArch32

[PATCH 0/3] KVM: arm64: Limit feature register reads from AArch32

[Oliver Upton]

KVM/arm64 does not restrict the guest's view of the AArch32 feature
registers when read from AArch32. HCR_EL2.TID3 is cleared for AArch32
guests, meaning that register reads come straight from hardware. This is
problematic as KVM relies on read_sanitised_ftr_reg() to expose a set of
features consistent for a particular system.

Appropriate handlers must first be put in place for CP10 and CP15 ID
register accesses before setting TID3. Rather than exhaustively
enumerating each of the encodings for CP10 and CP15 registers, take the
lazy route and aim the register accesses at the AArch64 system register
table.

Patch 1 reroutes the CP15 registers into the AArch64 table, taking care
to immediately RAZ undefined ranges of registers. This is done to avoid
possibly conflicting with encodings for future AArch64 registers.

Patch 2 installs an exit handler for the CP10 ID registers and also
relies on the general AArch64 register handler to implement reads.

Finally, patch 3 actually sets TID3 for AArch32 guests, providing
known-safe values for feature register accesses.

I'll leave it as an exercise for the reader to decide whether or not I'm
being _too_ lazy here ;-)

Series applies cleanly to kvmarm/fixes at commit:

  8872d9b3e35a ("KVM: arm64: Drop unneeded minor version check from PSCI v1.x handler")

Tested with AArch32 kvm-unit-tests and booting an AArch32 debian image
on a Raspberry Pi 4. Nothing seems to have gone up in smoke yet...

Oliver Upton (3):
  KVM: arm64: Wire up CP15 feature registers to their AArch64
    equivalents
  KVM: arm64: Plumb cp10 ID traps through the AArch64 sysreg handler
  KVM: arm64: Start trapping ID registers for 32 bit guests

 arch/arm64/include/asm/kvm_emulate.h |   8 --
 arch/arm64/include/asm/kvm_host.h    |   1 +
 arch/arm64/kvm/handle_exit.c         |   1 +
 arch/arm64/kvm/sys_regs.c            | 128 +++++++++++++++++++++++++++
 4 files changed, 130 insertions(+), 8 deletions(-)

-- 
2.35.1.1021.g381101b075-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 0/3] KVM: arm64: Limit feature register reads from AArch32

[Oliver Upton]

KVM/arm64 does not restrict the guest's view of the AArch32 feature
registers when read from AArch32. HCR_EL2.TID3 is cleared for AArch32
guests, meaning that register reads come straight from hardware. This is
problematic as KVM relies on read_sanitised_ftr_reg() to expose a set of
features consistent for a particular system.

Appropriate handlers must first be put in place for CP10 and CP15 ID
register accesses before setting TID3. Rather than exhaustively
enumerating each of the encodings for CP10 and CP15 registers, take the
lazy route and aim the register accesses at the AArch64 system register
table.

Patch 1 reroutes the CP15 registers into the AArch64 table, taking care
to immediately RAZ undefined ranges of registers. This is done to avoid
possibly conflicting with encodings for future AArch64 registers.

Patch 2 installs an exit handler for the CP10 ID registers and also
relies on the general AArch64 register handler to implement reads.

Finally, patch 3 actually sets TID3 for AArch32 guests, providing
known-safe values for feature register accesses.

I'll leave it as an exercise for the reader to decide whether or not I'm
being _too_ lazy here ;-)

Series applies cleanly to kvmarm/fixes at commit:

  8872d9b3e35a ("KVM: arm64: Drop unneeded minor version check from PSCI v1.x handler")

Tested with AArch32 kvm-unit-tests and booting an AArch32 debian image
on a Raspberry Pi 4. Nothing seems to have gone up in smoke yet...

Oliver Upton (3):
  KVM: arm64: Wire up CP15 feature registers to their AArch64
    equivalents
  KVM: arm64: Plumb cp10 ID traps through the AArch64 sysreg handler
  KVM: arm64: Start trapping ID registers for 32 bit guests

 arch/arm64/include/asm/kvm_emulate.h |   8 --
 arch/arm64/include/asm/kvm_host.h    |   1 +
 arch/arm64/kvm/handle_exit.c         |   1 +
 arch/arm64/kvm/sys_regs.c            | 128 +++++++++++++++++++++++++++
 4 files changed, 130 insertions(+), 8 deletions(-)

-- 
2.35.1.1021.g381101b075-goog

[PATCH 0/3] KVM: arm64: Limit feature register reads from AArch32

[Oliver Upton]

KVM/arm64 does not restrict the guest's view of the AArch32 feature
registers when read from AArch32. HCR_EL2.TID3 is cleared for AArch32
guests, meaning that register reads come straight from hardware. This is
problematic as KVM relies on read_sanitised_ftr_reg() to expose a set of
features consistent for a particular system.

Appropriate handlers must first be put in place for CP10 and CP15 ID
register accesses before setting TID3. Rather than exhaustively
enumerating each of the encodings for CP10 and CP15 registers, take the
lazy route and aim the register accesses at the AArch64 system register
table.

Patch 1 reroutes the CP15 registers into the AArch64 table, taking care
to immediately RAZ undefined ranges of registers. This is done to avoid
possibly conflicting with encodings for future AArch64 registers.

Patch 2 installs an exit handler for the CP10 ID registers and also
relies on the general AArch64 register handler to implement reads.

Finally, patch 3 actually sets TID3 for AArch32 guests, providing
known-safe values for feature register accesses.

I'll leave it as an exercise for the reader to decide whether or not I'm
being _too_ lazy here ;-)

Series applies cleanly to kvmarm/fixes at commit:

  8872d9b3e35a ("KVM: arm64: Drop unneeded minor version check from PSCI v1.x handler")

Tested with AArch32 kvm-unit-tests and booting an AArch32 debian image
on a Raspberry Pi 4. Nothing seems to have gone up in smoke yet...

Oliver Upton (3):
  KVM: arm64: Wire up CP15 feature registers to their AArch64
    equivalents
  KVM: arm64: Plumb cp10 ID traps through the AArch64 sysreg handler
  KVM: arm64: Start trapping ID registers for 32 bit guests

 arch/arm64/include/asm/kvm_emulate.h |   8 --
 arch/arm64/include/asm/kvm_host.h    |   1 +
 arch/arm64/kvm/handle_exit.c         |   1 +
 arch/arm64/kvm/sys_regs.c            | 128 +++++++++++++++++++++++++++
 4 files changed, 130 insertions(+), 8 deletions(-)

-- 
2.35.1.1021.g381101b075-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Oliver Upton]

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;
+
+	/*
+	 * 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);
-- 
2.35.1.1021.g381101b075-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

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

[Oliver Upton]

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;
+
+	/*
+	 * 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);
-- 
2.35.1.1021.g381101b075-goog

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

[Oliver Upton]

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;
+
+	/*
+	 * 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);
-- 
2.35.1.1021.g381101b075-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Reiji Watanabe]

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
>
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

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

[Reiji Watanabe]

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
>

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

[Reiji Watanabe]

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
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Oliver Upton]

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

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

[Oliver Upton]

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

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

[Oliver Upton]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/3] KVM: arm64: Plumb cp10 ID traps through the AArch64 sysreg handler

[Oliver Upton]

In order to enable HCR_EL2.TID3 for AArch32 guests KVM needs to handle
traps where ESR_EL2.EC=0x8, which corresponds to an attempted VMRS
access from an ID group register. Specifically, the MVFR{0-2} registers
are accessed this way from AArch32. Conveniently, these registers are
architecturally mapped to MVFR{0-2}_EL1 in AArch64. Furthermore, KVM
already handles reads to these aliases in AArch64.

Plumb VMRS read traps through to the general AArch64 system register
handler.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 arch/arm64/include/asm/kvm_host.h |  1 +
 arch/arm64/kvm/handle_exit.c      |  1 +
 arch/arm64/kvm/sys_regs.c         | 62 +++++++++++++++++++++++++++++++
 3 files changed, 64 insertions(+)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 0e96087885fe..7a65ac268a22 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -673,6 +673,7 @@ int kvm_handle_cp14_64(struct kvm_vcpu *vcpu);
 int kvm_handle_cp15_32(struct kvm_vcpu *vcpu);
 int kvm_handle_cp15_64(struct kvm_vcpu *vcpu);
 int kvm_handle_sys_reg(struct kvm_vcpu *vcpu);
+int kvm_handle_cp10_id(struct kvm_vcpu *vcpu);
 
 void kvm_reset_sys_regs(struct kvm_vcpu *vcpu);
 
diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
index 97fe14aab1a3..5088a86ace5b 100644
--- a/arch/arm64/kvm/handle_exit.c
+++ b/arch/arm64/kvm/handle_exit.c
@@ -167,6 +167,7 @@ static exit_handle_fn arm_exit_handlers[] = {
 	[ESR_ELx_EC_CP15_64]	= kvm_handle_cp15_64,
 	[ESR_ELx_EC_CP14_MR]	= kvm_handle_cp14_32,
 	[ESR_ELx_EC_CP14_LS]	= kvm_handle_cp14_load_store,
+	[ESR_ELx_EC_CP10_ID]	= kvm_handle_cp10_id,
 	[ESR_ELx_EC_CP14_64]	= kvm_handle_cp14_64,
 	[ESR_ELx_EC_HVC32]	= handle_hvc,
 	[ESR_ELx_EC_SMC32]	= handle_smc,
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 30771f950027..1caac72b0cb0 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -2341,6 +2341,68 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu,
 
 static int emulate_sys_reg(struct kvm_vcpu *vcpu, struct sys_reg_params *params);
 
+/*
+ * The CP10 ID registers are architecturally mapped to AArch64 feature
+ * registers. Abuse that fact so we can rely on the AArch64 handler for accesses
+ * from AArch32.
+ */
+static bool kvm_esr_cp10_id_to_sys64(u32 esr, struct sys_reg_params *params)
+{
+	params->is_write = ((esr & 1) == 0);
+	params->Op0 = 3;
+	params->Op1 = 0;
+	params->CRn = 0;
+	params->CRm = 3;
+
+	switch ((esr >> 10) & 0xf) {
+	/* MVFR0 */
+	case 0b0111:
+		params->Op2 = 0;
+		break;
+	/* MVFR1 */
+	case 0b0110:
+		params->Op2 = 1;
+		break;
+	/* MVFR2 */
+	case 0b0101:
+		params->Op2 = 2;
+		break;
+	default:
+		return false;
+	}
+
+	return true;
+}
+
+/**
+ * kvm_handle_cp10_id() - Handles a VMRS trap on guest access to a 'Media and
+ *			  VFP Register' from AArch32.
+ * @vcpu: The vCPU pointer
+ *
+ * MVFR{0-2} are architecturally mapped to the AArch64 MVFR{0-2}_EL1 registers.
+ * Work out the correct AArch64 system register encoding and reroute to the
+ * AArch64 system register emulation.
+ */
+int kvm_handle_cp10_id(struct kvm_vcpu *vcpu)
+{
+	int Rt = kvm_vcpu_sys_get_rt(vcpu);
+	u32 esr = kvm_vcpu_get_esr(vcpu);
+	struct sys_reg_params params;
+	int ret;
+
+	/* UNDEF on any unhandled register or an attempted write */
+	if (!kvm_esr_cp10_id_to_sys64(esr, &params) || params.is_write) {
+		kvm_inject_undefined(vcpu);
+		return 1;
+	}
+
+	params.regval = vcpu_get_reg(vcpu, Rt);
+	ret = emulate_sys_reg(vcpu, &params);
+
+	vcpu_set_reg(vcpu, Rt, params.regval);
+	return ret;
+}
+
 /**
  * kvm_emulate_cp15_id_reg() - Handles an MRC trap on a guest CP15 access where
  *			       CRn=0, which corresponds to the AArch32 feature
-- 
2.35.1.1021.g381101b075-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 2/3] KVM: arm64: Plumb cp10 ID traps through the AArch64 sysreg handler

[Oliver Upton]

In order to enable HCR_EL2.TID3 for AArch32 guests KVM needs to handle
traps where ESR_EL2.EC=0x8, which corresponds to an attempted VMRS
access from an ID group register. Specifically, the MVFR{0-2} registers
are accessed this way from AArch32. Conveniently, these registers are
architecturally mapped to MVFR{0-2}_EL1 in AArch64. Furthermore, KVM
already handles reads to these aliases in AArch64.

Plumb VMRS read traps through to the general AArch64 system register
handler.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 arch/arm64/include/asm/kvm_host.h |  1 +
 arch/arm64/kvm/handle_exit.c      |  1 +
 arch/arm64/kvm/sys_regs.c         | 62 +++++++++++++++++++++++++++++++
 3 files changed, 64 insertions(+)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 0e96087885fe..7a65ac268a22 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -673,6 +673,7 @@ int kvm_handle_cp14_64(struct kvm_vcpu *vcpu);
 int kvm_handle_cp15_32(struct kvm_vcpu *vcpu);
 int kvm_handle_cp15_64(struct kvm_vcpu *vcpu);
 int kvm_handle_sys_reg(struct kvm_vcpu *vcpu);
+int kvm_handle_cp10_id(struct kvm_vcpu *vcpu);
 
 void kvm_reset_sys_regs(struct kvm_vcpu *vcpu);
 
diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
index 97fe14aab1a3..5088a86ace5b 100644
--- a/arch/arm64/kvm/handle_exit.c
+++ b/arch/arm64/kvm/handle_exit.c
@@ -167,6 +167,7 @@ static exit_handle_fn arm_exit_handlers[] = {
 	[ESR_ELx_EC_CP15_64]	= kvm_handle_cp15_64,
 	[ESR_ELx_EC_CP14_MR]	= kvm_handle_cp14_32,
 	[ESR_ELx_EC_CP14_LS]	= kvm_handle_cp14_load_store,
+	[ESR_ELx_EC_CP10_ID]	= kvm_handle_cp10_id,
 	[ESR_ELx_EC_CP14_64]	= kvm_handle_cp14_64,
 	[ESR_ELx_EC_HVC32]	= handle_hvc,
 	[ESR_ELx_EC_SMC32]	= handle_smc,
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 30771f950027..1caac72b0cb0 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -2341,6 +2341,68 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu,
 
 static int emulate_sys_reg(struct kvm_vcpu *vcpu, struct sys_reg_params *params);
 
+/*
+ * The CP10 ID registers are architecturally mapped to AArch64 feature
+ * registers. Abuse that fact so we can rely on the AArch64 handler for accesses
+ * from AArch32.
+ */
+static bool kvm_esr_cp10_id_to_sys64(u32 esr, struct sys_reg_params *params)
+{
+	params->is_write = ((esr & 1) == 0);
+	params->Op0 = 3;
+	params->Op1 = 0;
+	params->CRn = 0;
+	params->CRm = 3;
+
+	switch ((esr >> 10) & 0xf) {
+	/* MVFR0 */
+	case 0b0111:
+		params->Op2 = 0;
+		break;
+	/* MVFR1 */
+	case 0b0110:
+		params->Op2 = 1;
+		break;
+	/* MVFR2 */
+	case 0b0101:
+		params->Op2 = 2;
+		break;
+	default:
+		return false;
+	}
+
+	return true;
+}
+
+/**
+ * kvm_handle_cp10_id() - Handles a VMRS trap on guest access to a 'Media and
+ *			  VFP Register' from AArch32.
+ * @vcpu: The vCPU pointer
+ *
+ * MVFR{0-2} are architecturally mapped to the AArch64 MVFR{0-2}_EL1 registers.
+ * Work out the correct AArch64 system register encoding and reroute to the
+ * AArch64 system register emulation.
+ */
+int kvm_handle_cp10_id(struct kvm_vcpu *vcpu)
+{
+	int Rt = kvm_vcpu_sys_get_rt(vcpu);
+	u32 esr = kvm_vcpu_get_esr(vcpu);
+	struct sys_reg_params params;
+	int ret;
+
+	/* UNDEF on any unhandled register or an attempted write */
+	if (!kvm_esr_cp10_id_to_sys64(esr, &params) || params.is_write) {
+		kvm_inject_undefined(vcpu);
+		return 1;
+	}
+
+	params.regval = vcpu_get_reg(vcpu, Rt);
+	ret = emulate_sys_reg(vcpu, &params);
+
+	vcpu_set_reg(vcpu, Rt, params.regval);
+	return ret;
+}
+
 /**
  * kvm_emulate_cp15_id_reg() - Handles an MRC trap on a guest CP15 access where
  *			       CRn=0, which corresponds to the AArch32 feature
-- 
2.35.1.1021.g381101b075-goog

[PATCH 2/3] KVM: arm64: Plumb cp10 ID traps through the AArch64 sysreg handler

[Oliver Upton]

In order to enable HCR_EL2.TID3 for AArch32 guests KVM needs to handle
traps where ESR_EL2.EC=0x8, which corresponds to an attempted VMRS
access from an ID group register. Specifically, the MVFR{0-2} registers
are accessed this way from AArch32. Conveniently, these registers are
architecturally mapped to MVFR{0-2}_EL1 in AArch64. Furthermore, KVM
already handles reads to these aliases in AArch64.

Plumb VMRS read traps through to the general AArch64 system register
handler.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 arch/arm64/include/asm/kvm_host.h |  1 +
 arch/arm64/kvm/handle_exit.c      |  1 +
 arch/arm64/kvm/sys_regs.c         | 62 +++++++++++++++++++++++++++++++
 3 files changed, 64 insertions(+)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 0e96087885fe..7a65ac268a22 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -673,6 +673,7 @@ int kvm_handle_cp14_64(struct kvm_vcpu *vcpu);
 int kvm_handle_cp15_32(struct kvm_vcpu *vcpu);
 int kvm_handle_cp15_64(struct kvm_vcpu *vcpu);
 int kvm_handle_sys_reg(struct kvm_vcpu *vcpu);
+int kvm_handle_cp10_id(struct kvm_vcpu *vcpu);
 
 void kvm_reset_sys_regs(struct kvm_vcpu *vcpu);
 
diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
index 97fe14aab1a3..5088a86ace5b 100644
--- a/arch/arm64/kvm/handle_exit.c
+++ b/arch/arm64/kvm/handle_exit.c
@@ -167,6 +167,7 @@ static exit_handle_fn arm_exit_handlers[] = {
 	[ESR_ELx_EC_CP15_64]	= kvm_handle_cp15_64,
 	[ESR_ELx_EC_CP14_MR]	= kvm_handle_cp14_32,
 	[ESR_ELx_EC_CP14_LS]	= kvm_handle_cp14_load_store,
+	[ESR_ELx_EC_CP10_ID]	= kvm_handle_cp10_id,
 	[ESR_ELx_EC_CP14_64]	= kvm_handle_cp14_64,
 	[ESR_ELx_EC_HVC32]	= handle_hvc,
 	[ESR_ELx_EC_SMC32]	= handle_smc,
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 30771f950027..1caac72b0cb0 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -2341,6 +2341,68 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu,
 
 static int emulate_sys_reg(struct kvm_vcpu *vcpu, struct sys_reg_params *params);
 
+/*
+ * The CP10 ID registers are architecturally mapped to AArch64 feature
+ * registers. Abuse that fact so we can rely on the AArch64 handler for accesses
+ * from AArch32.
+ */
+static bool kvm_esr_cp10_id_to_sys64(u32 esr, struct sys_reg_params *params)
+{
+	params->is_write = ((esr & 1) == 0);
+	params->Op0 = 3;
+	params->Op1 = 0;
+	params->CRn = 0;
+	params->CRm = 3;
+
+	switch ((esr >> 10) & 0xf) {
+	/* MVFR0 */
+	case 0b0111:
+		params->Op2 = 0;
+		break;
+	/* MVFR1 */
+	case 0b0110:
+		params->Op2 = 1;
+		break;
+	/* MVFR2 */
+	case 0b0101:
+		params->Op2 = 2;
+		break;
+	default:
+		return false;
+	}
+
+	return true;
+}
+
+/**
+ * kvm_handle_cp10_id() - Handles a VMRS trap on guest access to a 'Media and
+ *			  VFP Register' from AArch32.
+ * @vcpu: The vCPU pointer
+ *
+ * MVFR{0-2} are architecturally mapped to the AArch64 MVFR{0-2}_EL1 registers.
+ * Work out the correct AArch64 system register encoding and reroute to the
+ * AArch64 system register emulation.
+ */
+int kvm_handle_cp10_id(struct kvm_vcpu *vcpu)
+{
+	int Rt = kvm_vcpu_sys_get_rt(vcpu);
+	u32 esr = kvm_vcpu_get_esr(vcpu);
+	struct sys_reg_params params;
+	int ret;
+
+	/* UNDEF on any unhandled register or an attempted write */
+	if (!kvm_esr_cp10_id_to_sys64(esr, &params) || params.is_write) {
+		kvm_inject_undefined(vcpu);
+		return 1;
+	}
+
+	params.regval = vcpu_get_reg(vcpu, Rt);
+	ret = emulate_sys_reg(vcpu, &params);
+
+	vcpu_set_reg(vcpu, Rt, params.regval);
+	return ret;
+}
+
 /**
  * kvm_emulate_cp15_id_reg() - Handles an MRC trap on a guest CP15 access where
  *			       CRn=0, which corresponds to the AArch32 feature
-- 
2.35.1.1021.g381101b075-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/3] KVM: arm64: Start trapping ID registers for 32 bit guests

[Oliver Upton]

To date KVM has not trapped ID register accesses from AArch32, meaning
that guests get an unconstrained view of what hardware supports. This
can be a serious problem because we try to base the guest's feature
registers on values that are safe system-wide. Furthermore, KVM does not
implement the latest ISA in the PMU and Debug architecture, so we
constrain these fields to supported values.

Since KVM now correctly handles CP15 and CP10 register traps, we no
longer need to clear HCR_EL2.TID3 for 32 bit guests and will instead
emulate reads with their safe values.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 arch/arm64/include/asm/kvm_emulate.h | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
index d62405ce3e6d..fe32b4c8b35b 100644
--- a/arch/arm64/include/asm/kvm_emulate.h
+++ b/arch/arm64/include/asm/kvm_emulate.h
@@ -75,14 +75,6 @@ static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
 	if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features))
 		vcpu->arch.hcr_el2 &= ~HCR_RW;
 
-	/*
-	 * TID3: trap feature register accesses that we virtualise.
-	 * For now this is conditional, since no AArch32 feature regs
-	 * are currently virtualised.
-	 */
-	if (!vcpu_el1_is_32bit(vcpu))
-		vcpu->arch.hcr_el2 |= HCR_TID3;
-
 	if (cpus_have_const_cap(ARM64_MISMATCHED_CACHE_TYPE) ||
 	    vcpu_el1_is_32bit(vcpu))
 		vcpu->arch.hcr_el2 |= HCR_TID2;
-- 
2.35.1.1021.g381101b075-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 3/3] KVM: arm64: Start trapping ID registers for 32 bit guests

[Oliver Upton]

To date KVM has not trapped ID register accesses from AArch32, meaning
that guests get an unconstrained view of what hardware supports. This
can be a serious problem because we try to base the guest's feature
registers on values that are safe system-wide. Furthermore, KVM does not
implement the latest ISA in the PMU and Debug architecture, so we
constrain these fields to supported values.

Since KVM now correctly handles CP15 and CP10 register traps, we no
longer need to clear HCR_EL2.TID3 for 32 bit guests and will instead
emulate reads with their safe values.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 arch/arm64/include/asm/kvm_emulate.h | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
index d62405ce3e6d..fe32b4c8b35b 100644
--- a/arch/arm64/include/asm/kvm_emulate.h
+++ b/arch/arm64/include/asm/kvm_emulate.h
@@ -75,14 +75,6 @@ static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
 	if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features))
 		vcpu->arch.hcr_el2 &= ~HCR_RW;
 
-	/*
-	 * TID3: trap feature register accesses that we virtualise.
-	 * For now this is conditional, since no AArch32 feature regs
-	 * are currently virtualised.
-	 */
-	if (!vcpu_el1_is_32bit(vcpu))
-		vcpu->arch.hcr_el2 |= HCR_TID3;
-
 	if (cpus_have_const_cap(ARM64_MISMATCHED_CACHE_TYPE) ||
 	    vcpu_el1_is_32bit(vcpu))
 		vcpu->arch.hcr_el2 |= HCR_TID2;
-- 
2.35.1.1021.g381101b075-goog

[PATCH 3/3] KVM: arm64: Start trapping ID registers for 32 bit guests

[Oliver Upton]

To date KVM has not trapped ID register accesses from AArch32, meaning
that guests get an unconstrained view of what hardware supports. This
can be a serious problem because we try to base the guest's feature
registers on values that are safe system-wide. Furthermore, KVM does not
implement the latest ISA in the PMU and Debug architecture, so we
constrain these fields to supported values.

Since KVM now correctly handles CP15 and CP10 register traps, we no
longer need to clear HCR_EL2.TID3 for 32 bit guests and will instead
emulate reads with their safe values.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 arch/arm64/include/asm/kvm_emulate.h | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
index d62405ce3e6d..fe32b4c8b35b 100644
--- a/arch/arm64/include/asm/kvm_emulate.h
+++ b/arch/arm64/include/asm/kvm_emulate.h
@@ -75,14 +75,6 @@ static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
 	if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features))
 		vcpu->arch.hcr_el2 &= ~HCR_RW;
 
-	/*
-	 * TID3: trap feature register accesses that we virtualise.
-	 * For now this is conditional, since no AArch32 feature regs
-	 * are currently virtualised.
-	 */
-	if (!vcpu_el1_is_32bit(vcpu))
-		vcpu->arch.hcr_el2 |= HCR_TID3;
-
 	if (cpus_have_const_cap(ARM64_MISMATCHED_CACHE_TYPE) ||
 	    vcpu_el1_is_32bit(vcpu))
 		vcpu->arch.hcr_el2 |= HCR_TID2;
-- 
2.35.1.1021.g381101b075-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/3] KVM: arm64: Start trapping ID registers for 32 bit guests

[Oliver Upton]

On Mon, Mar 28, 2022 at 6:13 PM Oliver Upton <oupton@google.com> wrote:
>
> To date KVM has not trapped ID register accesses from AArch32, meaning
> that guests get an unconstrained view of what hardware supports. This
> can be a serious problem because we try to base the guest's feature
> registers on values that are safe system-wide. Furthermore, KVM does not
> implement the latest ISA in the PMU and Debug architecture, so we
> constrain these fields to supported values.
>
> Since KVM now correctly handles CP15 and CP10 register traps, we no
> longer need to clear HCR_EL2.TID3 for 32 bit guests and will instead
> emulate reads with their safe values.
>
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  arch/arm64/include/asm/kvm_emulate.h | 8 --------
>  1 file changed, 8 deletions(-)
>
> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> index d62405ce3e6d..fe32b4c8b35b 100644
> --- a/arch/arm64/include/asm/kvm_emulate.h
> +++ b/arch/arm64/include/asm/kvm_emulate.h
> @@ -75,14 +75,6 @@ static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
>         if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features))
>                 vcpu->arch.hcr_el2 &= ~HCR_RW;
>
> -       /*
> -        * TID3: trap feature register accesses that we virtualise.
> -        * For now this is conditional, since no AArch32 feature regs
> -        * are currently virtualised.
> -        */
> -       if (!vcpu_el1_is_32bit(vcpu))
> -               vcpu->arch.hcr_el2 |= HCR_TID3;
> -

This is obviously wrong. I deleted one too many lines! Will retest and
resend, this time hopefully with register reads _actually_ being
emulated :)
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 3/3] KVM: arm64: Start trapping ID registers for 32 bit guests

[Oliver Upton]

On Mon, Mar 28, 2022 at 6:13 PM Oliver Upton <oupton@google.com> wrote:
>
> To date KVM has not trapped ID register accesses from AArch32, meaning
> that guests get an unconstrained view of what hardware supports. This
> can be a serious problem because we try to base the guest's feature
> registers on values that are safe system-wide. Furthermore, KVM does not
> implement the latest ISA in the PMU and Debug architecture, so we
> constrain these fields to supported values.
>
> Since KVM now correctly handles CP15 and CP10 register traps, we no
> longer need to clear HCR_EL2.TID3 for 32 bit guests and will instead
> emulate reads with their safe values.
>
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  arch/arm64/include/asm/kvm_emulate.h | 8 --------
>  1 file changed, 8 deletions(-)
>
> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> index d62405ce3e6d..fe32b4c8b35b 100644
> --- a/arch/arm64/include/asm/kvm_emulate.h
> +++ b/arch/arm64/include/asm/kvm_emulate.h
> @@ -75,14 +75,6 @@ static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
>         if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features))
>                 vcpu->arch.hcr_el2 &= ~HCR_RW;
>
> -       /*
> -        * TID3: trap feature register accesses that we virtualise.
> -        * For now this is conditional, since no AArch32 feature regs
> -        * are currently virtualised.
> -        */
> -       if (!vcpu_el1_is_32bit(vcpu))
> -               vcpu->arch.hcr_el2 |= HCR_TID3;
> -

This is obviously wrong. I deleted one too many lines! Will retest and
resend, this time hopefully with register reads _actually_ being
emulated :)

Re: [PATCH 3/3] KVM: arm64: Start trapping ID registers for 32 bit guests

[Oliver Upton]

On Mon, Mar 28, 2022 at 6:13 PM Oliver Upton <oupton@google.com> wrote:
>
> To date KVM has not trapped ID register accesses from AArch32, meaning
> that guests get an unconstrained view of what hardware supports. This
> can be a serious problem because we try to base the guest's feature
> registers on values that are safe system-wide. Furthermore, KVM does not
> implement the latest ISA in the PMU and Debug architecture, so we
> constrain these fields to supported values.
>
> Since KVM now correctly handles CP15 and CP10 register traps, we no
> longer need to clear HCR_EL2.TID3 for 32 bit guests and will instead
> emulate reads with their safe values.
>
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  arch/arm64/include/asm/kvm_emulate.h | 8 --------
>  1 file changed, 8 deletions(-)
>
> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> index d62405ce3e6d..fe32b4c8b35b 100644
> --- a/arch/arm64/include/asm/kvm_emulate.h
> +++ b/arch/arm64/include/asm/kvm_emulate.h
> @@ -75,14 +75,6 @@ static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
>         if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features))
>                 vcpu->arch.hcr_el2 &= ~HCR_RW;
>
> -       /*
> -        * TID3: trap feature register accesses that we virtualise.
> -        * For now this is conditional, since no AArch32 feature regs
> -        * are currently virtualised.
> -        */
> -       if (!vcpu_el1_is_32bit(vcpu))
> -               vcpu->arch.hcr_el2 |= HCR_TID3;
> -

This is obviously wrong. I deleted one too many lines! Will retest and
resend, this time hopefully with register reads _actually_ being
emulated :)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/3] KVM: arm64: Limit feature register reads from AArch32

[Oliver Upton]

On Mon, Mar 28, 2022 at 6:13 PM Oliver Upton <oupton@google.com> wrote:
>
> KVM/arm64 does not restrict the guest's view of the AArch32 feature
> registers when read from AArch32. HCR_EL2.TID3 is cleared for AArch32
> guests, meaning that register reads come straight from hardware. This is
> problematic as KVM relies on read_sanitised_ftr_reg() to expose a set of
> features consistent for a particular system.
>
> Appropriate handlers must first be put in place for CP10 and CP15 ID
> register accesses before setting TID3. Rather than exhaustively
> enumerating each of the encodings for CP10 and CP15 registers, take the
> lazy route and aim the register accesses at the AArch64 system register
> table.
>
> Patch 1 reroutes the CP15 registers into the AArch64 table, taking care
> to immediately RAZ undefined ranges of registers. This is done to avoid
> possibly conflicting with encodings for future AArch64 registers.
>
> Patch 2 installs an exit handler for the CP10 ID registers and also
> relies on the general AArch64 register handler to implement reads.
>
> Finally, patch 3 actually sets TID3 for AArch32 guests, providing
> known-safe values for feature register accesses.
>
> I'll leave it as an exercise for the reader to decide whether or not I'm
> being _too_ lazy here ;-)
>
> Series applies cleanly to kvmarm/fixes at commit:
>
>   8872d9b3e35a ("KVM: arm64: Drop unneeded minor version check from PSCI v1.x handler")
>
> Tested with AArch32 kvm-unit-tests and booting an AArch32 debian image
> on a Raspberry Pi 4. Nothing seems to have gone up in smoke yet...

That is to say, I booted an AArch32 debian image as a guest. The host
kernel was of course arm64.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 0/3] KVM: arm64: Limit feature register reads from AArch32

[Oliver Upton]

On Mon, Mar 28, 2022 at 6:13 PM Oliver Upton <oupton@google.com> wrote:
>
> KVM/arm64 does not restrict the guest's view of the AArch32 feature
> registers when read from AArch32. HCR_EL2.TID3 is cleared for AArch32
> guests, meaning that register reads come straight from hardware. This is
> problematic as KVM relies on read_sanitised_ftr_reg() to expose a set of
> features consistent for a particular system.
>
> Appropriate handlers must first be put in place for CP10 and CP15 ID
> register accesses before setting TID3. Rather than exhaustively
> enumerating each of the encodings for CP10 and CP15 registers, take the
> lazy route and aim the register accesses at the AArch64 system register
> table.
>
> Patch 1 reroutes the CP15 registers into the AArch64 table, taking care
> to immediately RAZ undefined ranges of registers. This is done to avoid
> possibly conflicting with encodings for future AArch64 registers.
>
> Patch 2 installs an exit handler for the CP10 ID registers and also
> relies on the general AArch64 register handler to implement reads.
>
> Finally, patch 3 actually sets TID3 for AArch32 guests, providing
> known-safe values for feature register accesses.
>
> I'll leave it as an exercise for the reader to decide whether or not I'm
> being _too_ lazy here ;-)
>
> Series applies cleanly to kvmarm/fixes at commit:
>
>   8872d9b3e35a ("KVM: arm64: Drop unneeded minor version check from PSCI v1.x handler")
>
> Tested with AArch32 kvm-unit-tests and booting an AArch32 debian image
> on a Raspberry Pi 4. Nothing seems to have gone up in smoke yet...

That is to say, I booted an AArch32 debian image as a guest. The host
kernel was of course arm64.

Re: [PATCH 0/3] KVM: arm64: Limit feature register reads from AArch32

[Oliver Upton]

On Mon, Mar 28, 2022 at 6:13 PM Oliver Upton <oupton@google.com> wrote:
>
> KVM/arm64 does not restrict the guest's view of the AArch32 feature
> registers when read from AArch32. HCR_EL2.TID3 is cleared for AArch32
> guests, meaning that register reads come straight from hardware. This is
> problematic as KVM relies on read_sanitised_ftr_reg() to expose a set of
> features consistent for a particular system.
>
> Appropriate handlers must first be put in place for CP10 and CP15 ID
> register accesses before setting TID3. Rather than exhaustively
> enumerating each of the encodings for CP10 and CP15 registers, take the
> lazy route and aim the register accesses at the AArch64 system register
> table.
>
> Patch 1 reroutes the CP15 registers into the AArch64 table, taking care
> to immediately RAZ undefined ranges of registers. This is done to avoid
> possibly conflicting with encodings for future AArch64 registers.
>
> Patch 2 installs an exit handler for the CP10 ID registers and also
> relies on the general AArch64 register handler to implement reads.
>
> Finally, patch 3 actually sets TID3 for AArch32 guests, providing
> known-safe values for feature register accesses.
>
> I'll leave it as an exercise for the reader to decide whether or not I'm
> being _too_ lazy here ;-)
>
> Series applies cleanly to kvmarm/fixes at commit:
>
>   8872d9b3e35a ("KVM: arm64: Drop unneeded minor version check from PSCI v1.x handler")
>
> Tested with AArch32 kvm-unit-tests and booting an AArch32 debian image
> on a Raspberry Pi 4. Nothing seems to have gone up in smoke yet...

That is to say, I booted an AArch32 debian image as a guest. The host
kernel was of course arm64.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel