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.
Patches 1-2 are small cleanups to how we handle register emulation
failure. No functional change for current KVM, but required to do
register emulation correctly in this series.
Patch 3 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 4 installs an exit handler for the CP10 ID registers and also
relies on the general AArch64 register handler to implement reads.
Finally, patch 5 actually sets TID3 for AArch32 guests, providing
known-safe values for feature register accesses.
There is an argument that the series is in fact a bug fix for running
AArch32 VMs on heterogeneous systems. To that end, it could be
blamed/backported to when we first knew better:
93390c0a1b20 ("arm64: KVM: Hide unsupported AArch64 CPU features from guests")
But I left that tag off as in the aforementioned change skipping
AArch32 was intentional. Up to you, Marc, if you want to call it a
bugfix ;-)
Applies cleanly to 5.18-rc4.
Tested with AArch32 kvm-unit-tests and booting an AArch32 debian guest
on a Raspberry Pi 4. Additionally, I tested AArch32 kvm-unit-tests w/
pmu={on,off} and saw no splat, as Alex had discovered [1]. The test
correctly skips with the PMU feature bit disabled now.
[1]: https://lore.kernel.org/r/[email protected]
v1: https://lore.kernel.org/kvmarm/[email protected]/
v2: https://lore.kernel.org/r/[email protected]
v2 -> v3:
- Collect R-b from Reiji (thanks!)
- Adopt Marc's suggestion for CP15 register handling
- Avoid writing to Rt when emulation fails (Marc)
- Print some debug info on an unexpected CP10 register access (Reiji)
v1 -> v2:
- Actually set TID3! Oops.
- Refactor kvm_emulate_cp15_id_reg() to check preconditions before
proceeding to emulation (Reiji)
- Tighten up comment on kvm_is_cp15_id_reg() to indicate that the only
other trapped ID register (CTR) is already handled in the cp15
Oliver Upton (5):
KVM: arm64: Return a bool from emulate_cp()
KVM: arm64: Don't write to Rt unless sys_reg emulation succeeds
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_arm.h | 3 +-
arch/arm64/include/asm/kvm_emulate.h | 7 -
arch/arm64/include/asm/kvm_host.h | 1 +
arch/arm64/kvm/handle_exit.c | 1 +
arch/arm64/kvm/sys_regs.c | 197 +++++++++++++++++++++++----
arch/arm64/kvm/sys_regs.h | 7 +
6 files changed, 178 insertions(+), 38 deletions(-)
--
2.36.0.rc2.479.g8af0fa9b8e-goog
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 <[email protected]>
Reviewed-by: Reiji Watanabe <[email protected]>
---
arch/arm64/include/asm/kvm_arm.h | 3 ++-
arch/arm64/include/asm/kvm_emulate.h | 7 -------
2 files changed, 2 insertions(+), 8 deletions(-)
diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
index 1767ded83888..b5de102928d8 100644
--- a/arch/arm64/include/asm/kvm_arm.h
+++ b/arch/arm64/include/asm/kvm_arm.h
@@ -80,11 +80,12 @@
* FMO: Override CPSR.F and enable signaling with VF
* SWIO: Turn set/way invalidates into set/way clean+invalidate
* PTW: Take a stage2 fault if a stage1 walk steps in device memory
+ * TID3: Trap EL1 reads of group 3 ID registers
*/
#define HCR_GUEST_FLAGS (HCR_TSC | HCR_TSW | HCR_TWE | HCR_TWI | HCR_VM | \
HCR_BSU_IS | HCR_FB | HCR_TACR | \
HCR_AMO | HCR_SWIO | HCR_TIDCP | HCR_RW | HCR_TLOR | \
- HCR_FMO | HCR_IMO | HCR_PTW )
+ HCR_FMO | HCR_IMO | HCR_PTW | HCR_TID3 )
#define HCR_VIRT_EXCP_MASK (HCR_VSE | HCR_VI | HCR_VF)
#define HCR_HOST_NVHE_FLAGS (HCR_RW | HCR_API | HCR_APK | HCR_ATA)
#define HCR_HOST_NVHE_PROTECTED_FLAGS (HCR_HOST_NVHE_FLAGS | HCR_TSC)
diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
index 7496deab025a..ab5c66b77bb0 100644
--- a/arch/arm64/include/asm/kvm_emulate.h
+++ b/arch/arm64/include/asm/kvm_emulate.h
@@ -86,13 +86,6 @@ static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
if (vcpu_el1_is_32bit(vcpu))
vcpu->arch.hcr_el2 &= ~HCR_RW;
- else
- /*
- * TID3: trap feature register accesses that we virtualise.
- * For now this is conditional, since no AArch32 feature regs
- * are currently virtualised.
- */
- vcpu->arch.hcr_el2 |= HCR_TID3;
if (cpus_have_const_cap(ARM64_MISMATCHED_CACHE_TYPE) ||
vcpu_el1_is_32bit(vcpu))
--
2.36.0.rc2.479.g8af0fa9b8e-goog
KVM indicates success/failure in several ways, but generally an integer
is used when conditionally bouncing to userspace is involved. That is
not the case from emulate_cp(); just use a bool instead.
No functional change intended.
Signed-off-by: Oliver Upton <[email protected]>
---
arch/arm64/kvm/sys_regs.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 7b45c040cc27..36895c163eae 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -2246,27 +2246,27 @@ static void perform_access(struct kvm_vcpu *vcpu,
* @table: array of trap descriptors
* @num: size of the trap descriptor array
*
- * Return 0 if the access has been handled, and -1 if not.
+ * Return true if the access has been handled, false if not.
*/
-static int emulate_cp(struct kvm_vcpu *vcpu,
- struct sys_reg_params *params,
- const struct sys_reg_desc *table,
- size_t num)
+static bool emulate_cp(struct kvm_vcpu *vcpu,
+ struct sys_reg_params *params,
+ const struct sys_reg_desc *table,
+ size_t num)
{
const struct sys_reg_desc *r;
if (!table)
- return -1; /* Not handled */
+ return false; /* Not handled */
r = find_reg(params, table, num);
if (r) {
perform_access(vcpu, params, r);
- return 0;
+ return true;
}
/* Not handled */
- return -1;
+ return false;
}
static void unhandled_cp_access(struct kvm_vcpu *vcpu,
@@ -2330,7 +2330,7 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu,
* potential register operation in the case of a read and return
* with success.
*/
- if (!emulate_cp(vcpu, ¶ms, global, nr_global)) {
+ if (emulate_cp(vcpu, ¶ms, global, nr_global)) {
/* Split up the value between registers for the read side */
if (!params.is_write) {
vcpu_set_reg(vcpu, Rt, lower_32_bits(params.regval));
@@ -2365,7 +2365,7 @@ static int kvm_handle_cp_32(struct kvm_vcpu *vcpu,
params.Op1 = (esr >> 14) & 0x7;
params.Op2 = (esr >> 17) & 0x7;
- if (!emulate_cp(vcpu, ¶ms, global, nr_global)) {
+ if (emulate_cp(vcpu, ¶ms, global, nr_global)) {
if (!params.is_write)
vcpu_set_reg(vcpu, Rt, params.regval);
return 1;
--
2.36.0.rc2.479.g8af0fa9b8e-goog
emulate_sys_reg() returns 1 unconditionally, even though a a system
register access can fail. Furthermore, kvm_handle_sys_reg() writes to Rt
for every register read, regardless of if it actually succeeded.
Though this pattern is safe (as params.regval is initialized with the
current value of Rt) it is a bit ugly. Indicate failure if the register
access could not be emulated and only write to Rt on success.
Signed-off-by: Oliver Upton <[email protected]>
---
arch/arm64/kvm/sys_regs.c | 22 ++++++++++++++++------
1 file changed, 16 insertions(+), 6 deletions(-)
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 36895c163eae..f0a076e5cc1c 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -2401,7 +2401,14 @@ static bool is_imp_def_sys_reg(struct sys_reg_params *params)
return params->Op0 == 3 && (params->CRn & 0b1011) == 0b1011;
}
-static int emulate_sys_reg(struct kvm_vcpu *vcpu,
+/**
+ * emulate_sys_reg - Emulate a guest access to an AArch64 system register
+ * @vcpu: The VCPU pointer
+ * @params: Decoded system register parameters
+ *
+ * Return: true if the system register access was successful, false otherwise.
+ */
+static bool emulate_sys_reg(struct kvm_vcpu *vcpu,
struct sys_reg_params *params)
{
const struct sys_reg_desc *r;
@@ -2410,7 +2417,10 @@ static int emulate_sys_reg(struct kvm_vcpu *vcpu,
if (likely(r)) {
perform_access(vcpu, params, r);
- } else if (is_imp_def_sys_reg(params)) {
+ return true;
+ }
+
+ if (is_imp_def_sys_reg(params)) {
kvm_inject_undefined(vcpu);
} else {
print_sys_reg_msg(params,
@@ -2418,7 +2428,7 @@ static int emulate_sys_reg(struct kvm_vcpu *vcpu,
*vcpu_pc(vcpu), *vcpu_cpsr(vcpu));
kvm_inject_undefined(vcpu);
}
- return 1;
+ return false;
}
/**
@@ -2446,18 +2456,18 @@ int kvm_handle_sys_reg(struct kvm_vcpu *vcpu)
struct sys_reg_params params;
unsigned long esr = kvm_vcpu_get_esr(vcpu);
int Rt = kvm_vcpu_sys_get_rt(vcpu);
- int ret;
trace_kvm_handle_sys_reg(esr);
params = esr_sys64_to_params(esr);
params.regval = vcpu_get_reg(vcpu, Rt);
- ret = emulate_sys_reg(vcpu, ¶ms);
+ if (!emulate_sys_reg(vcpu, ¶ms))
+ return 1;
if (!params.is_write)
vcpu_set_reg(vcpu, Rt, params.regval);
- return ret;
+ return 1;
}
/******************************************************************************
--
2.36.0.rc2.479.g8af0fa9b8e-goog
Hi,
On Mon, Apr 25, 2022 at 11:53:37PM +0000, Oliver Upton 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.
>
> Patches 1-2 are small cleanups to how we handle register emulation
> failure. No functional change for current KVM, but required to do
> register emulation correctly in this series.
>
> Patch 3 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 4 installs an exit handler for the CP10 ID registers and also
> relies on the general AArch64 register handler to implement reads.
>
> Finally, patch 5 actually sets TID3 for AArch32 guests, providing
> known-safe values for feature register accesses.
>
> There is an argument that the series is in fact a bug fix for running
> AArch32 VMs on heterogeneous systems. To that end, it could be
> blamed/backported to when we first knew better:
>
> 93390c0a1b20 ("arm64: KVM: Hide unsupported AArch64 CPU features from guests")
>
> But I left that tag off as in the aforementioned change skipping
> AArch32 was intentional. Up to you, Marc, if you want to call it a
> bugfix ;-)
>
> Applies cleanly to 5.18-rc4.
>
> Tested with AArch32 kvm-unit-tests and booting an AArch32 debian guest
> on a Raspberry Pi 4. Additionally, I tested AArch32 kvm-unit-tests w/
> pmu={on,off} and saw no splat, as Alex had discovered [1]. The test
> correctly skips with the PMU feature bit disabled now.
But a guest who ignores the fact that the ID register doesn't advertise a PMU
and tries to access the PMU registers regardless would still trigger the splat,
right? I don't think the series changes the AArch32 PMU registers visibility to
REG_HIDDEN when the VCPU feature is not set.
Thanks,
Alex
>
> [1]: https://lore.kernel.org/r/[email protected]
>
> v1: https://lore.kernel.org/kvmarm/[email protected]/
> v2: https://lore.kernel.org/r/[email protected]
>
> v2 -> v3:
> - Collect R-b from Reiji (thanks!)
> - Adopt Marc's suggestion for CP15 register handling
> - Avoid writing to Rt when emulation fails (Marc)
> - Print some debug info on an unexpected CP10 register access (Reiji)
>
> v1 -> v2:
> - Actually set TID3! Oops.
> - Refactor kvm_emulate_cp15_id_reg() to check preconditions before
> proceeding to emulation (Reiji)
> - Tighten up comment on kvm_is_cp15_id_reg() to indicate that the only
> other trapped ID register (CTR) is already handled in the cp15
>
> Oliver Upton (5):
> KVM: arm64: Return a bool from emulate_cp()
> KVM: arm64: Don't write to Rt unless sys_reg emulation succeeds
> 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_arm.h | 3 +-
> arch/arm64/include/asm/kvm_emulate.h | 7 -
> arch/arm64/include/asm/kvm_host.h | 1 +
> arch/arm64/kvm/handle_exit.c | 1 +
> arch/arm64/kvm/sys_regs.c | 197 +++++++++++++++++++++++----
> arch/arm64/kvm/sys_regs.h | 7 +
> 6 files changed, 178 insertions(+), 38 deletions(-)
>
> --
> 2.36.0.rc2.479.g8af0fa9b8e-goog
>