* [PATCH v8 0/6] Support writable CPU ID registers from userspace
@ 2023-05-03 17:16 Jing Zhang
2023-05-03 17:16 ` [PATCH v8 1/6] KVM: arm64: Move CPU ID feature registers emulation into a separate file Jing Zhang
` (6 more replies)
0 siblings, 7 replies; 34+ messages in thread
From: Jing Zhang @ 2023-05-03 17:16 UTC (permalink / raw)
To: KVM, KVMARM, ARMLinux, Marc Zyngier, Oliver Upton
Cc: Will Deacon, Paolo Bonzini, James Morse, Alexandru Elisei,
Suzuki K Poulose, Fuad Tabba, Reiji Watanabe,
Raghavendra Rao Ananta, Jing Zhang
This patchset refactors/adds code to support writable per guest CPU ID feature
registers. Part of the code/ideas are from
https://lore.kernel.org/all/20220419065544.3616948-1-reijiw@google.com .
No functional change is intended in this patchset. With the new CPU ID feature
registers infrastructure, only writtings of ID_AA64PFR0_EL1.[CSV2|CSV3],
ID_AA64DFR0_EL1.PMUVer and ID_DFR0_ELF.PerfMon are allowed as KVM does before.
Writable (Configurable) per guest CPU ID feature registers are useful for
creating/migrating guest on ARM CPUs with different kinds of features.
This patchset uses kvm->arch.config_lock from Oliver's lock inversion fixes at
https://lore.kernel.org/linux-arm-kernel/20230327164747.2466958-1-oliver.upton@linux.dev/
---
* v7 -> v8
- Move idregs table sanity check to kvm_sys_reg_table_init.
- Only allow userspace writing before VM running.
- No lock is hold for guest access to idregs.
- Addressed some other comments from Reiji and Oliver.
* v6 -> v7
- Rebased to v6.3-rc7.
- Add helpers for idregs read/write.
- Guard all idregs reads/writes.
- Add code to fix features' safe value type which is different for KVM than
for the host.
* v5 -> v6
- Rebased to v6.3-rc5.
- Reuse struct sys_reg_desc's reset() callback and field val for KVM.
sanitisation function and writable mask instead of creating a new data
structure for idregs.
- Use get_arm64_ftr_reg() instead of exposing idregs ftr_bits array.
* v4 -> v5
- Rebased to 2fad20ae05cb (kvmarm/next)
Merge branch kvm-arm64/selftest/misc-6,4 into kvmarm-master/next
- Use kvm->arch.config_lock to guard update to multiple VM scope idregs
to avoid lock inversion
- Add back IDREG() macro for idregs access
- Refactor struct id_reg_desc by using existing infrastructure.
- Addressed many other comments from Marc.
* v3 -> v4
- Remove IDREG() macro for ID reg access, use simple array access instead
- Rename kvm_arm_read_id_reg_with_encoding() to kvm_arm_read_id_reg()
- Save perfmon value in ID_DFR0_EL1 instead of pmuver
- Update perfmon in ID_DFR0_EL1 and pmuver in ID_AA64DFR0_EL1 atomically
- Remove kvm_vcpu_has_pmu() in macro kvm_pmu_is_3p5()
- Improve ID register sanity checking in kvm_arm_check_idreg_table()
* v2 -> v3
- Rebased to 96a4627dbbd4 (kvmarm/next)
Merge tag ' https://github.com/oupton/linux tags/kvmarm-6.3' from into kvmarm-master/next
- Add id registere emulation entry point function emulate_id_reg
- Fix consistency for ID_AA64DFR0_EL1.PMUVer and ID_DFR0_EL1.PerfMon
- Improve the checking for id register table by ensuring that every entry has
the correct id register encoding.
- Addressed other comments from Reiji and Marc.
* v1 -> v2
- Rebase to 7121a2e1d107 (kvmarm/next) Merge branch kvm-arm64/nv-prefix into kvmarm/next
- Address writing issue for PMUVer
[1] https://lore.kernel.org/all/20230201025048.205820-1-jingzhangos@google.com
[2] https://lore.kernel.org/all/20230212215830.2975485-1-jingzhangos@google.com
[3] https://lore.kernel.org/all/20230228062246.1222387-1-jingzhangos@google.com
[4] https://lore.kernel.org/all/20230317050637.766317-1-jingzhangos@google.com
[5] https://lore.kernel.org/all/20230402183735.3011540-1-jingzhangos@google.com
[6] https://lore.kernel.org/all/20230404035344.4043856-1-jingzhangos@google.com
[7] https://lore.kernel.org/all/20230424234704.2571444-1-jingzhangos@google.com
---
Jing Zhang (6):
KVM: arm64: Move CPU ID feature registers emulation into a separate
file
KVM: arm64: Save ID registers' sanitized value per guest
KVM: arm64: Use per guest ID register for ID_AA64PFR0_EL1.[CSV2|CSV3]
KVM: arm64: Use per guest ID register for ID_AA64DFR0_EL1.PMUVer
KVM: arm64: Reuse fields of sys_reg_desc for idreg
KVM: arm64: Refactor writings for PMUVer/CSV2/CSV3
arch/arm64/include/asm/cpufeature.h | 1 +
arch/arm64/include/asm/kvm_host.h | 33 +-
arch/arm64/kernel/cpufeature.c | 2 +-
arch/arm64/kvm/Makefile | 2 +-
arch/arm64/kvm/arm.c | 24 +-
arch/arm64/kvm/id_regs.c | 717 ++++++++++++++++++++++++++++
arch/arm64/kvm/sys_regs.c | 534 ++++-----------------
arch/arm64/kvm/sys_regs.h | 28 +-
include/kvm/arm_pmu.h | 5 +-
9 files changed, 864 insertions(+), 482 deletions(-)
create mode 100644 arch/arm64/kvm/id_regs.c
base-commit: 6a8f57ae2eb07ab39a6f0ccad60c760743051026
--
2.40.1.495.gc816e09b53d-goog
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v8 1/6] KVM: arm64: Move CPU ID feature registers emulation into a separate file
2023-05-03 17:16 [PATCH v8 0/6] Support writable CPU ID registers from userspace Jing Zhang
@ 2023-05-03 17:16 ` Jing Zhang
2023-05-16 16:11 ` Marc Zyngier
2023-05-03 17:16 ` [PATCH v8 2/6] KVM: arm64: Save ID registers' sanitized value per guest Jing Zhang
` (5 subsequent siblings)
6 siblings, 1 reply; 34+ messages in thread
From: Jing Zhang @ 2023-05-03 17:16 UTC (permalink / raw)
To: KVM, KVMARM, ARMLinux, Marc Zyngier, Oliver Upton
Cc: Will Deacon, Paolo Bonzini, James Morse, Alexandru Elisei,
Suzuki K Poulose, Fuad Tabba, Reiji Watanabe,
Raghavendra Rao Ananta, Jing Zhang
Create a new file id_regs.c for CPU ID feature registers emulation code,
which are moved from sys_regs.c and tweak sys_regs code accordingly.
No functional change intended.
Signed-off-by: Jing Zhang <jingzhangos@google.com>
---
arch/arm64/kvm/Makefile | 2 +-
arch/arm64/kvm/id_regs.c | 460 +++++++++++++++++++++++++++++++++++++
arch/arm64/kvm/sys_regs.c | 464 ++++----------------------------------
arch/arm64/kvm/sys_regs.h | 19 ++
4 files changed, 519 insertions(+), 426 deletions(-)
create mode 100644 arch/arm64/kvm/id_regs.c
diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
index c0c050e53157..a6a315fcd81e 100644
--- a/arch/arm64/kvm/Makefile
+++ b/arch/arm64/kvm/Makefile
@@ -13,7 +13,7 @@ obj-$(CONFIG_KVM) += hyp/
kvm-y += arm.o mmu.o mmio.o psci.o hypercalls.o pvtime.o \
inject_fault.o va_layout.o handle_exit.o \
guest.o debug.o reset.o sys_regs.o stacktrace.o \
- vgic-sys-reg-v3.o fpsimd.o pkvm.o \
+ vgic-sys-reg-v3.o fpsimd.o pkvm.o id_regs.o \
arch_timer.o trng.o vmid.o emulate-nested.o nested.o \
vgic/vgic.o vgic/vgic-init.o \
vgic/vgic-irqfd.o vgic/vgic-v2.o \
diff --git a/arch/arm64/kvm/id_regs.c b/arch/arm64/kvm/id_regs.c
new file mode 100644
index 000000000000..96b4c43a5100
--- /dev/null
+++ b/arch/arm64/kvm/id_regs.c
@@ -0,0 +1,460 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2023 - Google LLC
+ * Author: Jing Zhang <jingzhangos@google.com>
+ *
+ * Moved from arch/arm64/kvm/sys_regs.c
+ * Copyright (C) 2012,2013 - ARM Ltd
+ * Author: Marc Zyngier <marc.zyngier@arm.com>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bsearch.h>
+#include <linux/kvm_host.h>
+#include <asm/kvm_emulate.h>
+#include <asm/sysreg.h>
+#include <asm/cpufeature.h>
+#include <asm/kvm_nested.h>
+
+#include "sys_regs.h"
+
+static u8 vcpu_pmuver(const struct kvm_vcpu *vcpu)
+{
+ if (kvm_vcpu_has_pmu(vcpu))
+ return vcpu->kvm->arch.dfr0_pmuver.imp;
+
+ return vcpu->kvm->arch.dfr0_pmuver.unimp;
+}
+
+static u8 perfmon_to_pmuver(u8 perfmon)
+{
+ switch (perfmon) {
+ case ID_DFR0_EL1_PerfMon_PMUv3:
+ return ID_AA64DFR0_EL1_PMUVer_IMP;
+ case ID_DFR0_EL1_PerfMon_IMPDEF:
+ return ID_AA64DFR0_EL1_PMUVer_IMP_DEF;
+ default:
+ /* Anything ARMv8.1+ and NI have the same value. For now. */
+ return perfmon;
+ }
+}
+
+static u8 pmuver_to_perfmon(u8 pmuver)
+{
+ switch (pmuver) {
+ case ID_AA64DFR0_EL1_PMUVer_IMP:
+ return ID_DFR0_EL1_PerfMon_PMUv3;
+ case ID_AA64DFR0_EL1_PMUVer_IMP_DEF:
+ return ID_DFR0_EL1_PerfMon_IMPDEF;
+ default:
+ /* Anything ARMv8.1+ and NI have the same value. For now. */
+ return pmuver;
+ }
+}
+
+/* Read a sanitised cpufeature ID register by sys_reg_desc */
+static u64 read_id_reg(const struct kvm_vcpu *vcpu, struct sys_reg_desc const *r)
+{
+ u32 id = reg_to_encoding(r);
+ u64 val;
+
+ if (sysreg_visible_as_raz(vcpu, r))
+ return 0;
+
+ val = read_sanitised_ftr_reg(id);
+
+ switch (id) {
+ case SYS_ID_AA64PFR0_EL1:
+ if (!vcpu_has_sve(vcpu))
+ val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_SVE);
+ val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_AMU);
+ val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV2);
+ val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV2),
+ (u64)vcpu->kvm->arch.pfr0_csv2);
+ val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV3);
+ val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV3),
+ (u64)vcpu->kvm->arch.pfr0_csv3);
+ if (kvm_vgic_global_state.type == VGIC_V3) {
+ val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_GIC);
+ val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_GIC), 1);
+ }
+ break;
+ case SYS_ID_AA64PFR1_EL1:
+ if (!kvm_has_mte(vcpu->kvm))
+ val &= ~ARM64_FEATURE_MASK(ID_AA64PFR1_EL1_MTE);
+
+ val &= ~ARM64_FEATURE_MASK(ID_AA64PFR1_EL1_SME);
+ break;
+ case SYS_ID_AA64ISAR1_EL1:
+ if (!vcpu_has_ptrauth(vcpu))
+ val &= ~(ARM64_FEATURE_MASK(ID_AA64ISAR1_EL1_APA) |
+ ARM64_FEATURE_MASK(ID_AA64ISAR1_EL1_API) |
+ ARM64_FEATURE_MASK(ID_AA64ISAR1_EL1_GPA) |
+ ARM64_FEATURE_MASK(ID_AA64ISAR1_EL1_GPI));
+ break;
+ case SYS_ID_AA64ISAR2_EL1:
+ if (!vcpu_has_ptrauth(vcpu))
+ val &= ~(ARM64_FEATURE_MASK(ID_AA64ISAR2_EL1_APA3) |
+ ARM64_FEATURE_MASK(ID_AA64ISAR2_EL1_GPA3));
+ if (!cpus_have_final_cap(ARM64_HAS_WFXT))
+ val &= ~ARM64_FEATURE_MASK(ID_AA64ISAR2_EL1_WFxT);
+ break;
+ case SYS_ID_AA64DFR0_EL1:
+ /* Limit debug to ARMv8.0 */
+ val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_DebugVer);
+ val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_DebugVer), 6);
+ /* Set PMUver to the required version */
+ val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer);
+ val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer),
+ vcpu_pmuver(vcpu));
+ /* Hide SPE from guests */
+ val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMSVer);
+ break;
+ case SYS_ID_DFR0_EL1:
+ val &= ~ARM64_FEATURE_MASK(ID_DFR0_EL1_PerfMon);
+ val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_DFR0_EL1_PerfMon),
+ pmuver_to_perfmon(vcpu_pmuver(vcpu)));
+ break;
+ case SYS_ID_AA64MMFR2_EL1:
+ val &= ~ID_AA64MMFR2_EL1_CCIDX_MASK;
+ break;
+ case SYS_ID_MMFR4_EL1:
+ val &= ~ARM64_FEATURE_MASK(ID_MMFR4_EL1_CCIDX);
+ break;
+ }
+
+ return val;
+}
+
+/* cpufeature ID register access trap handlers */
+
+static bool access_id_reg(struct kvm_vcpu *vcpu,
+ struct sys_reg_params *p,
+ const struct sys_reg_desc *r)
+{
+ if (p->is_write)
+ return write_to_read_only(vcpu, p, r);
+
+ p->regval = read_id_reg(vcpu, r);
+ if (vcpu_has_nv(vcpu))
+ access_nested_id_reg(vcpu, p, r);
+
+ return true;
+}
+
+/*
+ * cpufeature ID register user accessors
+ *
+ * For now, these registers are immutable for userspace, so no values
+ * are stored, and for set_id_reg() we don't allow the effective value
+ * to be changed.
+ */
+static int get_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
+ u64 *val)
+{
+ *val = read_id_reg(vcpu, rd);
+ return 0;
+}
+
+static int set_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
+ u64 val)
+{
+ /* This is what we mean by invariant: you can't change it. */
+ if (val != read_id_reg(vcpu, rd))
+ return -EINVAL;
+
+ return 0;
+}
+
+static unsigned int id_visibility(const struct kvm_vcpu *vcpu,
+ const struct sys_reg_desc *r)
+{
+ u32 id = reg_to_encoding(r);
+
+ switch (id) {
+ case SYS_ID_AA64ZFR0_EL1:
+ if (!vcpu_has_sve(vcpu))
+ return REG_RAZ;
+ break;
+ }
+
+ return 0;
+}
+
+static unsigned int aa32_id_visibility(const struct kvm_vcpu *vcpu,
+ const struct sys_reg_desc *r)
+{
+ /*
+ * AArch32 ID registers are UNKNOWN if AArch32 isn't implemented at any
+ * EL. Promote to RAZ/WI in order to guarantee consistency between
+ * systems.
+ */
+ if (!kvm_supports_32bit_el0())
+ return REG_RAZ | REG_USER_WI;
+
+ return id_visibility(vcpu, r);
+}
+
+static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
+ const struct sys_reg_desc *rd,
+ u64 val)
+{
+ u8 csv2, csv3;
+
+ /*
+ * Allow AA64PFR0_EL1.CSV2 to be set from userspace as long as
+ * it doesn't promise more than what is actually provided (the
+ * guest could otherwise be covered in ectoplasmic residue).
+ */
+ csv2 = cpuid_feature_extract_unsigned_field(val, ID_AA64PFR0_EL1_CSV2_SHIFT);
+ if (csv2 > 1 || (csv2 && arm64_get_spectre_v2_state() != SPECTRE_UNAFFECTED))
+ return -EINVAL;
+
+ /* Same thing for CSV3 */
+ csv3 = cpuid_feature_extract_unsigned_field(val, ID_AA64PFR0_EL1_CSV3_SHIFT);
+ if (csv3 > 1 || (csv3 && arm64_get_meltdown_state() != SPECTRE_UNAFFECTED))
+ return -EINVAL;
+
+ /* We can only differ with CSV[23], and anything else is an error */
+ val ^= read_id_reg(vcpu, rd);
+ val &= ~(ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV2) |
+ ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV3));
+ if (val)
+ return -EINVAL;
+
+ vcpu->kvm->arch.pfr0_csv2 = csv2;
+ vcpu->kvm->arch.pfr0_csv3 = csv3;
+
+ return 0;
+}
+
+static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
+ const struct sys_reg_desc *rd,
+ u64 val)
+{
+ u8 pmuver, host_pmuver;
+ bool valid_pmu;
+
+ host_pmuver = kvm_arm_pmu_get_pmuver_limit();
+
+ /*
+ * Allow AA64DFR0_EL1.PMUver to be set from userspace as long
+ * as it doesn't promise more than what the HW gives us. We
+ * allow an IMPDEF PMU though, only if no PMU is supported
+ * (KVM backward compatibility handling).
+ */
+ pmuver = FIELD_GET(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer), val);
+ if ((pmuver != ID_AA64DFR0_EL1_PMUVer_IMP_DEF && pmuver > host_pmuver))
+ return -EINVAL;
+
+ valid_pmu = (pmuver != 0 && pmuver != ID_AA64DFR0_EL1_PMUVer_IMP_DEF);
+
+ /* Make sure view register and PMU support do match */
+ if (kvm_vcpu_has_pmu(vcpu) != valid_pmu)
+ return -EINVAL;
+
+ /* We can only differ with PMUver, and anything else is an error */
+ val ^= read_id_reg(vcpu, rd);
+ val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer);
+ if (val)
+ return -EINVAL;
+
+ if (valid_pmu)
+ vcpu->kvm->arch.dfr0_pmuver.imp = pmuver;
+ else
+ vcpu->kvm->arch.dfr0_pmuver.unimp = pmuver;
+
+ return 0;
+}
+
+static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
+ const struct sys_reg_desc *rd,
+ u64 val)
+{
+ u8 perfmon, host_perfmon;
+ bool valid_pmu;
+
+ host_perfmon = pmuver_to_perfmon(kvm_arm_pmu_get_pmuver_limit());
+
+ /*
+ * Allow DFR0_EL1.PerfMon to be set from userspace as long as
+ * it doesn't promise more than what the HW gives us on the
+ * AArch64 side (as everything is emulated with that), and
+ * that this is a PMUv3.
+ */
+ perfmon = FIELD_GET(ARM64_FEATURE_MASK(ID_DFR0_EL1_PerfMon), val);
+ if ((perfmon != ID_DFR0_EL1_PerfMon_IMPDEF && perfmon > host_perfmon) ||
+ (perfmon != 0 && perfmon < ID_DFR0_EL1_PerfMon_PMUv3))
+ return -EINVAL;
+
+ valid_pmu = (perfmon != 0 && perfmon != ID_DFR0_EL1_PerfMon_IMPDEF);
+
+ /* Make sure view register and PMU support do match */
+ if (kvm_vcpu_has_pmu(vcpu) != valid_pmu)
+ return -EINVAL;
+
+ /* We can only differ with PerfMon, and anything else is an error */
+ val ^= read_id_reg(vcpu, rd);
+ val &= ~ARM64_FEATURE_MASK(ID_DFR0_EL1_PerfMon);
+ if (val)
+ return -EINVAL;
+
+ if (valid_pmu)
+ vcpu->kvm->arch.dfr0_pmuver.imp = perfmon_to_pmuver(perfmon);
+ else
+ vcpu->kvm->arch.dfr0_pmuver.unimp = perfmon_to_pmuver(perfmon);
+
+ return 0;
+}
+
+/* sys_reg_desc initialiser for known cpufeature ID registers */
+#define ID_SANITISED(name) { \
+ SYS_DESC(SYS_##name), \
+ .access = access_id_reg, \
+ .get_user = get_id_reg, \
+ .set_user = set_id_reg, \
+ .visibility = id_visibility, \
+}
+
+/* sys_reg_desc initialiser for known cpufeature ID registers */
+#define AA32_ID_SANITISED(name) { \
+ SYS_DESC(SYS_##name), \
+ .access = access_id_reg, \
+ .get_user = get_id_reg, \
+ .set_user = set_id_reg, \
+ .visibility = aa32_id_visibility, \
+}
+
+/*
+ * sys_reg_desc initialiser for architecturally unallocated cpufeature ID
+ * register with encoding Op0=3, Op1=0, CRn=0, CRm=crm, Op2=op2
+ * (1 <= crm < 8, 0 <= Op2 < 8).
+ */
+#define ID_UNALLOCATED(crm, op2) { \
+ Op0(3), Op1(0), CRn(0), CRm(crm), Op2(op2), \
+ .access = access_id_reg, \
+ .get_user = get_id_reg, \
+ .set_user = set_id_reg, \
+ .visibility = raz_visibility \
+}
+
+/*
+ * sys_reg_desc initialiser for known ID registers that we hide from guests.
+ * For now, these are exposed just like unallocated ID regs: they appear
+ * RAZ for the guest.
+ */
+#define ID_HIDDEN(name) { \
+ SYS_DESC(SYS_##name), \
+ .access = access_id_reg, \
+ .get_user = get_id_reg, \
+ .set_user = set_id_reg, \
+ .visibility = raz_visibility, \
+}
+
+const struct sys_reg_desc id_reg_descs[KVM_ARM_ID_REG_NUM] = {
+ /*
+ * ID regs: all ID_SANITISED() entries here must have corresponding
+ * entries in arm64_ftr_regs[].
+ */
+
+ /* AArch64 mappings of the AArch32 ID registers */
+ /* CRm=1 */
+ AA32_ID_SANITISED(ID_PFR0_EL1),
+ AA32_ID_SANITISED(ID_PFR1_EL1),
+ { SYS_DESC(SYS_ID_DFR0_EL1), .access = access_id_reg,
+ .get_user = get_id_reg, .set_user = set_id_dfr0_el1,
+ .visibility = aa32_id_visibility, },
+ ID_HIDDEN(ID_AFR0_EL1),
+ AA32_ID_SANITISED(ID_MMFR0_EL1),
+ AA32_ID_SANITISED(ID_MMFR1_EL1),
+ AA32_ID_SANITISED(ID_MMFR2_EL1),
+ AA32_ID_SANITISED(ID_MMFR3_EL1),
+
+ /* CRm=2 */
+ AA32_ID_SANITISED(ID_ISAR0_EL1),
+ AA32_ID_SANITISED(ID_ISAR1_EL1),
+ AA32_ID_SANITISED(ID_ISAR2_EL1),
+ AA32_ID_SANITISED(ID_ISAR3_EL1),
+ AA32_ID_SANITISED(ID_ISAR4_EL1),
+ AA32_ID_SANITISED(ID_ISAR5_EL1),
+ AA32_ID_SANITISED(ID_MMFR4_EL1),
+ AA32_ID_SANITISED(ID_ISAR6_EL1),
+
+ /* CRm=3 */
+ AA32_ID_SANITISED(MVFR0_EL1),
+ AA32_ID_SANITISED(MVFR1_EL1),
+ AA32_ID_SANITISED(MVFR2_EL1),
+ ID_UNALLOCATED(3, 3),
+ AA32_ID_SANITISED(ID_PFR2_EL1),
+ ID_HIDDEN(ID_DFR1_EL1),
+ AA32_ID_SANITISED(ID_MMFR5_EL1),
+ ID_UNALLOCATED(3, 7),
+
+ /* AArch64 ID registers */
+ /* CRm=4 */
+ { SYS_DESC(SYS_ID_AA64PFR0_EL1), .access = access_id_reg,
+ .get_user = get_id_reg, .set_user = set_id_aa64pfr0_el1, },
+ ID_SANITISED(ID_AA64PFR1_EL1),
+ ID_UNALLOCATED(4, 2),
+ ID_UNALLOCATED(4, 3),
+ ID_SANITISED(ID_AA64ZFR0_EL1),
+ ID_HIDDEN(ID_AA64SMFR0_EL1),
+ ID_UNALLOCATED(4, 6),
+ ID_UNALLOCATED(4, 7),
+
+ /* CRm=5 */
+ { SYS_DESC(SYS_ID_AA64DFR0_EL1), .access = access_id_reg,
+ .get_user = get_id_reg, .set_user = set_id_aa64dfr0_el1, },
+ ID_SANITISED(ID_AA64DFR1_EL1),
+ ID_UNALLOCATED(5, 2),
+ ID_UNALLOCATED(5, 3),
+ ID_HIDDEN(ID_AA64AFR0_EL1),
+ ID_HIDDEN(ID_AA64AFR1_EL1),
+ ID_UNALLOCATED(5, 6),
+ ID_UNALLOCATED(5, 7),
+
+ /* CRm=6 */
+ ID_SANITISED(ID_AA64ISAR0_EL1),
+ ID_SANITISED(ID_AA64ISAR1_EL1),
+ ID_SANITISED(ID_AA64ISAR2_EL1),
+ ID_UNALLOCATED(6, 3),
+ ID_UNALLOCATED(6, 4),
+ ID_UNALLOCATED(6, 5),
+ ID_UNALLOCATED(6, 6),
+ ID_UNALLOCATED(6, 7),
+
+ /* CRm=7 */
+ ID_SANITISED(ID_AA64MMFR0_EL1),
+ ID_SANITISED(ID_AA64MMFR1_EL1),
+ ID_SANITISED(ID_AA64MMFR2_EL1),
+ ID_UNALLOCATED(7, 3),
+ ID_UNALLOCATED(7, 4),
+ ID_UNALLOCATED(7, 5),
+ ID_UNALLOCATED(7, 6),
+ ID_UNALLOCATED(7, 7),
+};
+
+/**
+ * emulate_id_reg - Emulate a guest access to an AArch64 CPU ID feature register
+ * @vcpu: The VCPU pointer
+ * @params: Decoded system register parameters
+ *
+ * Return: true if the ID register access was successful, false otherwise.
+ */
+int emulate_id_reg(struct kvm_vcpu *vcpu, struct sys_reg_params *params)
+{
+ const struct sys_reg_desc *r;
+
+ r = find_reg(params, id_reg_descs, ARRAY_SIZE(id_reg_descs));
+
+ if (likely(r)) {
+ perform_access(vcpu, params, r);
+ } else {
+ print_sys_reg_msg(params,
+ "Unsupported guest id_reg access at: %lx [%08lx]\n",
+ *vcpu_pc(vcpu), *vcpu_cpsr(vcpu));
+ kvm_inject_undefined(vcpu);
+ }
+
+ return 1;
+}
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 34688918c811..8dba10696dac 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -53,9 +53,9 @@ static bool read_from_write_only(struct kvm_vcpu *vcpu,
return false;
}
-static bool write_to_read_only(struct kvm_vcpu *vcpu,
- struct sys_reg_params *params,
- const struct sys_reg_desc *r)
+bool write_to_read_only(struct kvm_vcpu *vcpu,
+ struct sys_reg_params *params,
+ const struct sys_reg_desc *r)
{
WARN_ONCE(1, "Unexpected sys_reg write to read-only register\n");
print_sys_reg_instr(params);
@@ -1168,163 +1168,11 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu,
return true;
}
-static u8 vcpu_pmuver(const struct kvm_vcpu *vcpu)
-{
- if (kvm_vcpu_has_pmu(vcpu))
- return vcpu->kvm->arch.dfr0_pmuver.imp;
-
- return vcpu->kvm->arch.dfr0_pmuver.unimp;
-}
-
-static u8 perfmon_to_pmuver(u8 perfmon)
-{
- switch (perfmon) {
- case ID_DFR0_EL1_PerfMon_PMUv3:
- return ID_AA64DFR0_EL1_PMUVer_IMP;
- case ID_DFR0_EL1_PerfMon_IMPDEF:
- return ID_AA64DFR0_EL1_PMUVer_IMP_DEF;
- default:
- /* Anything ARMv8.1+ and NI have the same value. For now. */
- return perfmon;
- }
-}
-
-static u8 pmuver_to_perfmon(u8 pmuver)
-{
- switch (pmuver) {
- case ID_AA64DFR0_EL1_PMUVer_IMP:
- return ID_DFR0_EL1_PerfMon_PMUv3;
- case ID_AA64DFR0_EL1_PMUVer_IMP_DEF:
- return ID_DFR0_EL1_PerfMon_IMPDEF;
- default:
- /* Anything ARMv8.1+ and NI have the same value. For now. */
- return pmuver;
- }
-}
-
-/* Read a sanitised cpufeature ID register by sys_reg_desc */
-static u64 read_id_reg(const struct kvm_vcpu *vcpu, struct sys_reg_desc const *r)
-{
- u32 id = reg_to_encoding(r);
- u64 val;
-
- if (sysreg_visible_as_raz(vcpu, r))
- return 0;
-
- val = read_sanitised_ftr_reg(id);
-
- switch (id) {
- case SYS_ID_AA64PFR0_EL1:
- if (!vcpu_has_sve(vcpu))
- val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_SVE);
- val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_AMU);
- val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV2);
- val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV2), (u64)vcpu->kvm->arch.pfr0_csv2);
- val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV3);
- val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV3), (u64)vcpu->kvm->arch.pfr0_csv3);
- if (kvm_vgic_global_state.type == VGIC_V3) {
- val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_GIC);
- val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_GIC), 1);
- }
- break;
- case SYS_ID_AA64PFR1_EL1:
- if (!kvm_has_mte(vcpu->kvm))
- val &= ~ARM64_FEATURE_MASK(ID_AA64PFR1_EL1_MTE);
-
- val &= ~ARM64_FEATURE_MASK(ID_AA64PFR1_EL1_SME);
- break;
- case SYS_ID_AA64ISAR1_EL1:
- if (!vcpu_has_ptrauth(vcpu))
- val &= ~(ARM64_FEATURE_MASK(ID_AA64ISAR1_EL1_APA) |
- ARM64_FEATURE_MASK(ID_AA64ISAR1_EL1_API) |
- ARM64_FEATURE_MASK(ID_AA64ISAR1_EL1_GPA) |
- ARM64_FEATURE_MASK(ID_AA64ISAR1_EL1_GPI));
- break;
- case SYS_ID_AA64ISAR2_EL1:
- if (!vcpu_has_ptrauth(vcpu))
- val &= ~(ARM64_FEATURE_MASK(ID_AA64ISAR2_EL1_APA3) |
- ARM64_FEATURE_MASK(ID_AA64ISAR2_EL1_GPA3));
- if (!cpus_have_final_cap(ARM64_HAS_WFXT))
- val &= ~ARM64_FEATURE_MASK(ID_AA64ISAR2_EL1_WFxT);
- break;
- case SYS_ID_AA64DFR0_EL1:
- /* Limit debug to ARMv8.0 */
- val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_DebugVer);
- val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_DebugVer), 6);
- /* Set PMUver to the required version */
- val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer);
- val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer),
- vcpu_pmuver(vcpu));
- /* Hide SPE from guests */
- val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMSVer);
- break;
- case SYS_ID_DFR0_EL1:
- val &= ~ARM64_FEATURE_MASK(ID_DFR0_EL1_PerfMon);
- val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_DFR0_EL1_PerfMon),
- pmuver_to_perfmon(vcpu_pmuver(vcpu)));
- break;
- case SYS_ID_AA64MMFR2_EL1:
- val &= ~ID_AA64MMFR2_EL1_CCIDX_MASK;
- break;
- case SYS_ID_MMFR4_EL1:
- val &= ~ARM64_FEATURE_MASK(ID_MMFR4_EL1_CCIDX);
- break;
- }
-
- return val;
-}
-
-static unsigned int id_visibility(const struct kvm_vcpu *vcpu,
- const struct sys_reg_desc *r)
-{
- u32 id = reg_to_encoding(r);
-
- switch (id) {
- case SYS_ID_AA64ZFR0_EL1:
- if (!vcpu_has_sve(vcpu))
- return REG_RAZ;
- break;
- }
-
- return 0;
-}
-
-static unsigned int aa32_id_visibility(const struct kvm_vcpu *vcpu,
- const struct sys_reg_desc *r)
-{
- /*
- * AArch32 ID registers are UNKNOWN if AArch32 isn't implemented at any
- * EL. Promote to RAZ/WI in order to guarantee consistency between
- * systems.
- */
- if (!kvm_supports_32bit_el0())
- return REG_RAZ | REG_USER_WI;
-
- return id_visibility(vcpu, r);
-}
-
-static unsigned int raz_visibility(const struct kvm_vcpu *vcpu,
- const struct sys_reg_desc *r)
+unsigned int raz_visibility(const struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
{
return REG_RAZ;
}
-/* cpufeature ID register access trap handlers */
-
-static bool access_id_reg(struct kvm_vcpu *vcpu,
- struct sys_reg_params *p,
- const struct sys_reg_desc *r)
-{
- if (p->is_write)
- return write_to_read_only(vcpu, p, r);
-
- p->regval = read_id_reg(vcpu, r);
- if (vcpu_has_nv(vcpu))
- access_nested_id_reg(vcpu, p, r);
-
- return true;
-}
-
/* Visibility overrides for SVE-specific control registers */
static unsigned int sve_visibility(const struct kvm_vcpu *vcpu,
const struct sys_reg_desc *rd)
@@ -1335,144 +1183,6 @@ static unsigned int sve_visibility(const struct kvm_vcpu *vcpu,
return REG_HIDDEN;
}
-static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
- const struct sys_reg_desc *rd,
- u64 val)
-{
- u8 csv2, csv3;
-
- /*
- * Allow AA64PFR0_EL1.CSV2 to be set from userspace as long as
- * it doesn't promise more than what is actually provided (the
- * guest could otherwise be covered in ectoplasmic residue).
- */
- csv2 = cpuid_feature_extract_unsigned_field(val, ID_AA64PFR0_EL1_CSV2_SHIFT);
- if (csv2 > 1 ||
- (csv2 && arm64_get_spectre_v2_state() != SPECTRE_UNAFFECTED))
- return -EINVAL;
-
- /* Same thing for CSV3 */
- csv3 = cpuid_feature_extract_unsigned_field(val, ID_AA64PFR0_EL1_CSV3_SHIFT);
- if (csv3 > 1 ||
- (csv3 && arm64_get_meltdown_state() != SPECTRE_UNAFFECTED))
- return -EINVAL;
-
- /* We can only differ with CSV[23], and anything else is an error */
- val ^= read_id_reg(vcpu, rd);
- val &= ~(ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV2) |
- ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV3));
- if (val)
- return -EINVAL;
-
- vcpu->kvm->arch.pfr0_csv2 = csv2;
- vcpu->kvm->arch.pfr0_csv3 = csv3;
-
- return 0;
-}
-
-static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
- const struct sys_reg_desc *rd,
- u64 val)
-{
- u8 pmuver, host_pmuver;
- bool valid_pmu;
-
- host_pmuver = kvm_arm_pmu_get_pmuver_limit();
-
- /*
- * Allow AA64DFR0_EL1.PMUver to be set from userspace as long
- * as it doesn't promise more than what the HW gives us. We
- * allow an IMPDEF PMU though, only if no PMU is supported
- * (KVM backward compatibility handling).
- */
- pmuver = FIELD_GET(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer), val);
- if ((pmuver != ID_AA64DFR0_EL1_PMUVer_IMP_DEF && pmuver > host_pmuver))
- return -EINVAL;
-
- valid_pmu = (pmuver != 0 && pmuver != ID_AA64DFR0_EL1_PMUVer_IMP_DEF);
-
- /* Make sure view register and PMU support do match */
- if (kvm_vcpu_has_pmu(vcpu) != valid_pmu)
- return -EINVAL;
-
- /* We can only differ with PMUver, and anything else is an error */
- val ^= read_id_reg(vcpu, rd);
- val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer);
- if (val)
- return -EINVAL;
-
- if (valid_pmu)
- vcpu->kvm->arch.dfr0_pmuver.imp = pmuver;
- else
- vcpu->kvm->arch.dfr0_pmuver.unimp = pmuver;
-
- return 0;
-}
-
-static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
- const struct sys_reg_desc *rd,
- u64 val)
-{
- u8 perfmon, host_perfmon;
- bool valid_pmu;
-
- host_perfmon = pmuver_to_perfmon(kvm_arm_pmu_get_pmuver_limit());
-
- /*
- * Allow DFR0_EL1.PerfMon to be set from userspace as long as
- * it doesn't promise more than what the HW gives us on the
- * AArch64 side (as everything is emulated with that), and
- * that this is a PMUv3.
- */
- perfmon = FIELD_GET(ARM64_FEATURE_MASK(ID_DFR0_EL1_PerfMon), val);
- if ((perfmon != ID_DFR0_EL1_PerfMon_IMPDEF && perfmon > host_perfmon) ||
- (perfmon != 0 && perfmon < ID_DFR0_EL1_PerfMon_PMUv3))
- return -EINVAL;
-
- valid_pmu = (perfmon != 0 && perfmon != ID_DFR0_EL1_PerfMon_IMPDEF);
-
- /* Make sure view register and PMU support do match */
- if (kvm_vcpu_has_pmu(vcpu) != valid_pmu)
- return -EINVAL;
-
- /* We can only differ with PerfMon, and anything else is an error */
- val ^= read_id_reg(vcpu, rd);
- val &= ~ARM64_FEATURE_MASK(ID_DFR0_EL1_PerfMon);
- if (val)
- return -EINVAL;
-
- if (valid_pmu)
- vcpu->kvm->arch.dfr0_pmuver.imp = perfmon_to_pmuver(perfmon);
- else
- vcpu->kvm->arch.dfr0_pmuver.unimp = perfmon_to_pmuver(perfmon);
-
- return 0;
-}
-
-/*
- * cpufeature ID register user accessors
- *
- * For now, these registers are immutable for userspace, so no values
- * are stored, and for set_id_reg() we don't allow the effective value
- * to be changed.
- */
-static int get_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
- u64 *val)
-{
- *val = read_id_reg(vcpu, rd);
- return 0;
-}
-
-static int set_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
- u64 val)
-{
- /* This is what we mean by invariant: you can't change it. */
- if (val != read_id_reg(vcpu, rd))
- return -EINVAL;
-
- return 0;
-}
-
static int get_raz_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
u64 *val)
{
@@ -1657,50 +1367,6 @@ static unsigned int elx2_visibility(const struct kvm_vcpu *vcpu,
.visibility = elx2_visibility, \
}
-/* sys_reg_desc initialiser for known cpufeature ID registers */
-#define ID_SANITISED(name) { \
- SYS_DESC(SYS_##name), \
- .access = access_id_reg, \
- .get_user = get_id_reg, \
- .set_user = set_id_reg, \
- .visibility = id_visibility, \
-}
-
-/* sys_reg_desc initialiser for known cpufeature ID registers */
-#define AA32_ID_SANITISED(name) { \
- SYS_DESC(SYS_##name), \
- .access = access_id_reg, \
- .get_user = get_id_reg, \
- .set_user = set_id_reg, \
- .visibility = aa32_id_visibility, \
-}
-
-/*
- * sys_reg_desc initialiser for architecturally unallocated cpufeature ID
- * register with encoding Op0=3, Op1=0, CRn=0, CRm=crm, Op2=op2
- * (1 <= crm < 8, 0 <= Op2 < 8).
- */
-#define ID_UNALLOCATED(crm, op2) { \
- Op0(3), Op1(0), CRn(0), CRm(crm), Op2(op2), \
- .access = access_id_reg, \
- .get_user = get_id_reg, \
- .set_user = set_id_reg, \
- .visibility = raz_visibility \
-}
-
-/*
- * sys_reg_desc initialiser for known ID registers that we hide from guests.
- * For now, these are exposed just like unallocated ID regs: they appear
- * RAZ for the guest.
- */
-#define ID_HIDDEN(name) { \
- SYS_DESC(SYS_##name), \
- .access = access_id_reg, \
- .get_user = get_id_reg, \
- .set_user = set_id_reg, \
- .visibility = raz_visibility, \
-}
-
static bool access_sp_el1(struct kvm_vcpu *vcpu,
struct sys_reg_params *p,
const struct sys_reg_desc *r)
@@ -1737,6 +1403,8 @@ static bool access_spsr(struct kvm_vcpu *vcpu,
return true;
}
+extern const struct sys_reg_desc id_reg_descs[KVM_ARM_ID_REG_NUM];
+
/*
* Architected system registers.
* Important: Must be sorted ascending by Op0, Op1, CRn, CRm, Op2
@@ -1791,87 +1459,6 @@ static const struct sys_reg_desc sys_reg_descs[] = {
{ SYS_DESC(SYS_MPIDR_EL1), NULL, reset_mpidr, MPIDR_EL1 },
- /*
- * ID regs: all ID_SANITISED() entries here must have corresponding
- * entries in arm64_ftr_regs[].
- */
-
- /* AArch64 mappings of the AArch32 ID registers */
- /* CRm=1 */
- AA32_ID_SANITISED(ID_PFR0_EL1),
- AA32_ID_SANITISED(ID_PFR1_EL1),
- { SYS_DESC(SYS_ID_DFR0_EL1), .access = access_id_reg,
- .get_user = get_id_reg, .set_user = set_id_dfr0_el1,
- .visibility = aa32_id_visibility, },
- ID_HIDDEN(ID_AFR0_EL1),
- AA32_ID_SANITISED(ID_MMFR0_EL1),
- AA32_ID_SANITISED(ID_MMFR1_EL1),
- AA32_ID_SANITISED(ID_MMFR2_EL1),
- AA32_ID_SANITISED(ID_MMFR3_EL1),
-
- /* CRm=2 */
- AA32_ID_SANITISED(ID_ISAR0_EL1),
- AA32_ID_SANITISED(ID_ISAR1_EL1),
- AA32_ID_SANITISED(ID_ISAR2_EL1),
- AA32_ID_SANITISED(ID_ISAR3_EL1),
- AA32_ID_SANITISED(ID_ISAR4_EL1),
- AA32_ID_SANITISED(ID_ISAR5_EL1),
- AA32_ID_SANITISED(ID_MMFR4_EL1),
- AA32_ID_SANITISED(ID_ISAR6_EL1),
-
- /* CRm=3 */
- AA32_ID_SANITISED(MVFR0_EL1),
- AA32_ID_SANITISED(MVFR1_EL1),
- AA32_ID_SANITISED(MVFR2_EL1),
- ID_UNALLOCATED(3,3),
- AA32_ID_SANITISED(ID_PFR2_EL1),
- ID_HIDDEN(ID_DFR1_EL1),
- AA32_ID_SANITISED(ID_MMFR5_EL1),
- ID_UNALLOCATED(3,7),
-
- /* AArch64 ID registers */
- /* CRm=4 */
- { SYS_DESC(SYS_ID_AA64PFR0_EL1), .access = access_id_reg,
- .get_user = get_id_reg, .set_user = set_id_aa64pfr0_el1, },
- ID_SANITISED(ID_AA64PFR1_EL1),
- ID_UNALLOCATED(4,2),
- ID_UNALLOCATED(4,3),
- ID_SANITISED(ID_AA64ZFR0_EL1),
- ID_HIDDEN(ID_AA64SMFR0_EL1),
- ID_UNALLOCATED(4,6),
- ID_UNALLOCATED(4,7),
-
- /* CRm=5 */
- { SYS_DESC(SYS_ID_AA64DFR0_EL1), .access = access_id_reg,
- .get_user = get_id_reg, .set_user = set_id_aa64dfr0_el1, },
- ID_SANITISED(ID_AA64DFR1_EL1),
- ID_UNALLOCATED(5,2),
- ID_UNALLOCATED(5,3),
- ID_HIDDEN(ID_AA64AFR0_EL1),
- ID_HIDDEN(ID_AA64AFR1_EL1),
- ID_UNALLOCATED(5,6),
- ID_UNALLOCATED(5,7),
-
- /* CRm=6 */
- ID_SANITISED(ID_AA64ISAR0_EL1),
- ID_SANITISED(ID_AA64ISAR1_EL1),
- ID_SANITISED(ID_AA64ISAR2_EL1),
- ID_UNALLOCATED(6,3),
- ID_UNALLOCATED(6,4),
- ID_UNALLOCATED(6,5),
- ID_UNALLOCATED(6,6),
- ID_UNALLOCATED(6,7),
-
- /* CRm=7 */
- ID_SANITISED(ID_AA64MMFR0_EL1),
- ID_SANITISED(ID_AA64MMFR1_EL1),
- ID_SANITISED(ID_AA64MMFR2_EL1),
- ID_UNALLOCATED(7,3),
- ID_UNALLOCATED(7,4),
- ID_UNALLOCATED(7,5),
- ID_UNALLOCATED(7,6),
- ID_UNALLOCATED(7,7),
-
{ SYS_DESC(SYS_SCTLR_EL1), access_vm_reg, reset_val, SCTLR_EL1, 0x00C50078 },
{ SYS_DESC(SYS_ACTLR_EL1), access_actlr, reset_actlr, ACTLR_EL1 },
{ SYS_DESC(SYS_CPACR_EL1), NULL, reset_val, CPACR_EL1, 0 },
@@ -2573,7 +2160,7 @@ int kvm_handle_cp14_load_store(struct kvm_vcpu *vcpu)
return 1;
}
-static void perform_access(struct kvm_vcpu *vcpu,
+void perform_access(struct kvm_vcpu *vcpu,
struct sys_reg_params *params,
const struct sys_reg_desc *r)
{
@@ -2948,6 +2535,9 @@ int kvm_handle_sys_reg(struct kvm_vcpu *vcpu)
params = esr_sys64_to_params(esr);
params.regval = vcpu_get_reg(vcpu, Rt);
+ if (is_id_reg(reg_to_encoding(¶ms)))
+ return emulate_id_reg(vcpu, ¶ms);
+
if (!emulate_sys_reg(vcpu, ¶ms))
return 1;
@@ -3167,6 +2757,7 @@ int kvm_sys_reg_get_user(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg,
int kvm_arm_sys_reg_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
{
void __user *uaddr = (void __user *)(unsigned long)reg->addr;
+ struct sys_reg_params params;
int err;
if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_DEMUX)
@@ -3176,6 +2767,13 @@ int kvm_arm_sys_reg_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg
if (err != -ENOENT)
return err;
+ if (!index_to_params(reg->id, ¶ms))
+ return -ENOENT;
+
+ if (is_id_reg(reg_to_encoding(¶ms)))
+ return kvm_sys_reg_get_user(vcpu, reg,
+ id_reg_descs, ARRAY_SIZE(id_reg_descs));
+
return kvm_sys_reg_get_user(vcpu, reg,
sys_reg_descs, ARRAY_SIZE(sys_reg_descs));
}
@@ -3211,6 +2809,7 @@ int kvm_sys_reg_set_user(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg,
int kvm_arm_sys_reg_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
{
void __user *uaddr = (void __user *)(unsigned long)reg->addr;
+ struct sys_reg_params params;
int err;
if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_DEMUX)
@@ -3220,6 +2819,13 @@ int kvm_arm_sys_reg_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg
if (err != -ENOENT)
return err;
+ if (!index_to_params(reg->id, ¶ms))
+ return -ENOENT;
+
+ if (is_id_reg(reg_to_encoding(¶ms)))
+ return kvm_sys_reg_set_user(vcpu, reg,
+ id_reg_descs, ARRAY_SIZE(id_reg_descs));
+
return kvm_sys_reg_set_user(vcpu, reg,
sys_reg_descs, ARRAY_SIZE(sys_reg_descs));
}
@@ -3289,14 +2895,15 @@ static int walk_one_sys_reg(const struct kvm_vcpu *vcpu,
}
/* Assumed ordered tables, see kvm_sys_reg_table_init. */
-static int walk_sys_regs(struct kvm_vcpu *vcpu, u64 __user *uind)
+static int walk_sys_regs(struct kvm_vcpu *vcpu, u64 __user *uind,
+ const struct sys_reg_desc table[], unsigned int num)
{
const struct sys_reg_desc *i2, *end2;
unsigned int total = 0;
int err;
- i2 = sys_reg_descs;
- end2 = sys_reg_descs + ARRAY_SIZE(sys_reg_descs);
+ i2 = table;
+ end2 = table + num;
while (i2 != end2) {
err = walk_one_sys_reg(vcpu, i2++, &uind, &total);
@@ -3310,7 +2917,8 @@ unsigned long kvm_arm_num_sys_reg_descs(struct kvm_vcpu *vcpu)
{
return ARRAY_SIZE(invariant_sys_regs)
+ num_demux_regs()
- + walk_sys_regs(vcpu, (u64 __user *)NULL);
+ + walk_sys_regs(vcpu, (u64 __user *)NULL, id_reg_descs, ARRAY_SIZE(id_reg_descs))
+ + walk_sys_regs(vcpu, (u64 __user *)NULL, sys_reg_descs, ARRAY_SIZE(sys_reg_descs));
}
int kvm_arm_copy_sys_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
@@ -3325,7 +2933,12 @@ int kvm_arm_copy_sys_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
uindices++;
}
- err = walk_sys_regs(vcpu, uindices);
+ err = walk_sys_regs(vcpu, uindices, id_reg_descs, ARRAY_SIZE(id_reg_descs));
+ if (err < 0)
+ return err;
+ uindices += err;
+
+ err = walk_sys_regs(vcpu, uindices, sys_reg_descs, ARRAY_SIZE(sys_reg_descs));
if (err < 0)
return err;
uindices += err;
@@ -3339,6 +2952,7 @@ int __init kvm_sys_reg_table_init(void)
unsigned int i;
/* Make sure tables are unique and in order. */
+ valid &= check_sysreg_table(id_reg_descs, ARRAY_SIZE(id_reg_descs), false);
valid &= check_sysreg_table(sys_reg_descs, ARRAY_SIZE(sys_reg_descs), false);
valid &= check_sysreg_table(cp14_regs, ARRAY_SIZE(cp14_regs), true);
valid &= check_sysreg_table(cp14_64_regs, ARRAY_SIZE(cp14_64_regs), true);
diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
index 6b11f2cc7146..7ce546a8be60 100644
--- a/arch/arm64/kvm/sys_regs.h
+++ b/arch/arm64/kvm/sys_regs.h
@@ -210,6 +210,19 @@ find_reg(const struct sys_reg_params *params, const struct sys_reg_desc table[],
return __inline_bsearch((void *)pval, table, num, sizeof(table[0]), match_sys_reg);
}
+/*
+ * Return true if the register's (Op0, Op1, CRn, CRm, Op2) is
+ * (3, 0, 0, crm, op2), where 1<=crm<8, 0<=op2<8.
+ */
+static inline bool is_id_reg(u32 id)
+{
+ return (sys_reg_Op0(id) == 3 && sys_reg_Op1(id) == 0 &&
+ sys_reg_CRn(id) == 0 && sys_reg_CRm(id) >= 1 &&
+ sys_reg_CRm(id) < 8);
+}
+
+void perform_access(struct kvm_vcpu *vcpu, struct sys_reg_params *params,
+ const struct sys_reg_desc *r);
const struct sys_reg_desc *get_reg_by_id(u64 id,
const struct sys_reg_desc table[],
unsigned int num);
@@ -220,6 +233,10 @@ int kvm_sys_reg_get_user(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg,
const struct sys_reg_desc table[], unsigned int num);
int kvm_sys_reg_set_user(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg,
const struct sys_reg_desc table[], unsigned int num);
+bool write_to_read_only(struct kvm_vcpu *vcpu,
+ struct sys_reg_params *params, const struct sys_reg_desc *r);
+unsigned int raz_visibility(const struct kvm_vcpu *vcpu, const struct sys_reg_desc *r);
+int emulate_id_reg(struct kvm_vcpu *vcpu, struct sys_reg_params *params);
#define AA32(_x) .aarch32_map = AA32_##_x
#define Op0(_x) .Op0 = _x
@@ -234,4 +251,6 @@ int kvm_sys_reg_set_user(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg,
CRn(sys_reg_CRn(reg)), CRm(sys_reg_CRm(reg)), \
Op2(sys_reg_Op2(reg))
+#define KVM_ARM_ID_REG_NUM 56
+
#endif /* __ARM64_KVM_SYS_REGS_LOCAL_H__ */
--
2.40.1.495.gc816e09b53d-goog
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v8 2/6] KVM: arm64: Save ID registers' sanitized value per guest
2023-05-03 17:16 [PATCH v8 0/6] Support writable CPU ID registers from userspace Jing Zhang
2023-05-03 17:16 ` [PATCH v8 1/6] KVM: arm64: Move CPU ID feature registers emulation into a separate file Jing Zhang
@ 2023-05-03 17:16 ` Jing Zhang
2023-05-17 7:41 ` Marc Zyngier
2023-05-03 17:16 ` [PATCH v8 3/6] KVM: arm64: Use per guest ID register for ID_AA64PFR0_EL1.[CSV2|CSV3] Jing Zhang
` (4 subsequent siblings)
6 siblings, 1 reply; 34+ messages in thread
From: Jing Zhang @ 2023-05-03 17:16 UTC (permalink / raw)
To: KVM, KVMARM, ARMLinux, Marc Zyngier, Oliver Upton
Cc: Will Deacon, Paolo Bonzini, James Morse, Alexandru Elisei,
Suzuki K Poulose, Fuad Tabba, Reiji Watanabe,
Raghavendra Rao Ananta, Jing Zhang
Introduce id_regs[] in kvm_arch as a storage of guest's ID registers,
and save ID registers' sanitized value in the array at KVM_CREATE_VM.
Use the saved ones when ID registers are read by the guest or
userspace (via KVM_GET_ONE_REG).
No functional change intended.
Co-developed-by: Reiji Watanabe <reijiw@google.com>
Signed-off-by: Reiji Watanabe <reijiw@google.com>
Signed-off-by: Jing Zhang <jingzhangos@google.com>
---
arch/arm64/include/asm/kvm_host.h | 20 ++++++++++++++
arch/arm64/kvm/arm.c | 1 +
arch/arm64/kvm/id_regs.c | 46 +++++++++++++++++++++++++------
arch/arm64/kvm/sys_regs.c | 11 +++++++-
arch/arm64/kvm/sys_regs.h | 3 +-
5 files changed, 69 insertions(+), 12 deletions(-)
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index bcd774d74f34..a7d4d9e093e3 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -177,6 +177,21 @@ struct kvm_smccc_features {
unsigned long vendor_hyp_bmap;
};
+/*
+ * Emulated CPU ID registers per VM
+ * (Op0, Op1, CRn, CRm, Op2) of the ID registers to be saved in it
+ * is (3, 0, 0, crm, op2), where 1<=crm<8, 0<=op2<8.
+ *
+ * These emulated idregs are VM-wide, but accessed from the context of a vCPU.
+ * Access to id regs are guarded by kvm_arch.config_lock.
+ */
+#define KVM_ARM_ID_REG_NUM 56
+#define IDREG_IDX(id) (((sys_reg_CRm(id) - 1) << 3) | sys_reg_Op2(id))
+#define IDREG(kvm, id) kvm->arch.idregs.regs[IDREG_IDX(id)]
+struct kvm_idregs {
+ u64 regs[KVM_ARM_ID_REG_NUM];
+};
+
typedef unsigned int pkvm_handle_t;
struct kvm_protected_vm {
@@ -243,6 +258,9 @@ struct kvm_arch {
/* Hypercall features firmware registers' descriptor */
struct kvm_smccc_features smccc_feat;
+ /* Emulated CPU ID registers */
+ struct kvm_idregs idregs;
+
/*
* For an untrusted host VM, 'pkvm.handle' is used to lookup
* the associated pKVM instance in the hypervisor.
@@ -1008,6 +1026,8 @@ int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu,
long kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm,
struct kvm_arm_copy_mte_tags *copy_tags);
+void kvm_arm_init_id_regs(struct kvm *kvm);
+
/* Guest/host FPSIMD coordination helpers */
int kvm_arch_vcpu_run_map_fp(struct kvm_vcpu *vcpu);
void kvm_arch_vcpu_load_fp(struct kvm_vcpu *vcpu);
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 4b2e16e696a8..e34744c36406 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -153,6 +153,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
set_default_spectre(kvm);
kvm_arm_init_hypercalls(kvm);
+ kvm_arm_init_id_regs(kvm);
/*
* Initialise the default PMUver before there is a chance to
diff --git a/arch/arm64/kvm/id_regs.c b/arch/arm64/kvm/id_regs.c
index 96b4c43a5100..e769223bcee2 100644
--- a/arch/arm64/kvm/id_regs.c
+++ b/arch/arm64/kvm/id_regs.c
@@ -52,16 +52,9 @@ static u8 pmuver_to_perfmon(u8 pmuver)
}
}
-/* Read a sanitised cpufeature ID register by sys_reg_desc */
-static u64 read_id_reg(const struct kvm_vcpu *vcpu, struct sys_reg_desc const *r)
+u64 kvm_arm_read_id_reg(const struct kvm_vcpu *vcpu, u32 id)
{
- u32 id = reg_to_encoding(r);
- u64 val;
-
- if (sysreg_visible_as_raz(vcpu, r))
- return 0;
-
- val = read_sanitised_ftr_reg(id);
+ u64 val = IDREG(vcpu->kvm, id);
switch (id) {
case SYS_ID_AA64PFR0_EL1:
@@ -126,6 +119,14 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu, struct sys_reg_desc const *r
return val;
}
+static u64 read_id_reg(const struct kvm_vcpu *vcpu, struct sys_reg_desc const *r)
+{
+ if (sysreg_visible_as_raz(vcpu, r))
+ return 0;
+
+ return kvm_arm_read_id_reg(vcpu, reg_to_encoding(r));
+}
+
/* cpufeature ID register access trap handlers */
static bool access_id_reg(struct kvm_vcpu *vcpu,
@@ -458,3 +459,30 @@ int emulate_id_reg(struct kvm_vcpu *vcpu, struct sys_reg_params *params)
return 1;
}
+
+/*
+ * Set the guest's ID registers that are defined in id_reg_descs[]
+ * with ID_SANITISED() to the host's sanitized value.
+ */
+void kvm_arm_init_id_regs(struct kvm *kvm)
+{
+ u64 val;
+ u32 id;
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(id_reg_descs); i++) {
+ id = reg_to_encoding(&id_reg_descs[i]);
+
+ /*
+ * Some hidden ID registers which are not in arm64_ftr_regs[]
+ * would cause warnings from read_sanitised_ftr_reg().
+ * Skip those ID registers to avoid the warnings.
+ */
+ if (id_reg_descs[i].visibility == raz_visibility)
+ /* Hidden or reserved ID register */
+ continue;
+
+ val = read_sanitised_ftr_reg(id);
+ IDREG(kvm, id) = val;
+ }
+}
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 8dba10696dac..2e7b17d14b4f 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -364,7 +364,7 @@ static bool trap_loregion(struct kvm_vcpu *vcpu,
struct sys_reg_params *p,
const struct sys_reg_desc *r)
{
- u64 val = read_sanitised_ftr_reg(SYS_ID_AA64MMFR1_EL1);
+ u64 val = kvm_arm_read_id_reg(vcpu, SYS_ID_AA64MMFR1_EL1);
u32 sr = reg_to_encoding(r);
if (!(val & (0xfUL << ID_AA64MMFR1_EL1_LO_SHIFT))) {
@@ -2967,5 +2967,14 @@ int __init kvm_sys_reg_table_init(void)
for (i = 0; i < ARRAY_SIZE(invariant_sys_regs); i++)
invariant_sys_regs[i].reset(NULL, &invariant_sys_regs[i]);
+ /* Sanity check for ID registers */
+ for (i = 0; i < ARRAY_SIZE(id_reg_descs); i++) {
+ if (!is_id_reg(reg_to_encoding(&id_reg_descs[i]))) {
+ kvm_err("Non-ID register(index:%d, name: %s) found in idregs table!\n",
+ i, id_reg_descs[i].name);
+ return -EINVAL;
+ }
+ }
+
return 0;
}
diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
index 7ce546a8be60..e88fd77309b2 100644
--- a/arch/arm64/kvm/sys_regs.h
+++ b/arch/arm64/kvm/sys_regs.h
@@ -237,6 +237,7 @@ bool write_to_read_only(struct kvm_vcpu *vcpu,
struct sys_reg_params *params, const struct sys_reg_desc *r);
unsigned int raz_visibility(const struct kvm_vcpu *vcpu, const struct sys_reg_desc *r);
int emulate_id_reg(struct kvm_vcpu *vcpu, struct sys_reg_params *params);
+u64 kvm_arm_read_id_reg(const struct kvm_vcpu *vcpu, u32 id);
#define AA32(_x) .aarch32_map = AA32_##_x
#define Op0(_x) .Op0 = _x
@@ -251,6 +252,4 @@ int emulate_id_reg(struct kvm_vcpu *vcpu, struct sys_reg_params *params);
CRn(sys_reg_CRn(reg)), CRm(sys_reg_CRm(reg)), \
Op2(sys_reg_Op2(reg))
-#define KVM_ARM_ID_REG_NUM 56
-
#endif /* __ARM64_KVM_SYS_REGS_LOCAL_H__ */
--
2.40.1.495.gc816e09b53d-goog
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v8 3/6] KVM: arm64: Use per guest ID register for ID_AA64PFR0_EL1.[CSV2|CSV3]
2023-05-03 17:16 [PATCH v8 0/6] Support writable CPU ID registers from userspace Jing Zhang
2023-05-03 17:16 ` [PATCH v8 1/6] KVM: arm64: Move CPU ID feature registers emulation into a separate file Jing Zhang
2023-05-03 17:16 ` [PATCH v8 2/6] KVM: arm64: Save ID registers' sanitized value per guest Jing Zhang
@ 2023-05-03 17:16 ` Jing Zhang
2023-05-03 23:43 ` kernel test robot
2023-05-03 17:16 ` [PATCH v8 4/6] KVM: arm64: Use per guest ID register for ID_AA64DFR0_EL1.PMUVer Jing Zhang
` (3 subsequent siblings)
6 siblings, 1 reply; 34+ messages in thread
From: Jing Zhang @ 2023-05-03 17:16 UTC (permalink / raw)
To: KVM, KVMARM, ARMLinux, Marc Zyngier, Oliver Upton
Cc: Will Deacon, Paolo Bonzini, James Morse, Alexandru Elisei,
Suzuki K Poulose, Fuad Tabba, Reiji Watanabe,
Raghavendra Rao Ananta, Jing Zhang
With per guest ID registers, ID_AA64PFR0_EL1.[CSV2|CSV3] settings from
userspace can be stored in its corresponding ID register.
The setting of CSV bits for protected VMs are removed according to the
discussion from Fuad below:
https://lore.kernel.org/all/CA+EHjTwXA9TprX4jeG+-D+c8v9XG+oFdU1o6TSkvVye145_OvA@mail.gmail.com
Besides the removal of CSV bits setting for protected VMs, No other
functional change intended.
Signed-off-by: Jing Zhang <jingzhangos@google.com>
---
arch/arm64/include/asm/kvm_host.h | 2 --
arch/arm64/kvm/arm.c | 17 ----------
arch/arm64/kvm/id_regs.c | 56 ++++++++++++++++++++++++-------
3 files changed, 44 insertions(+), 31 deletions(-)
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index a7d4d9e093e3..4699f6b829b2 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -248,8 +248,6 @@ struct kvm_arch {
cpumask_var_t supported_cpus;
- u8 pfr0_csv2;
- u8 pfr0_csv3;
struct {
u8 imp:4;
u8 unimp:4;
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index e34744c36406..0f71b10a2f05 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -104,22 +104,6 @@ static int kvm_arm_default_max_vcpus(void)
return vgic_present ? kvm_vgic_get_max_vcpus() : KVM_MAX_VCPUS;
}
-static void set_default_spectre(struct kvm *kvm)
-{
- /*
- * The default is to expose CSV2 == 1 if the HW isn't affected.
- * Although this is a per-CPU feature, we make it global because
- * asymmetric systems are just a nuisance.
- *
- * Userspace can override this as long as it doesn't promise
- * the impossible.
- */
- if (arm64_get_spectre_v2_state() == SPECTRE_UNAFFECTED)
- kvm->arch.pfr0_csv2 = 1;
- if (arm64_get_meltdown_state() == SPECTRE_UNAFFECTED)
- kvm->arch.pfr0_csv3 = 1;
-}
-
/**
* kvm_arch_init_vm - initializes a VM data structure
* @kvm: pointer to the KVM struct
@@ -151,7 +135,6 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
/* The maximum number of VCPUs is limited by the host's GIC model */
kvm->max_vcpus = kvm_arm_default_max_vcpus();
- set_default_spectre(kvm);
kvm_arm_init_hypercalls(kvm);
kvm_arm_init_id_regs(kvm);
diff --git a/arch/arm64/kvm/id_regs.c b/arch/arm64/kvm/id_regs.c
index e769223bcee2..5e0fd4c8b375 100644
--- a/arch/arm64/kvm/id_regs.c
+++ b/arch/arm64/kvm/id_regs.c
@@ -61,12 +61,6 @@ u64 kvm_arm_read_id_reg(const struct kvm_vcpu *vcpu, u32 id)
if (!vcpu_has_sve(vcpu))
val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_SVE);
val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_AMU);
- val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV2);
- val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV2),
- (u64)vcpu->kvm->arch.pfr0_csv2);
- val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV3);
- val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV3),
- (u64)vcpu->kvm->arch.pfr0_csv3);
if (kvm_vgic_global_state.type == VGIC_V3) {
val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_GIC);
val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_GIC), 1);
@@ -153,7 +147,12 @@ static bool access_id_reg(struct kvm_vcpu *vcpu,
static int get_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
u64 *val)
{
+ struct kvm_arch *arch = &vcpu->kvm->arch;
+
+ mutex_lock(&arch->config_lock);
*val = read_id_reg(vcpu, rd);
+ mutex_unlock(&arch->config_lock);
+
return 0;
}
@@ -200,7 +199,10 @@ static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
const struct sys_reg_desc *rd,
u64 val)
{
+ struct kvm_arch *arch = &vcpu->kvm->arch;
+ u64 sval = val;
u8 csv2, csv3;
+ int ret = 0;
/*
* Allow AA64PFR0_EL1.CSV2 to be set from userspace as long as
@@ -216,17 +218,26 @@ static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
if (csv3 > 1 || (csv3 && arm64_get_meltdown_state() != SPECTRE_UNAFFECTED))
return -EINVAL;
+ mutex_lock(&arch->config_lock);
/* We can only differ with CSV[23], and anything else is an error */
val ^= read_id_reg(vcpu, rd);
val &= ~(ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV2) |
ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV3));
- if (val)
- return -EINVAL;
-
- vcpu->kvm->arch.pfr0_csv2 = csv2;
- vcpu->kvm->arch.pfr0_csv3 = csv3;
+ if (val) {
+ ret = -EINVAL;
+ goto out;
+ }
- return 0;
+ /* Only allow userspace to change the idregs before VM running */
+ if (test_bit(KVM_ARCH_FLAG_HAS_RAN_ONCE, &vcpu->kvm->arch.flags)) {
+ if (sval != read_id_reg(vcpu, rd))
+ ret = -EBUSY;
+ } else {
+ IDREG(vcpu->kvm, reg_to_encoding(rd)) = sval;
+ }
+out:
+ mutex_unlock(&arch->config_lock);
+ return ret;
}
static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
@@ -485,4 +496,25 @@ void kvm_arm_init_id_regs(struct kvm *kvm)
val = read_sanitised_ftr_reg(id);
IDREG(kvm, id) = val;
}
+
+ /*
+ * The default is to expose CSV2 == 1 if the HW isn't affected.
+ * Although this is a per-CPU feature, we make it global because
+ * asymmetric systems are just a nuisance.
+ *
+ * Userspace can override this as long as it doesn't promise
+ * the impossible.
+ */
+ val = IDREG(kvm, SYS_ID_AA64PFR0_EL1);
+
+ if (arm64_get_spectre_v2_state() == SPECTRE_UNAFFECTED) {
+ val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV2);
+ val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV2), 1);
+ }
+ if (arm64_get_meltdown_state() == SPECTRE_UNAFFECTED) {
+ val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV3);
+ val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV3), 1);
+ }
+
+ IDREG(kvm, SYS_ID_AA64PFR0_EL1) = val;
}
--
2.40.1.495.gc816e09b53d-goog
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v8 4/6] KVM: arm64: Use per guest ID register for ID_AA64DFR0_EL1.PMUVer
2023-05-03 17:16 [PATCH v8 0/6] Support writable CPU ID registers from userspace Jing Zhang
` (2 preceding siblings ...)
2023-05-03 17:16 ` [PATCH v8 3/6] KVM: arm64: Use per guest ID register for ID_AA64PFR0_EL1.[CSV2|CSV3] Jing Zhang
@ 2023-05-03 17:16 ` Jing Zhang
2023-05-03 17:16 ` [PATCH v8 5/6] KVM: arm64: Reuse fields of sys_reg_desc for idreg Jing Zhang
` (2 subsequent siblings)
6 siblings, 0 replies; 34+ messages in thread
From: Jing Zhang @ 2023-05-03 17:16 UTC (permalink / raw)
To: KVM, KVMARM, ARMLinux, Marc Zyngier, Oliver Upton
Cc: Will Deacon, Paolo Bonzini, James Morse, Alexandru Elisei,
Suzuki K Poulose, Fuad Tabba, Reiji Watanabe,
Raghavendra Rao Ananta, Jing Zhang
With per guest ID registers, PMUver settings from userspace
can be stored in its corresponding ID register.
No functional change intended.
Signed-off-by: Jing Zhang <jingzhangos@google.com>
---
arch/arm64/include/asm/kvm_host.h | 11 ++--
arch/arm64/kvm/arm.c | 6 --
arch/arm64/kvm/id_regs.c | 94 +++++++++++++++++++++++++------
include/kvm/arm_pmu.h | 5 +-
4 files changed, 87 insertions(+), 29 deletions(-)
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 4699f6b829b2..009f6ff41078 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -236,6 +236,12 @@ struct kvm_arch {
#define KVM_ARCH_FLAG_EL1_32BIT 4
/* PSCI SYSTEM_SUSPEND enabled for the guest */
#define KVM_ARCH_FLAG_SYSTEM_SUSPEND_ENABLED 5
+ /*
+ * AA64DFR0_EL1.PMUver was set as ID_AA64DFR0_EL1_PMUVer_IMP_DEF
+ * or DFR0_EL1.PerfMon was set as ID_DFR0_EL1_PerfMon_IMPDEF from
+ * userspace for VCPUs without PMU.
+ */
+#define KVM_ARCH_FLAG_VCPU_HAS_IMP_DEF_PMU 6
unsigned long flags;
@@ -248,11 +254,6 @@ struct kvm_arch {
cpumask_var_t supported_cpus;
- struct {
- u8 imp:4;
- u8 unimp:4;
- } dfr0_pmuver;
-
/* Hypercall features firmware registers' descriptor */
struct kvm_smccc_features smccc_feat;
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 0f71b10a2f05..9ecd0c5d0754 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -138,12 +138,6 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
kvm_arm_init_hypercalls(kvm);
kvm_arm_init_id_regs(kvm);
- /*
- * Initialise the default PMUver before there is a chance to
- * create an actual PMU.
- */
- kvm->arch.dfr0_pmuver.imp = kvm_arm_pmu_get_pmuver_limit();
-
return 0;
err_free_cpumask:
diff --git a/arch/arm64/kvm/id_regs.c b/arch/arm64/kvm/id_regs.c
index 5e0fd4c8b375..0a04a90a8676 100644
--- a/arch/arm64/kvm/id_regs.c
+++ b/arch/arm64/kvm/id_regs.c
@@ -21,9 +21,12 @@
static u8 vcpu_pmuver(const struct kvm_vcpu *vcpu)
{
if (kvm_vcpu_has_pmu(vcpu))
- return vcpu->kvm->arch.dfr0_pmuver.imp;
+ return FIELD_GET(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer),
+ IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1));
+ else if (test_bit(KVM_ARCH_FLAG_VCPU_HAS_IMP_DEF_PMU, &vcpu->kvm->arch.flags))
+ return ID_AA64DFR0_EL1_PMUVer_IMP_DEF;
- return vcpu->kvm->arch.dfr0_pmuver.unimp;
+ return 0;
}
static u8 perfmon_to_pmuver(u8 perfmon)
@@ -244,8 +247,11 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
const struct sys_reg_desc *rd,
u64 val)
{
+ struct kvm_arch *arch = &vcpu->kvm->arch;
u8 pmuver, host_pmuver;
bool valid_pmu;
+ u64 sval = val;
+ int ret = 0;
host_pmuver = kvm_arm_pmu_get_pmuver_limit();
@@ -265,26 +271,50 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
if (kvm_vcpu_has_pmu(vcpu) != valid_pmu)
return -EINVAL;
+ mutex_lock(&arch->config_lock);
/* We can only differ with PMUver, and anything else is an error */
val ^= read_id_reg(vcpu, rd);
val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer);
- if (val)
- return -EINVAL;
+ if (val) {
+ ret = -EINVAL;
+ goto out;
+ }
- if (valid_pmu)
- vcpu->kvm->arch.dfr0_pmuver.imp = pmuver;
- else
- vcpu->kvm->arch.dfr0_pmuver.unimp = pmuver;
+ /* Only allow userspace to change the idregs before VM running */
+ if (test_bit(KVM_ARCH_FLAG_HAS_RAN_ONCE, &vcpu->kvm->arch.flags)) {
+ if (sval != read_id_reg(vcpu, rd))
+ ret = -EBUSY;
+ } else {
+ if (valid_pmu) {
+ val = IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1);
+ val &= ~ID_AA64DFR0_EL1_PMUVer_MASK;
+ val |= FIELD_PREP(ID_AA64DFR0_EL1_PMUVer_MASK, pmuver);
+ IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1) = val;
+
+ val = IDREG(vcpu->kvm, SYS_ID_DFR0_EL1);
+ val &= ~ID_DFR0_EL1_PerfMon_MASK;
+ val |= FIELD_PREP(ID_DFR0_EL1_PerfMon_MASK, pmuver_to_perfmon(pmuver));
+ IDREG(vcpu->kvm, SYS_ID_DFR0_EL1) = val;
+ } else {
+ assign_bit(KVM_ARCH_FLAG_VCPU_HAS_IMP_DEF_PMU, &vcpu->kvm->arch.flags,
+ pmuver == ID_AA64DFR0_EL1_PMUVer_IMP_DEF);
+ }
+ }
- return 0;
+out:
+ mutex_unlock(&arch->config_lock);
+ return ret;
}
static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
const struct sys_reg_desc *rd,
u64 val)
{
+ struct kvm_arch *arch = &vcpu->kvm->arch;
u8 perfmon, host_perfmon;
bool valid_pmu;
+ u64 sval = val;
+ int ret = 0;
host_perfmon = pmuver_to_perfmon(kvm_arm_pmu_get_pmuver_limit());
@@ -305,18 +335,39 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
if (kvm_vcpu_has_pmu(vcpu) != valid_pmu)
return -EINVAL;
+ mutex_lock(&arch->config_lock);
/* We can only differ with PerfMon, and anything else is an error */
val ^= read_id_reg(vcpu, rd);
val &= ~ARM64_FEATURE_MASK(ID_DFR0_EL1_PerfMon);
- if (val)
- return -EINVAL;
+ if (val) {
+ ret = -EINVAL;
+ goto out;
+ }
- if (valid_pmu)
- vcpu->kvm->arch.dfr0_pmuver.imp = perfmon_to_pmuver(perfmon);
- else
- vcpu->kvm->arch.dfr0_pmuver.unimp = perfmon_to_pmuver(perfmon);
+ /* Only allow userspace to change the idregs before VM running */
+ if (test_bit(KVM_ARCH_FLAG_HAS_RAN_ONCE, &vcpu->kvm->arch.flags)) {
+ if (sval != read_id_reg(vcpu, rd))
+ ret = -EBUSY;
+ } else {
+ if (valid_pmu) {
+ val = IDREG(vcpu->kvm, SYS_ID_DFR0_EL1);
+ val &= ~ID_DFR0_EL1_PerfMon_MASK;
+ val |= FIELD_PREP(ID_DFR0_EL1_PerfMon_MASK, perfmon);
+ IDREG(vcpu->kvm, SYS_ID_DFR0_EL1) = val;
+
+ val = IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1);
+ val &= ~ID_AA64DFR0_EL1_PMUVer_MASK;
+ val |= FIELD_PREP(ID_AA64DFR0_EL1_PMUVer_MASK, perfmon_to_pmuver(perfmon));
+ IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1) = val;
+ } else {
+ assign_bit(KVM_ARCH_FLAG_VCPU_HAS_IMP_DEF_PMU, &vcpu->kvm->arch.flags,
+ perfmon == ID_DFR0_EL1_PerfMon_IMPDEF);
+ }
+ }
- return 0;
+out:
+ mutex_unlock(&arch->config_lock);
+ return ret;
}
/* sys_reg_desc initialiser for known cpufeature ID registers */
@@ -517,4 +568,15 @@ void kvm_arm_init_id_regs(struct kvm *kvm)
}
IDREG(kvm, SYS_ID_AA64PFR0_EL1) = val;
+ /*
+ * Initialise the default PMUver before there is a chance to
+ * create an actual PMU.
+ */
+ val = IDREG(kvm, SYS_ID_AA64DFR0_EL1);
+
+ val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer);
+ val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer),
+ kvm_arm_pmu_get_pmuver_limit());
+
+ IDREG(kvm, SYS_ID_AA64DFR0_EL1) = val;
}
diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h
index 628775334d5e..e486347b297d 100644
--- a/include/kvm/arm_pmu.h
+++ b/include/kvm/arm_pmu.h
@@ -92,8 +92,9 @@ void kvm_vcpu_pmu_restore_host(struct kvm_vcpu *vcpu);
/*
* Evaluates as true when emulating PMUv3p5, and false otherwise.
*/
-#define kvm_pmu_is_3p5(vcpu) \
- (vcpu->kvm->arch.dfr0_pmuver.imp >= ID_AA64DFR0_EL1_PMUVer_V3P5)
+#define kvm_pmu_is_3p5(vcpu) \
+ (FIELD_GET(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer), \
+ IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1)) >= ID_AA64DFR0_EL1_PMUVer_V3P5)
u8 kvm_arm_pmu_get_pmuver_limit(void);
--
2.40.1.495.gc816e09b53d-goog
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v8 5/6] KVM: arm64: Reuse fields of sys_reg_desc for idreg
2023-05-03 17:16 [PATCH v8 0/6] Support writable CPU ID registers from userspace Jing Zhang
` (3 preceding siblings ...)
2023-05-03 17:16 ` [PATCH v8 4/6] KVM: arm64: Use per guest ID register for ID_AA64DFR0_EL1.PMUVer Jing Zhang
@ 2023-05-03 17:16 ` Jing Zhang
2023-05-16 10:26 ` Cornelia Huck
2023-05-03 17:16 ` [PATCH v8 6/6] KVM: arm64: Refactor writings for PMUVer/CSV2/CSV3 Jing Zhang
2023-05-16 10:37 ` [PATCH v8 0/6] Support writable CPU ID registers from userspace Shameerali Kolothum Thodi
6 siblings, 1 reply; 34+ messages in thread
From: Jing Zhang @ 2023-05-03 17:16 UTC (permalink / raw)
To: KVM, KVMARM, ARMLinux, Marc Zyngier, Oliver Upton
Cc: Will Deacon, Paolo Bonzini, James Morse, Alexandru Elisei,
Suzuki K Poulose, Fuad Tabba, Reiji Watanabe,
Raghavendra Rao Ananta, Jing Zhang
Since reset() and val are not used for idreg in sys_reg_desc, they would
be used with other purposes for idregs.
The callback reset() would be used to return KVM sanitised id register
values. The u64 val would be used as mask for writable fields in idregs.
Only bits with 1 in val are writable from userspace.
Signed-off-by: Jing Zhang <jingzhangos@google.com>
---
arch/arm64/kvm/id_regs.c | 44 +++++++++++++++++++----------
arch/arm64/kvm/sys_regs.c | 59 +++++++++++++++++++++++++++------------
arch/arm64/kvm/sys_regs.h | 10 ++++---
3 files changed, 77 insertions(+), 36 deletions(-)
diff --git a/arch/arm64/kvm/id_regs.c b/arch/arm64/kvm/id_regs.c
index 0a04a90a8676..64d40aa395be 100644
--- a/arch/arm64/kvm/id_regs.c
+++ b/arch/arm64/kvm/id_regs.c
@@ -55,6 +55,11 @@ static u8 pmuver_to_perfmon(u8 pmuver)
}
}
+static u64 general_read_kvm_sanitised_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd)
+{
+ return read_sanitised_ftr_reg(reg_to_encoding(rd));
+}
+
u64 kvm_arm_read_id_reg(const struct kvm_vcpu *vcpu, u32 id)
{
u64 val = IDREG(vcpu->kvm, id);
@@ -370,6 +375,17 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
return ret;
}
+/*
+ * Since reset() callback and field val are not used for idregs, they will be
+ * used for specific purposes for idregs.
+ * The reset() would return KVM sanitised register value. The value would be the
+ * same as the host kernel sanitised value if there is no KVM sanitisation.
+ * The val would be used as a mask indicating writable fields for the idreg.
+ * Only bits with 1 are writable from userspace. This mask might not be
+ * necessary in the future whenever all ID registers are enabled as writable
+ * from userspace.
+ */
+
/* sys_reg_desc initialiser for known cpufeature ID registers */
#define ID_SANITISED(name) { \
SYS_DESC(SYS_##name), \
@@ -377,6 +393,8 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
.get_user = get_id_reg, \
.set_user = set_id_reg, \
.visibility = id_visibility, \
+ .reset = general_read_kvm_sanitised_reg,\
+ .val = 0, \
}
/* sys_reg_desc initialiser for known cpufeature ID registers */
@@ -386,6 +404,8 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
.get_user = get_id_reg, \
.set_user = set_id_reg, \
.visibility = aa32_id_visibility, \
+ .reset = general_read_kvm_sanitised_reg,\
+ .val = 0, \
}
/*
@@ -398,7 +418,9 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
.access = access_id_reg, \
.get_user = get_id_reg, \
.set_user = set_id_reg, \
- .visibility = raz_visibility \
+ .visibility = raz_visibility, \
+ .reset = NULL, \
+ .val = 0, \
}
/*
@@ -412,6 +434,8 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
.get_user = get_id_reg, \
.set_user = set_id_reg, \
.visibility = raz_visibility, \
+ .reset = NULL, \
+ .val = 0, \
}
const struct sys_reg_desc id_reg_descs[KVM_ARM_ID_REG_NUM] = {
@@ -522,10 +546,7 @@ int emulate_id_reg(struct kvm_vcpu *vcpu, struct sys_reg_params *params)
return 1;
}
-/*
- * Set the guest's ID registers that are defined in id_reg_descs[]
- * with ID_SANITISED() to the host's sanitized value.
- */
+/* Initialize the guest's ID registers with KVM sanitised values. */
void kvm_arm_init_id_regs(struct kvm *kvm)
{
u64 val;
@@ -535,16 +556,11 @@ void kvm_arm_init_id_regs(struct kvm *kvm)
for (i = 0; i < ARRAY_SIZE(id_reg_descs); i++) {
id = reg_to_encoding(&id_reg_descs[i]);
- /*
- * Some hidden ID registers which are not in arm64_ftr_regs[]
- * would cause warnings from read_sanitised_ftr_reg().
- * Skip those ID registers to avoid the warnings.
- */
- if (id_reg_descs[i].visibility == raz_visibility)
- /* Hidden or reserved ID register */
- continue;
+ val = 0;
+ /* Read KVM sanitised register value if available */
+ if (id_reg_descs[i].reset)
+ val = id_reg_descs[i].reset(NULL, &id_reg_descs[i]);
- val = read_sanitised_ftr_reg(id);
IDREG(kvm, id) = val;
}
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 2e7b17d14b4f..8bd0916a8068 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -540,10 +540,11 @@ static int get_bvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
return 0;
}
-static void reset_bvr(struct kvm_vcpu *vcpu,
+static u64 reset_bvr(struct kvm_vcpu *vcpu,
const struct sys_reg_desc *rd)
{
vcpu->arch.vcpu_debug_state.dbg_bvr[rd->CRm] = rd->val;
+ return rd->val;
}
static bool trap_bcr(struct kvm_vcpu *vcpu,
@@ -576,10 +577,11 @@ static int get_bcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
return 0;
}
-static void reset_bcr(struct kvm_vcpu *vcpu,
+static u64 reset_bcr(struct kvm_vcpu *vcpu,
const struct sys_reg_desc *rd)
{
vcpu->arch.vcpu_debug_state.dbg_bcr[rd->CRm] = rd->val;
+ return rd->val;
}
static bool trap_wvr(struct kvm_vcpu *vcpu,
@@ -613,10 +615,11 @@ static int get_wvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
return 0;
}
-static void reset_wvr(struct kvm_vcpu *vcpu,
+static u64 reset_wvr(struct kvm_vcpu *vcpu,
const struct sys_reg_desc *rd)
{
vcpu->arch.vcpu_debug_state.dbg_wvr[rd->CRm] = rd->val;
+ return rd->val;
}
static bool trap_wcr(struct kvm_vcpu *vcpu,
@@ -649,25 +652,28 @@ static int get_wcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
return 0;
}
-static void reset_wcr(struct kvm_vcpu *vcpu,
+static u64 reset_wcr(struct kvm_vcpu *vcpu,
const struct sys_reg_desc *rd)
{
vcpu->arch.vcpu_debug_state.dbg_wcr[rd->CRm] = rd->val;
+ return rd->val;
}
-static void reset_amair_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
+static u64 reset_amair_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
{
u64 amair = read_sysreg(amair_el1);
vcpu_write_sys_reg(vcpu, amair, AMAIR_EL1);
+ return amair;
}
-static void reset_actlr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
+static u64 reset_actlr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
{
u64 actlr = read_sysreg(actlr_el1);
vcpu_write_sys_reg(vcpu, actlr, ACTLR_EL1);
+ return actlr;
}
-static void reset_mpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
+static u64 reset_mpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
{
u64 mpidr;
@@ -681,7 +687,10 @@ static void reset_mpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
mpidr = (vcpu->vcpu_id & 0x0f) << MPIDR_LEVEL_SHIFT(0);
mpidr |= ((vcpu->vcpu_id >> 4) & 0xff) << MPIDR_LEVEL_SHIFT(1);
mpidr |= ((vcpu->vcpu_id >> 12) & 0xff) << MPIDR_LEVEL_SHIFT(2);
- vcpu_write_sys_reg(vcpu, (1ULL << 31) | mpidr, MPIDR_EL1);
+ mpidr |= (1ULL << 31);
+ vcpu_write_sys_reg(vcpu, mpidr, MPIDR_EL1);
+
+ return mpidr;
}
static unsigned int pmu_visibility(const struct kvm_vcpu *vcpu,
@@ -693,13 +702,13 @@ static unsigned int pmu_visibility(const struct kvm_vcpu *vcpu,
return REG_HIDDEN;
}
-static void reset_pmu_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
+static u64 reset_pmu_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
{
u64 n, mask = BIT(ARMV8_PMU_CYCLE_IDX);
/* No PMU available, any PMU reg may UNDEF... */
if (!kvm_arm_support_pmu_v3())
- return;
+ return 0;
n = read_sysreg(pmcr_el0) >> ARMV8_PMU_PMCR_N_SHIFT;
n &= ARMV8_PMU_PMCR_N_MASK;
@@ -708,33 +717,41 @@ static void reset_pmu_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
reset_unknown(vcpu, r);
__vcpu_sys_reg(vcpu, r->reg) &= mask;
+
+ return __vcpu_sys_reg(vcpu, r->reg);
}
-static void reset_pmevcntr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
+static u64 reset_pmevcntr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
{
reset_unknown(vcpu, r);
__vcpu_sys_reg(vcpu, r->reg) &= GENMASK(31, 0);
+
+ return __vcpu_sys_reg(vcpu, r->reg);
}
-static void reset_pmevtyper(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
+static u64 reset_pmevtyper(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
{
reset_unknown(vcpu, r);
__vcpu_sys_reg(vcpu, r->reg) &= ARMV8_PMU_EVTYPE_MASK;
+
+ return __vcpu_sys_reg(vcpu, r->reg);
}
-static void reset_pmselr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
+static u64 reset_pmselr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
{
reset_unknown(vcpu, r);
__vcpu_sys_reg(vcpu, r->reg) &= ARMV8_PMU_COUNTER_MASK;
+
+ return __vcpu_sys_reg(vcpu, r->reg);
}
-static void reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
+static u64 reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
{
u64 pmcr;
/* No PMU available, PMCR_EL0 may UNDEF... */
if (!kvm_arm_support_pmu_v3())
- return;
+ return 0;
/* Only preserve PMCR_EL0.N, and reset the rest to 0 */
pmcr = read_sysreg(pmcr_el0) & (ARMV8_PMU_PMCR_N_MASK << ARMV8_PMU_PMCR_N_SHIFT);
@@ -742,6 +759,8 @@ static void reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
pmcr |= ARMV8_PMU_PMCR_LC;
__vcpu_sys_reg(vcpu, r->reg) = pmcr;
+
+ return __vcpu_sys_reg(vcpu, r->reg);
}
static bool check_pmu_access_disabled(struct kvm_vcpu *vcpu, u64 flags)
@@ -1220,7 +1239,7 @@ static bool access_clidr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
* Fabricate a CLIDR_EL1 value instead of using the real value, which can vary
* by the physical CPU which the vcpu currently resides in.
*/
-static void reset_clidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
+static u64 reset_clidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
{
u64 ctr_el0 = read_sanitised_ftr_reg(SYS_CTR_EL0);
u64 clidr;
@@ -1268,6 +1287,8 @@ static void reset_clidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
clidr |= 2 << CLIDR_TTYPE_SHIFT(loc);
__vcpu_sys_reg(vcpu, r->reg) = clidr;
+
+ return __vcpu_sys_reg(vcpu, r->reg);
}
static int set_clidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
@@ -2621,19 +2642,21 @@ id_to_sys_reg_desc(struct kvm_vcpu *vcpu, u64 id,
*/
#define FUNCTION_INVARIANT(reg) \
- static void get_##reg(struct kvm_vcpu *v, \
+ static u64 get_##reg(struct kvm_vcpu *v, \
const struct sys_reg_desc *r) \
{ \
((struct sys_reg_desc *)r)->val = read_sysreg(reg); \
+ return ((struct sys_reg_desc *)r)->val; \
}
FUNCTION_INVARIANT(midr_el1)
FUNCTION_INVARIANT(revidr_el1)
FUNCTION_INVARIANT(aidr_el1)
-static void get_ctr_el0(struct kvm_vcpu *v, const struct sys_reg_desc *r)
+static u64 get_ctr_el0(struct kvm_vcpu *v, const struct sys_reg_desc *r)
{
((struct sys_reg_desc *)r)->val = read_sanitised_ftr_reg(SYS_CTR_EL0);
+ return ((struct sys_reg_desc *)r)->val;
}
/* ->val is filled in by kvm_sys_reg_table_init() */
diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
index e88fd77309b2..21869319f6e1 100644
--- a/arch/arm64/kvm/sys_regs.h
+++ b/arch/arm64/kvm/sys_regs.h
@@ -65,12 +65,12 @@ struct sys_reg_desc {
const struct sys_reg_desc *);
/* Initialization for vcpu. */
- void (*reset)(struct kvm_vcpu *, const struct sys_reg_desc *);
+ u64 (*reset)(struct kvm_vcpu *, const struct sys_reg_desc *);
/* Index into sys_reg[], or 0 if we don't need to save it. */
int reg;
- /* Value (usually reset value) */
+ /* Value (usually reset value), or write mask for idregs */
u64 val;
/* Custom get/set_user functions, fallback to generic if NULL */
@@ -123,19 +123,21 @@ static inline bool read_zero(struct kvm_vcpu *vcpu,
}
/* Reset functions */
-static inline void reset_unknown(struct kvm_vcpu *vcpu,
+static inline u64 reset_unknown(struct kvm_vcpu *vcpu,
const struct sys_reg_desc *r)
{
BUG_ON(!r->reg);
BUG_ON(r->reg >= NR_SYS_REGS);
__vcpu_sys_reg(vcpu, r->reg) = 0x1de7ec7edbadc0deULL;
+ return __vcpu_sys_reg(vcpu, r->reg);
}
-static inline void reset_val(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
+static inline u64 reset_val(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
{
BUG_ON(!r->reg);
BUG_ON(r->reg >= NR_SYS_REGS);
__vcpu_sys_reg(vcpu, r->reg) = r->val;
+ return __vcpu_sys_reg(vcpu, r->reg);
}
static inline unsigned int sysreg_visibility(const struct kvm_vcpu *vcpu,
--
2.40.1.495.gc816e09b53d-goog
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v8 6/6] KVM: arm64: Refactor writings for PMUVer/CSV2/CSV3
2023-05-03 17:16 [PATCH v8 0/6] Support writable CPU ID registers from userspace Jing Zhang
` (4 preceding siblings ...)
2023-05-03 17:16 ` [PATCH v8 5/6] KVM: arm64: Reuse fields of sys_reg_desc for idreg Jing Zhang
@ 2023-05-03 17:16 ` Jing Zhang
2023-05-17 22:00 ` Jitindar Singh, Suraj
2023-05-19 23:25 ` Suraj Jitindar Singh
2023-05-16 10:37 ` [PATCH v8 0/6] Support writable CPU ID registers from userspace Shameerali Kolothum Thodi
6 siblings, 2 replies; 34+ messages in thread
From: Jing Zhang @ 2023-05-03 17:16 UTC (permalink / raw)
To: KVM, KVMARM, ARMLinux, Marc Zyngier, Oliver Upton
Cc: Will Deacon, Paolo Bonzini, James Morse, Alexandru Elisei,
Suzuki K Poulose, Fuad Tabba, Reiji Watanabe,
Raghavendra Rao Ananta, Jing Zhang
Refactor writings for ID_AA64PFR0_EL1.[CSV2|CSV3],
ID_AA64DFR0_EL1.PMUVer and ID_DFR0_ELF.PerfMon based on utilities
introduced by ID register descriptor array.
Signed-off-by: Jing Zhang <jingzhangos@google.com>
---
arch/arm64/include/asm/cpufeature.h | 1 +
arch/arm64/kernel/cpufeature.c | 2 +-
arch/arm64/kvm/id_regs.c | 361 ++++++++++++++++++----------
3 files changed, 242 insertions(+), 122 deletions(-)
diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index 6bf013fb110d..dc769c2eb7a4 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -915,6 +915,7 @@ static inline unsigned int get_vmid_bits(u64 mmfr1)
return 8;
}
+s64 arm64_ftr_safe_value(const struct arm64_ftr_bits *ftrp, s64 new, s64 cur);
struct arm64_ftr_reg *get_arm64_ftr_reg(u32 sys_id);
extern struct arm64_ftr_override id_aa64mmfr1_override;
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 2e3e55139777..677ec4fe9f6b 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -791,7 +791,7 @@ static u64 arm64_ftr_set_value(const struct arm64_ftr_bits *ftrp, s64 reg,
return reg;
}
-static s64 arm64_ftr_safe_value(const struct arm64_ftr_bits *ftrp, s64 new,
+s64 arm64_ftr_safe_value(const struct arm64_ftr_bits *ftrp, s64 new,
s64 cur)
{
s64 ret = 0;
diff --git a/arch/arm64/kvm/id_regs.c b/arch/arm64/kvm/id_regs.c
index 64d40aa395be..6cd56c9e6428 100644
--- a/arch/arm64/kvm/id_regs.c
+++ b/arch/arm64/kvm/id_regs.c
@@ -18,6 +18,86 @@
#include "sys_regs.h"
+static s64 kvm_arm64_ftr_safe_value(u32 id, const struct arm64_ftr_bits *ftrp,
+ s64 new, s64 cur)
+{
+ struct arm64_ftr_bits kvm_ftr = *ftrp;
+
+ /* Some features have different safe value type in KVM than host features */
+ switch (id) {
+ case SYS_ID_AA64DFR0_EL1:
+ if (kvm_ftr.shift == ID_AA64DFR0_EL1_PMUVer_SHIFT)
+ kvm_ftr.type = FTR_LOWER_SAFE;
+ break;
+ case SYS_ID_DFR0_EL1:
+ if (kvm_ftr.shift == ID_DFR0_EL1_PerfMon_SHIFT)
+ kvm_ftr.type = FTR_LOWER_SAFE;
+ break;
+ }
+
+ return arm64_ftr_safe_value(&kvm_ftr, new, cur);
+}
+
+/**
+ * arm64_check_features() - Check if a feature register value constitutes
+ * a subset of features indicated by the idreg's KVM sanitised limit.
+ *
+ * This function will check if each feature field of @val is the "safe" value
+ * against idreg's KVM sanitised limit return from reset() callback.
+ * If a field value in @val is the same as the one in limit, it is always
+ * considered the safe value regardless For register fields that are not in
+ * writable, only the value in limit is considered the safe value.
+ *
+ * Return: 0 if all the fields are safe. Otherwise, return negative errno.
+ */
+static int arm64_check_features(struct kvm_vcpu *vcpu,
+ const struct sys_reg_desc *rd,
+ u64 val)
+{
+ const struct arm64_ftr_reg *ftr_reg;
+ const struct arm64_ftr_bits *ftrp = NULL;
+ u32 id = reg_to_encoding(rd);
+ u64 writable_mask = rd->val;
+ u64 limit = 0;
+ u64 mask = 0;
+
+ /* For hidden and unallocated idregs without reset, only val = 0 is allowed. */
+ if (rd->reset) {
+ limit = rd->reset(vcpu, rd);
+ ftr_reg = get_arm64_ftr_reg(id);
+ if (!ftr_reg)
+ return -EINVAL;
+ ftrp = ftr_reg->ftr_bits;
+ }
+
+ for (; ftrp && ftrp->width; ftrp++) {
+ s64 f_val, f_lim, safe_val;
+ u64 ftr_mask;
+
+ ftr_mask = arm64_ftr_mask(ftrp);
+ if ((ftr_mask & writable_mask) != ftr_mask)
+ continue;
+
+ f_val = arm64_ftr_value(ftrp, val);
+ f_lim = arm64_ftr_value(ftrp, limit);
+ mask |= ftr_mask;
+
+ if (f_val == f_lim)
+ safe_val = f_val;
+ else
+ safe_val = kvm_arm64_ftr_safe_value(id, ftrp, f_val, f_lim);
+
+ if (safe_val != f_val)
+ return -E2BIG;
+ }
+
+ /* For fields that are not writable, values in limit are the safe values. */
+ if ((val & ~mask) != (limit & ~mask))
+ return -E2BIG;
+
+ return 0;
+}
+
static u8 vcpu_pmuver(const struct kvm_vcpu *vcpu)
{
if (kvm_vcpu_has_pmu(vcpu))
@@ -68,7 +148,6 @@ u64 kvm_arm_read_id_reg(const struct kvm_vcpu *vcpu, u32 id)
case SYS_ID_AA64PFR0_EL1:
if (!vcpu_has_sve(vcpu))
val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_SVE);
- val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_AMU);
if (kvm_vgic_global_state.type == VGIC_V3) {
val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_GIC);
val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_GIC), 1);
@@ -95,15 +174,10 @@ u64 kvm_arm_read_id_reg(const struct kvm_vcpu *vcpu, u32 id)
val &= ~ARM64_FEATURE_MASK(ID_AA64ISAR2_EL1_WFxT);
break;
case SYS_ID_AA64DFR0_EL1:
- /* Limit debug to ARMv8.0 */
- val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_DebugVer);
- val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_DebugVer), 6);
/* Set PMUver to the required version */
val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer);
val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer),
vcpu_pmuver(vcpu));
- /* Hide SPE from guests */
- val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMSVer);
break;
case SYS_ID_DFR0_EL1:
val &= ~ARM64_FEATURE_MASK(ID_DFR0_EL1_PerfMon);
@@ -167,11 +241,23 @@ static int get_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
static int set_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
u64 val)
{
- /* This is what we mean by invariant: you can't change it. */
- if (val != read_id_reg(vcpu, rd))
- return -EINVAL;
+ struct kvm_arch *arch = &vcpu->kvm->arch;
+ u32 id = reg_to_encoding(rd);
+ int ret = 0;
- return 0;
+ mutex_lock(&arch->config_lock);
+ /* Only allow userspace to change the idregs before VM running */
+ if (test_bit(KVM_ARCH_FLAG_HAS_RAN_ONCE, &vcpu->kvm->arch.flags)) {
+ if (val != read_id_reg(vcpu, rd))
+ ret = -EBUSY;
+ } else {
+ ret = arm64_check_features(vcpu, rd, val);
+ if (!ret)
+ IDREG(vcpu->kvm, id) = val;
+ }
+ mutex_unlock(&arch->config_lock);
+
+ return ret;
}
static unsigned int id_visibility(const struct kvm_vcpu *vcpu,
@@ -203,14 +289,40 @@ static unsigned int aa32_id_visibility(const struct kvm_vcpu *vcpu,
return id_visibility(vcpu, r);
}
+static u64 read_sanitised_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
+ const struct sys_reg_desc *rd)
+{
+ u64 val;
+ u32 id = reg_to_encoding(rd);
+
+ val = read_sanitised_ftr_reg(id);
+ /*
+ * The default is to expose CSV2 == 1 if the HW isn't affected.
+ * Although this is a per-CPU feature, we make it global because
+ * asymmetric systems are just a nuisance.
+ *
+ * Userspace can override this as long as it doesn't promise
+ * the impossible.
+ */
+ if (arm64_get_spectre_v2_state() == SPECTRE_UNAFFECTED) {
+ val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV2);
+ val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV2), 1);
+ }
+ if (arm64_get_meltdown_state() == SPECTRE_UNAFFECTED) {
+ val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV3);
+ val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV3), 1);
+ }
+
+ val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_AMU);
+
+ return val;
+}
+
static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
const struct sys_reg_desc *rd,
u64 val)
{
- struct kvm_arch *arch = &vcpu->kvm->arch;
- u64 sval = val;
u8 csv2, csv3;
- int ret = 0;
/*
* Allow AA64PFR0_EL1.CSV2 to be set from userspace as long as
@@ -226,26 +338,30 @@ static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
if (csv3 > 1 || (csv3 && arm64_get_meltdown_state() != SPECTRE_UNAFFECTED))
return -EINVAL;
- mutex_lock(&arch->config_lock);
- /* We can only differ with CSV[23], and anything else is an error */
- val ^= read_id_reg(vcpu, rd);
- val &= ~(ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV2) |
- ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV3));
- if (val) {
- ret = -EINVAL;
- goto out;
- }
+ return set_id_reg(vcpu, rd, val);
+}
- /* Only allow userspace to change the idregs before VM running */
- if (test_bit(KVM_ARCH_FLAG_HAS_RAN_ONCE, &vcpu->kvm->arch.flags)) {
- if (sval != read_id_reg(vcpu, rd))
- ret = -EBUSY;
- } else {
- IDREG(vcpu->kvm, reg_to_encoding(rd)) = sval;
- }
-out:
- mutex_unlock(&arch->config_lock);
- return ret;
+static u64 read_sanitised_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
+ const struct sys_reg_desc *rd)
+{
+ u64 val;
+ u32 id = reg_to_encoding(rd);
+
+ val = read_sanitised_ftr_reg(id);
+ /* Limit debug to ARMv8.0 */
+ val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_DebugVer);
+ val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_DebugVer), 6);
+ /*
+ * Initialise the default PMUver before there is a chance to
+ * create an actual PMU.
+ */
+ val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer);
+ val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer),
+ kvm_arm_pmu_get_pmuver_limit());
+ /* Hide SPE from guests */
+ val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMSVer);
+
+ return val;
}
static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
@@ -255,7 +371,6 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
struct kvm_arch *arch = &vcpu->kvm->arch;
u8 pmuver, host_pmuver;
bool valid_pmu;
- u64 sval = val;
int ret = 0;
host_pmuver = kvm_arm_pmu_get_pmuver_limit();
@@ -277,40 +392,61 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
return -EINVAL;
mutex_lock(&arch->config_lock);
- /* We can only differ with PMUver, and anything else is an error */
- val ^= read_id_reg(vcpu, rd);
- val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer);
- if (val) {
- ret = -EINVAL;
- goto out;
- }
-
/* Only allow userspace to change the idregs before VM running */
if (test_bit(KVM_ARCH_FLAG_HAS_RAN_ONCE, &vcpu->kvm->arch.flags)) {
- if (sval != read_id_reg(vcpu, rd))
+ if (val != read_id_reg(vcpu, rd))
ret = -EBUSY;
- } else {
- if (valid_pmu) {
- val = IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1);
- val &= ~ID_AA64DFR0_EL1_PMUVer_MASK;
- val |= FIELD_PREP(ID_AA64DFR0_EL1_PMUVer_MASK, pmuver);
- IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1) = val;
-
- val = IDREG(vcpu->kvm, SYS_ID_DFR0_EL1);
- val &= ~ID_DFR0_EL1_PerfMon_MASK;
- val |= FIELD_PREP(ID_DFR0_EL1_PerfMon_MASK, pmuver_to_perfmon(pmuver));
- IDREG(vcpu->kvm, SYS_ID_DFR0_EL1) = val;
- } else {
- assign_bit(KVM_ARCH_FLAG_VCPU_HAS_IMP_DEF_PMU, &vcpu->kvm->arch.flags,
- pmuver == ID_AA64DFR0_EL1_PMUVer_IMP_DEF);
- }
+ goto out;
+ }
+
+ if (!valid_pmu) {
+ /*
+ * Ignore the PMUVer filed in @val. The PMUVer would be determined
+ * by arch flags bit KVM_ARCH_FLAG_VCPU_HAS_IMP_DEF_PMU,
+ */
+ pmuver = FIELD_GET(ID_AA64DFR0_EL1_PMUVer_MASK,
+ IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1));
+ val &= ~ID_AA64DFR0_EL1_PMUVer_MASK;
+ val |= FIELD_PREP(ID_AA64DFR0_EL1_PMUVer_MASK, pmuver);
}
+ ret = arm64_check_features(vcpu, rd, val);
+ if (ret)
+ goto out;
+
+ IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1) = val;
+
+ val = IDREG(vcpu->kvm, SYS_ID_DFR0_EL1);
+ val &= ~ID_DFR0_EL1_PerfMon_MASK;
+ val |= FIELD_PREP(ID_DFR0_EL1_PerfMon_MASK, pmuver_to_perfmon(pmuver));
+ IDREG(vcpu->kvm, SYS_ID_DFR0_EL1) = val;
+
+ if (!valid_pmu)
+ assign_bit(KVM_ARCH_FLAG_VCPU_HAS_IMP_DEF_PMU, &vcpu->kvm->arch.flags,
+ pmuver == ID_AA64DFR0_EL1_PMUVer_IMP_DEF);
+
out:
mutex_unlock(&arch->config_lock);
return ret;
}
+static u64 read_sanitised_id_dfr0_el1(struct kvm_vcpu *vcpu,
+ const struct sys_reg_desc *rd)
+{
+ u64 val;
+ u32 id = reg_to_encoding(rd);
+
+ val = read_sanitised_ftr_reg(id);
+ /*
+ * Initialise the default PMUver before there is a chance to
+ * create an actual PMU.
+ */
+ val &= ~ARM64_FEATURE_MASK(ID_DFR0_EL1_PerfMon);
+ val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_DFR0_EL1_PerfMon), kvm_arm_pmu_get_pmuver_limit());
+
+ return val;
+}
+
static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
const struct sys_reg_desc *rd,
u64 val)
@@ -318,7 +454,6 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
struct kvm_arch *arch = &vcpu->kvm->arch;
u8 perfmon, host_perfmon;
bool valid_pmu;
- u64 sval = val;
int ret = 0;
host_perfmon = pmuver_to_perfmon(kvm_arm_pmu_get_pmuver_limit());
@@ -341,35 +476,39 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
return -EINVAL;
mutex_lock(&arch->config_lock);
- /* We can only differ with PerfMon, and anything else is an error */
- val ^= read_id_reg(vcpu, rd);
- val &= ~ARM64_FEATURE_MASK(ID_DFR0_EL1_PerfMon);
- if (val) {
- ret = -EINVAL;
- goto out;
- }
-
/* Only allow userspace to change the idregs before VM running */
if (test_bit(KVM_ARCH_FLAG_HAS_RAN_ONCE, &vcpu->kvm->arch.flags)) {
- if (sval != read_id_reg(vcpu, rd))
+ if (val != read_id_reg(vcpu, rd))
ret = -EBUSY;
- } else {
- if (valid_pmu) {
- val = IDREG(vcpu->kvm, SYS_ID_DFR0_EL1);
- val &= ~ID_DFR0_EL1_PerfMon_MASK;
- val |= FIELD_PREP(ID_DFR0_EL1_PerfMon_MASK, perfmon);
- IDREG(vcpu->kvm, SYS_ID_DFR0_EL1) = val;
-
- val = IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1);
- val &= ~ID_AA64DFR0_EL1_PMUVer_MASK;
- val |= FIELD_PREP(ID_AA64DFR0_EL1_PMUVer_MASK, perfmon_to_pmuver(perfmon));
- IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1) = val;
- } else {
- assign_bit(KVM_ARCH_FLAG_VCPU_HAS_IMP_DEF_PMU, &vcpu->kvm->arch.flags,
- perfmon == ID_DFR0_EL1_PerfMon_IMPDEF);
- }
+ goto out;
+ }
+
+ if (!valid_pmu) {
+ /*
+ * Ignore the PerfMon filed in @val. The PerfMon would be determined
+ * by arch flags bit KVM_ARCH_FLAG_VCPU_HAS_IMP_DEF_PMU,
+ */
+ perfmon = FIELD_GET(ID_DFR0_EL1_PerfMon_MASK,
+ IDREG(vcpu->kvm, SYS_ID_DFR0_EL1));
+ val &= ~ID_DFR0_EL1_PerfMon_MASK;
+ val |= FIELD_PREP(ID_DFR0_EL1_PerfMon_MASK, perfmon);
}
+ ret = arm64_check_features(vcpu, rd, val);
+ if (ret)
+ goto out;
+
+ IDREG(vcpu->kvm, SYS_ID_DFR0_EL1) = val;
+
+ val = IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1);
+ val &= ~ID_AA64DFR0_EL1_PMUVer_MASK;
+ val |= FIELD_PREP(ID_AA64DFR0_EL1_PMUVer_MASK, perfmon_to_pmuver(perfmon));
+ IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1) = val;
+
+ if (!valid_pmu)
+ assign_bit(KVM_ARCH_FLAG_VCPU_HAS_IMP_DEF_PMU, &vcpu->kvm->arch.flags,
+ perfmon == ID_DFR0_EL1_PerfMon_IMPDEF);
+
out:
mutex_unlock(&arch->config_lock);
return ret;
@@ -448,9 +587,13 @@ const struct sys_reg_desc id_reg_descs[KVM_ARM_ID_REG_NUM] = {
/* CRm=1 */
AA32_ID_SANITISED(ID_PFR0_EL1),
AA32_ID_SANITISED(ID_PFR1_EL1),
- { SYS_DESC(SYS_ID_DFR0_EL1), .access = access_id_reg,
- .get_user = get_id_reg, .set_user = set_id_dfr0_el1,
- .visibility = aa32_id_visibility, },
+ { SYS_DESC(SYS_ID_DFR0_EL1),
+ .access = access_id_reg,
+ .get_user = get_id_reg,
+ .set_user = set_id_dfr0_el1,
+ .visibility = aa32_id_visibility,
+ .reset = read_sanitised_id_dfr0_el1,
+ .val = ID_DFR0_EL1_PerfMon_MASK, },
ID_HIDDEN(ID_AFR0_EL1),
AA32_ID_SANITISED(ID_MMFR0_EL1),
AA32_ID_SANITISED(ID_MMFR1_EL1),
@@ -479,8 +622,12 @@ const struct sys_reg_desc id_reg_descs[KVM_ARM_ID_REG_NUM] = {
/* AArch64 ID registers */
/* CRm=4 */
- { SYS_DESC(SYS_ID_AA64PFR0_EL1), .access = access_id_reg,
- .get_user = get_id_reg, .set_user = set_id_aa64pfr0_el1, },
+ { SYS_DESC(SYS_ID_AA64PFR0_EL1),
+ .access = access_id_reg,
+ .get_user = get_id_reg,
+ .set_user = set_id_aa64pfr0_el1,
+ .reset = read_sanitised_id_aa64pfr0_el1,
+ .val = ID_AA64PFR0_EL1_CSV2_MASK | ID_AA64PFR0_EL1_CSV3_MASK, },
ID_SANITISED(ID_AA64PFR1_EL1),
ID_UNALLOCATED(4, 2),
ID_UNALLOCATED(4, 3),
@@ -490,8 +637,12 @@ const struct sys_reg_desc id_reg_descs[KVM_ARM_ID_REG_NUM] = {
ID_UNALLOCATED(4, 7),
/* CRm=5 */
- { SYS_DESC(SYS_ID_AA64DFR0_EL1), .access = access_id_reg,
- .get_user = get_id_reg, .set_user = set_id_aa64dfr0_el1, },
+ { SYS_DESC(SYS_ID_AA64DFR0_EL1),
+ .access = access_id_reg,
+ .get_user = get_id_reg,
+ .set_user = set_id_aa64dfr0_el1,
+ .reset = read_sanitised_id_aa64dfr0_el1,
+ .val = ID_AA64DFR0_EL1_PMUVer_MASK, },
ID_SANITISED(ID_AA64DFR1_EL1),
ID_UNALLOCATED(5, 2),
ID_UNALLOCATED(5, 3),
@@ -563,36 +714,4 @@ void kvm_arm_init_id_regs(struct kvm *kvm)
IDREG(kvm, id) = val;
}
-
- /*
- * The default is to expose CSV2 == 1 if the HW isn't affected.
- * Although this is a per-CPU feature, we make it global because
- * asymmetric systems are just a nuisance.
- *
- * Userspace can override this as long as it doesn't promise
- * the impossible.
- */
- val = IDREG(kvm, SYS_ID_AA64PFR0_EL1);
-
- if (arm64_get_spectre_v2_state() == SPECTRE_UNAFFECTED) {
- val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV2);
- val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV2), 1);
- }
- if (arm64_get_meltdown_state() == SPECTRE_UNAFFECTED) {
- val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV3);
- val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV3), 1);
- }
-
- IDREG(kvm, SYS_ID_AA64PFR0_EL1) = val;
- /*
- * Initialise the default PMUver before there is a chance to
- * create an actual PMU.
- */
- val = IDREG(kvm, SYS_ID_AA64DFR0_EL1);
-
- val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer);
- val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer),
- kvm_arm_pmu_get_pmuver_limit());
-
- IDREG(kvm, SYS_ID_AA64DFR0_EL1) = val;
}
--
2.40.1.495.gc816e09b53d-goog
^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH v8 3/6] KVM: arm64: Use per guest ID register for ID_AA64PFR0_EL1.[CSV2|CSV3]
2023-05-03 17:16 ` [PATCH v8 3/6] KVM: arm64: Use per guest ID register for ID_AA64PFR0_EL1.[CSV2|CSV3] Jing Zhang
@ 2023-05-03 23:43 ` kernel test robot
0 siblings, 0 replies; 34+ messages in thread
From: kernel test robot @ 2023-05-03 23:43 UTC (permalink / raw)
To: Jing Zhang, KVM, KVMARM, ARMLinux, Marc Zyngier, Oliver Upton
Cc: oe-kbuild-all, Will Deacon, Paolo Bonzini, James Morse,
Alexandru Elisei, Suzuki K Poulose, Fuad Tabba, Reiji Watanabe,
Raghavendra Rao Ananta, Jing Zhang
Hi Jing,
kernel test robot noticed the following build errors:
[auto build test ERROR on 6a8f57ae2eb07ab39a6f0ccad60c760743051026]
url: https://github.com/intel-lab-lkp/linux/commits/Jing-Zhang/KVM-arm64-Move-CPU-ID-feature-registers-emulation-into-a-separate-file/20230504-011759
base: 6a8f57ae2eb07ab39a6f0ccad60c760743051026
patch link: https://lore.kernel.org/r/20230503171618.2020461-4-jingzhangos%40google.com
patch subject: [PATCH v8 3/6] KVM: arm64: Use per guest ID register for ID_AA64PFR0_EL1.[CSV2|CSV3]
config: arm64-allyesconfig (https://download.01.org/0day-ci/archive/20230504/202305040748.hUxGyrJF-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/5c887874c5459c9690cf3eac8b68022d72789533
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Jing-Zhang/KVM-arm64-Move-CPU-ID-feature-registers-emulation-into-a-separate-file/20230504-011759
git checkout 5c887874c5459c9690cf3eac8b68022d72789533
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm64 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash arch/arm64/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202305040748.hUxGyrJF-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from include/linux/rhashtable-types.h:14,
from include/linux/ipc.h:7,
from include/uapi/linux/sem.h:5,
from include/linux/sem.h:5,
from include/linux/sched.h:15,
from include/linux/hardirq.h:9,
from include/linux/kvm_host.h:7,
from arch/arm64/kvm/id_regs.c:13:
arch/arm64/kvm/id_regs.c: In function 'get_id_reg':
>> arch/arm64/kvm/id_regs.c:152:25: error: 'struct kvm_arch' has no member named 'config_lock'
152 | mutex_lock(&arch->config_lock);
| ^~
include/linux/mutex.h:187:44: note: in definition of macro 'mutex_lock'
187 | #define mutex_lock(lock) mutex_lock_nested(lock, 0)
| ^~~~
arch/arm64/kvm/id_regs.c:154:27: error: 'struct kvm_arch' has no member named 'config_lock'
154 | mutex_unlock(&arch->config_lock);
| ^~
arch/arm64/kvm/id_regs.c: In function 'set_id_aa64pfr0_el1':
arch/arm64/kvm/id_regs.c:221:25: error: 'struct kvm_arch' has no member named 'config_lock'
221 | mutex_lock(&arch->config_lock);
| ^~
include/linux/mutex.h:187:44: note: in definition of macro 'mutex_lock'
187 | #define mutex_lock(lock) mutex_lock_nested(lock, 0)
| ^~~~
arch/arm64/kvm/id_regs.c:239:27: error: 'struct kvm_arch' has no member named 'config_lock'
239 | mutex_unlock(&arch->config_lock);
| ^~
vim +152 arch/arm64/kvm/id_regs.c
139
140 /*
141 * cpufeature ID register user accessors
142 *
143 * For now, these registers are immutable for userspace, so no values
144 * are stored, and for set_id_reg() we don't allow the effective value
145 * to be changed.
146 */
147 static int get_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
148 u64 *val)
149 {
150 struct kvm_arch *arch = &vcpu->kvm->arch;
151
> 152 mutex_lock(&arch->config_lock);
153 *val = read_id_reg(vcpu, rd);
154 mutex_unlock(&arch->config_lock);
155
156 return 0;
157 }
158
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v8 5/6] KVM: arm64: Reuse fields of sys_reg_desc for idreg
2023-05-03 17:16 ` [PATCH v8 5/6] KVM: arm64: Reuse fields of sys_reg_desc for idreg Jing Zhang
@ 2023-05-16 10:26 ` Cornelia Huck
2023-05-16 19:10 ` Jing Zhang
0 siblings, 1 reply; 34+ messages in thread
From: Cornelia Huck @ 2023-05-16 10:26 UTC (permalink / raw)
To: Jing Zhang, KVM, KVMARM, ARMLinux, Marc Zyngier, Oliver Upton
Cc: Will Deacon, Paolo Bonzini, James Morse, Alexandru Elisei,
Suzuki K Poulose, Fuad Tabba, Reiji Watanabe,
Raghavendra Rao Ananta, Jing Zhang
On Wed, May 03 2023, Jing Zhang <jingzhangos@google.com> wrote:
> Since reset() and val are not used for idreg in sys_reg_desc, they would
> be used with other purposes for idregs.
> The callback reset() would be used to return KVM sanitised id register
> values. The u64 val would be used as mask for writable fields in idregs.
> Only bits with 1 in val are writable from userspace.
>
> Signed-off-by: Jing Zhang <jingzhangos@google.com>
> ---
> arch/arm64/kvm/id_regs.c | 44 +++++++++++++++++++----------
> arch/arm64/kvm/sys_regs.c | 59 +++++++++++++++++++++++++++------------
> arch/arm64/kvm/sys_regs.h | 10 ++++---
> 3 files changed, 77 insertions(+), 36 deletions(-)
>
(...)
> diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
> index e88fd77309b2..21869319f6e1 100644
> --- a/arch/arm64/kvm/sys_regs.h
> +++ b/arch/arm64/kvm/sys_regs.h
> @@ -65,12 +65,12 @@ struct sys_reg_desc {
> const struct sys_reg_desc *);
>
> /* Initialization for vcpu. */
Maybe be a bit more verbose here?
/* Initialization for vcpu. Return initialized value, or KVM sanitized
value for id registers. */
> - void (*reset)(struct kvm_vcpu *, const struct sys_reg_desc *);
> + u64 (*reset)(struct kvm_vcpu *, const struct sys_reg_desc *);
>
> /* Index into sys_reg[], or 0 if we don't need to save it. */
> int reg;
>
> - /* Value (usually reset value) */
> + /* Value (usually reset value), or write mask for idregs */
> u64 val;
>
> /* Custom get/set_user functions, fallback to generic if NULL */
^ permalink raw reply [flat|nested] 34+ messages in thread
* RE: [PATCH v8 0/6] Support writable CPU ID registers from userspace
2023-05-03 17:16 [PATCH v8 0/6] Support writable CPU ID registers from userspace Jing Zhang
` (5 preceding siblings ...)
2023-05-03 17:16 ` [PATCH v8 6/6] KVM: arm64: Refactor writings for PMUVer/CSV2/CSV3 Jing Zhang
@ 2023-05-16 10:37 ` Shameerali Kolothum Thodi
[not found] ` <868rdomtfo.wl-maz@kernel.org>
6 siblings, 1 reply; 34+ messages in thread
From: Shameerali Kolothum Thodi @ 2023-05-16 10:37 UTC (permalink / raw)
To: Jing Zhang, KVM, KVMARM, ARMLinux, Marc Zyngier, Oliver Upton
Cc: Will Deacon, Paolo Bonzini, James Morse, Alexandru Elisei,
Suzuki K Poulose, Fuad Tabba, Reiji Watanabe,
Raghavendra Rao Ananta
> -----Original Message-----
> From: Jing Zhang [mailto:jingzhangos@google.com]
> Sent: 03 May 2023 18:16
> To: KVM <kvm@vger.kernel.org>; KVMARM <kvmarm@lists.linux.dev>;
> ARMLinux <linux-arm-kernel@lists.infradead.org>; Marc Zyngier
> <maz@kernel.org>; Oliver Upton <oupton@google.com>
> Cc: Will Deacon <will@kernel.org>; Paolo Bonzini <pbonzini@redhat.com>;
> James Morse <james.morse@arm.com>; Alexandru Elisei
> <alexandru.elisei@arm.com>; Suzuki K Poulose <suzuki.poulose@arm.com>;
> Fuad Tabba <tabba@google.com>; Reiji Watanabe <reijiw@google.com>;
> Raghavendra Rao Ananta <rananta@google.com>; Jing Zhang
> <jingzhangos@google.com>
> Subject: [PATCH v8 0/6] Support writable CPU ID registers from userspace
>
> This patchset refactors/adds code to support writable per guest CPU ID
> feature
> registers. Part of the code/ideas are from
> https://lore.kernel.org/all/20220419065544.3616948-1-reijiw@google.com
Hi Jing/Reiji,
Just to check the status on the above mentioned series "KVM: arm64: Make CPU
ID registers writable by userspace". Is there any plan to respin that one soon?
(Sorry, not sure there is any other series in progress for that support currently)
Please let me know.
Thanks,
Shameer
> .
> No functional change is intended in this patchset. With the new CPU ID
> feature
> registers infrastructure, only writtings of ID_AA64PFR0_EL1.[CSV2|CSV3],
> ID_AA64DFR0_EL1.PMUVer and ID_DFR0_ELF.PerfMon are allowed as KVM
> does before.
>
> Writable (Configurable) per guest CPU ID feature registers are useful for
> creating/migrating guest on ARM CPUs with different kinds of features.
>
> This patchset uses kvm->arch.config_lock from Oliver's lock inversion fixes at
> https://lore.kernel.org/linux-arm-kernel/20230327164747.2466958-1-oliver.
> upton@linux.dev/
>
> ---
>
> * v7 -> v8
> - Move idregs table sanity check to kvm_sys_reg_table_init.
> - Only allow userspace writing before VM running.
> - No lock is hold for guest access to idregs.
> - Addressed some other comments from Reiji and Oliver.
>
> * v6 -> v7
> - Rebased to v6.3-rc7.
> - Add helpers for idregs read/write.
> - Guard all idregs reads/writes.
> - Add code to fix features' safe value type which is different for KVM than
> for the host.
>
> * v5 -> v6
> - Rebased to v6.3-rc5.
> - Reuse struct sys_reg_desc's reset() callback and field val for KVM.
> sanitisation function and writable mask instead of creating a new data
> structure for idregs.
> - Use get_arm64_ftr_reg() instead of exposing idregs ftr_bits array.
>
> * v4 -> v5
> - Rebased to 2fad20ae05cb (kvmarm/next)
> Merge branch kvm-arm64/selftest/misc-6,4 into kvmarm-master/next
> - Use kvm->arch.config_lock to guard update to multiple VM scope idregs
> to avoid lock inversion
> - Add back IDREG() macro for idregs access
> - Refactor struct id_reg_desc by using existing infrastructure.
> - Addressed many other comments from Marc.
>
> * v3 -> v4
> - Remove IDREG() macro for ID reg access, use simple array access instead
> - Rename kvm_arm_read_id_reg_with_encoding() to
> kvm_arm_read_id_reg()
> - Save perfmon value in ID_DFR0_EL1 instead of pmuver
> - Update perfmon in ID_DFR0_EL1 and pmuver in ID_AA64DFR0_EL1
> atomically
> - Remove kvm_vcpu_has_pmu() in macro kvm_pmu_is_3p5()
> - Improve ID register sanity checking in kvm_arm_check_idreg_table()
>
> * v2 -> v3
> - Rebased to 96a4627dbbd4 (kvmarm/next)
> Merge tag ' https://github.com/oupton/linux tags/kvmarm-6.3' from
> into kvmarm-master/next
> - Add id registere emulation entry point function emulate_id_reg
> - Fix consistency for ID_AA64DFR0_EL1.PMUVer and
> ID_DFR0_EL1.PerfMon
> - Improve the checking for id register table by ensuring that every entry
> has
> the correct id register encoding.
> - Addressed other comments from Reiji and Marc.
>
> * v1 -> v2
> - Rebase to 7121a2e1d107 (kvmarm/next) Merge branch
> kvm-arm64/nv-prefix into kvmarm/next
> - Address writing issue for PMUVer
>
> [1]
> https://lore.kernel.org/all/20230201025048.205820-1-jingzhangos@google.
> com
> [2]
> https://lore.kernel.org/all/20230212215830.2975485-1-jingzhangos@googl
> e.com
> [3]
> https://lore.kernel.org/all/20230228062246.1222387-1-jingzhangos@googl
> e.com
> [4]
> https://lore.kernel.org/all/20230317050637.766317-1-jingzhangos@google.
> com
> [5]
> https://lore.kernel.org/all/20230402183735.3011540-1-jingzhangos@googl
> e.com
> [6]
> https://lore.kernel.org/all/20230404035344.4043856-1-jingzhangos@googl
> e.com
> [7]
> https://lore.kernel.org/all/20230424234704.2571444-1-jingzhangos@googl
> e.com
>
> ---
>
> Jing Zhang (6):
> KVM: arm64: Move CPU ID feature registers emulation into a separate
> file
> KVM: arm64: Save ID registers' sanitized value per guest
> KVM: arm64: Use per guest ID register for ID_AA64PFR0_EL1.[CSV2|CSV3]
> KVM: arm64: Use per guest ID register for ID_AA64DFR0_EL1.PMUVer
> KVM: arm64: Reuse fields of sys_reg_desc for idreg
> KVM: arm64: Refactor writings for PMUVer/CSV2/CSV3
>
> arch/arm64/include/asm/cpufeature.h | 1 +
> arch/arm64/include/asm/kvm_host.h | 33 +-
> arch/arm64/kernel/cpufeature.c | 2 +-
> arch/arm64/kvm/Makefile | 2 +-
> arch/arm64/kvm/arm.c | 24 +-
> arch/arm64/kvm/id_regs.c | 717
> ++++++++++++++++++++++++++++
> arch/arm64/kvm/sys_regs.c | 534 ++++-----------------
> arch/arm64/kvm/sys_regs.h | 28 +-
> include/kvm/arm_pmu.h | 5 +-
> 9 files changed, 864 insertions(+), 482 deletions(-)
> create mode 100644 arch/arm64/kvm/id_regs.c
>
>
> base-commit: 6a8f57ae2eb07ab39a6f0ccad60c760743051026
> --
> 2.40.1.495.gc816e09b53d-goog
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* RE: [PATCH v8 0/6] Support writable CPU ID registers from userspace
[not found] ` <868rdomtfo.wl-maz@kernel.org>
@ 2023-05-16 11:11 ` Shameerali Kolothum Thodi
2023-05-16 11:55 ` Cornelia Huck
0 siblings, 1 reply; 34+ messages in thread
From: Shameerali Kolothum Thodi @ 2023-05-16 11:11 UTC (permalink / raw)
To: Marc Zyngier
Cc: Jing Zhang, KVM, KVMARM, ARMLinux, Oliver Upton, Will Deacon,
Paolo Bonzini, James Morse, Alexandru Elisei, Suzuki K Poulose,
Fuad Tabba, Reiji Watanabe, Raghavendra Rao Ananta
> -----Original Message-----
> From: Marc Zyngier [mailto:maz@kernel.org]
> Sent: 16 May 2023 12:01
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: Jing Zhang <jingzhangos@google.com>; KVM <kvm@vger.kernel.org>;
> KVMARM <kvmarm@lists.linux.dev>; ARMLinux
> <linux-arm-kernel@lists.infradead.org>; Oliver Upton <oupton@google.com>;
> Will Deacon <will@kernel.org>; Paolo Bonzini <pbonzini@redhat.com>;
> James Morse <james.morse@arm.com>; Alexandru Elisei
> <alexandru.elisei@arm.com>; Suzuki K Poulose <suzuki.poulose@arm.com>;
> Fuad Tabba <tabba@google.com>; Reiji Watanabe <reijiw@google.com>;
> Raghavendra Rao Ananta <rananta@google.com>
> Subject: Re: [PATCH v8 0/6] Support writable CPU ID registers from
> userspace
>
> On Tue, 16 May 2023 11:37:20 +0100,
> Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> wrote:
> >
> > > -----Original Message-----
> > > From: Jing Zhang [mailto:jingzhangos@google.com]
> > > Sent: 03 May 2023 18:16
> > > To: KVM <kvm@vger.kernel.org>; KVMARM <kvmarm@lists.linux.dev>;
> > > ARMLinux <linux-arm-kernel@lists.infradead.org>; Marc Zyngier
> > > <maz@kernel.org>; Oliver Upton <oupton@google.com>
> > > Cc: Will Deacon <will@kernel.org>; Paolo Bonzini
> <pbonzini@redhat.com>;
> > > James Morse <james.morse@arm.com>; Alexandru Elisei
> > > <alexandru.elisei@arm.com>; Suzuki K Poulose
> <suzuki.poulose@arm.com>;
> > > Fuad Tabba <tabba@google.com>; Reiji Watanabe <reijiw@google.com>;
> > > Raghavendra Rao Ananta <rananta@google.com>; Jing Zhang
> > > <jingzhangos@google.com>
> > > Subject: [PATCH v8 0/6] Support writable CPU ID registers from
> userspace
> > >
> > > This patchset refactors/adds code to support writable per guest CPU ID
> > > feature
> > > registers. Part of the code/ideas are from
> > >
> https://lore.kernel.org/all/20220419065544.3616948-1-reijiw@google.com
> >
> > Hi Jing/Reiji,
> >
> > Just to check the status on the above mentioned series "KVM: arm64: Make
> CPU
> > ID registers writable by userspace". Is there any plan to respin that one
> soon?
> > (Sorry, not sure there is any other series in progress for that support
> currently)
>
> I think this still is the latest, which I'm about to review again. I'd
> appreciate if you could have a look to!
Thanks Marc for confirming. Will go through. We do have some requirement to
add support for Qemu CPU models/migration between different hosts.
Thanks,
Shameer
>
> Thanks,
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 34+ messages in thread
* RE: [PATCH v8 0/6] Support writable CPU ID registers from userspace
2023-05-16 11:11 ` Shameerali Kolothum Thodi
@ 2023-05-16 11:55 ` Cornelia Huck
2023-05-16 13:11 ` Marc Zyngier
0 siblings, 1 reply; 34+ messages in thread
From: Cornelia Huck @ 2023-05-16 11:55 UTC (permalink / raw)
To: Shameerali Kolothum Thodi, Marc Zyngier
Cc: Jing Zhang, KVM, KVMARM, ARMLinux, Oliver Upton, Will Deacon,
Paolo Bonzini, James Morse, Alexandru Elisei, Suzuki K Poulose,
Fuad Tabba, Reiji Watanabe, Raghavendra Rao Ananta
On Tue, May 16 2023, Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> wrote:
>> -----Original Message-----
>> From: Marc Zyngier [mailto:maz@kernel.org]
>> Sent: 16 May 2023 12:01
>> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
>> Cc: Jing Zhang <jingzhangos@google.com>; KVM <kvm@vger.kernel.org>;
>> KVMARM <kvmarm@lists.linux.dev>; ARMLinux
>> <linux-arm-kernel@lists.infradead.org>; Oliver Upton <oupton@google.com>;
>> Will Deacon <will@kernel.org>; Paolo Bonzini <pbonzini@redhat.com>;
>> James Morse <james.morse@arm.com>; Alexandru Elisei
>> <alexandru.elisei@arm.com>; Suzuki K Poulose <suzuki.poulose@arm.com>;
>> Fuad Tabba <tabba@google.com>; Reiji Watanabe <reijiw@google.com>;
>> Raghavendra Rao Ananta <rananta@google.com>
>> Subject: Re: [PATCH v8 0/6] Support writable CPU ID registers from
>> userspace
>>
>> On Tue, 16 May 2023 11:37:20 +0100,
>> Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
>> wrote:
>> >
>> > > -----Original Message-----
>> > > From: Jing Zhang [mailto:jingzhangos@google.com]
>> > > Sent: 03 May 2023 18:16
>> > > To: KVM <kvm@vger.kernel.org>; KVMARM <kvmarm@lists.linux.dev>;
>> > > ARMLinux <linux-arm-kernel@lists.infradead.org>; Marc Zyngier
>> > > <maz@kernel.org>; Oliver Upton <oupton@google.com>
>> > > Cc: Will Deacon <will@kernel.org>; Paolo Bonzini
>> <pbonzini@redhat.com>;
>> > > James Morse <james.morse@arm.com>; Alexandru Elisei
>> > > <alexandru.elisei@arm.com>; Suzuki K Poulose
>> <suzuki.poulose@arm.com>;
>> > > Fuad Tabba <tabba@google.com>; Reiji Watanabe <reijiw@google.com>;
>> > > Raghavendra Rao Ananta <rananta@google.com>; Jing Zhang
>> > > <jingzhangos@google.com>
>> > > Subject: [PATCH v8 0/6] Support writable CPU ID registers from
>> userspace
>> > >
>> > > This patchset refactors/adds code to support writable per guest CPU ID
>> > > feature
>> > > registers. Part of the code/ideas are from
>> > >
>> https://lore.kernel.org/all/20220419065544.3616948-1-reijiw@google.com
>> >
>> > Hi Jing/Reiji,
>> >
>> > Just to check the status on the above mentioned series "KVM: arm64: Make
>> CPU
>> > ID registers writable by userspace". Is there any plan to respin that one
>> soon?
>> > (Sorry, not sure there is any other series in progress for that support
>> currently)
>>
>> I think this still is the latest, which I'm about to review again. I'd
>> appreciate if you could have a look to!
>
> Thanks Marc for confirming. Will go through. We do have some requirement to
> add support for Qemu CPU models/migration between different hosts.
Do you have more concrete ideas for QEMU CPU models already? Asking
because I wanted to talk about this at KVM Forum, so collecting what
others would like to do seems like a good idea :)
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v8 0/6] Support writable CPU ID registers from userspace
2023-05-16 11:55 ` Cornelia Huck
@ 2023-05-16 13:11 ` Marc Zyngier
2023-05-16 13:44 ` Shameerali Kolothum Thodi
` (2 more replies)
0 siblings, 3 replies; 34+ messages in thread
From: Marc Zyngier @ 2023-05-16 13:11 UTC (permalink / raw)
To: Cornelia Huck
Cc: Shameerali Kolothum Thodi, Jing Zhang, KVM, KVMARM, ARMLinux,
Oliver Upton, Will Deacon, Paolo Bonzini, James Morse,
Alexandru Elisei, Suzuki K Poulose, Fuad Tabba, Reiji Watanabe,
Raghavendra Rao Ananta
On Tue, 16 May 2023 12:55:14 +0100,
Cornelia Huck <cohuck@redhat.com> wrote:
>
> Do you have more concrete ideas for QEMU CPU models already? Asking
> because I wanted to talk about this at KVM Forum, so collecting what
> others would like to do seems like a good idea :)
I'm not being asked, but I'll share my thoughts anyway! ;-)
I don't think CPU models are necessarily the most important thing.
Specially when you look at the diversity of the ecosystem (and even
the same CPU can be configured in different ways at integration
time). Case in point, Neoverse N1 which can have its I/D caches made
coherent or not. And the guest really wants to know which one it is
(you can only lie in one direction).
But being able to control the feature set exposed to the guest from
userspace is a huge benefit in terms of migration.
Now, this is only half of the problem (and we're back to the CPU
model): most of these CPUs have various degrees of brokenness. Most of
the workarounds have to be implemented by the guest, and are keyed on
the MIDR values. So somehow, you need to be able to expose *all* the
possible MIDR values that a guest can observe in its lifetime.
I have a vague prototype for that that I'd need to dust off and
finish, because that's also needed for this very silly construct
called big-little...
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 34+ messages in thread
* RE: [PATCH v8 0/6] Support writable CPU ID registers from userspace
2023-05-16 13:11 ` Marc Zyngier
@ 2023-05-16 13:44 ` Shameerali Kolothum Thodi
2023-05-16 14:21 ` Cornelia Huck
2023-05-16 14:19 ` Cornelia Huck
2023-05-16 16:31 ` Oliver Upton
2 siblings, 1 reply; 34+ messages in thread
From: Shameerali Kolothum Thodi @ 2023-05-16 13:44 UTC (permalink / raw)
To: Marc Zyngier, Cornelia Huck
Cc: Jing Zhang, KVM, KVMARM, ARMLinux, Oliver Upton, Will Deacon,
Paolo Bonzini, James Morse, Alexandru Elisei, Suzuki K Poulose,
Fuad Tabba, Reiji Watanabe, Raghavendra Rao Ananta
> -----Original Message-----
> From: Marc Zyngier [mailto:maz@kernel.org]
> Sent: 16 May 2023 14:12
> To: Cornelia Huck <cohuck@redhat.com>
> Cc: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> Jing Zhang <jingzhangos@google.com>; KVM <kvm@vger.kernel.org>;
> KVMARM <kvmarm@lists.linux.dev>; ARMLinux
> <linux-arm-kernel@lists.infradead.org>; Oliver Upton <oupton@google.com>;
> Will Deacon <will@kernel.org>; Paolo Bonzini <pbonzini@redhat.com>;
> James Morse <james.morse@arm.com>; Alexandru Elisei
> <alexandru.elisei@arm.com>; Suzuki K Poulose <suzuki.poulose@arm.com>;
> Fuad Tabba <tabba@google.com>; Reiji Watanabe <reijiw@google.com>;
> Raghavendra Rao Ananta <rananta@google.com>
> Subject: Re: [PATCH v8 0/6] Support writable CPU ID registers from
> userspace
>
> On Tue, 16 May 2023 12:55:14 +0100,
> Cornelia Huck <cohuck@redhat.com> wrote:
> >
> > Do you have more concrete ideas for QEMU CPU models already? Asking
> > because I wanted to talk about this at KVM Forum, so collecting what
> > others would like to do seems like a good idea :)
>
> I'm not being asked, but I'll share my thoughts anyway! ;-)
>
> I don't think CPU models are necessarily the most important thing.
> Specially when you look at the diversity of the ecosystem (and even
> the same CPU can be configured in different ways at integration
> time). Case in point, Neoverse N1 which can have its I/D caches made
> coherent or not. And the guest really wants to know which one it is
> (you can only lie in one direction).
>
> But being able to control the feature set exposed to the guest from
> userspace is a huge benefit in terms of migration.
Yes, this is what we also need and was thinking of adding a named CPU with
common min feature set exposed to Guest. There were some previous
attempts to add the basic support in Qemu here,
https://lists.gnu.org/archive/html/qemu-devel/2020-11/msg00087.html
> Now, this is only half of the problem (and we're back to the CPU
> model): most of these CPUs have various degrees of brokenness. Most of
> the workarounds have to be implemented by the guest, and are keyed on
> the MIDR values. So somehow, you need to be able to expose *all* the
> possible MIDR values that a guest can observe in its lifetime.
Ok. This will be a problem and I am not sure this has an impact on our
platforms or not.
Thanks,
Shameer
> I have a vague prototype for that that I'd need to dust off and
> finish, because that's also needed for this very silly construct
> called big-little...
>
> Thanks,
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v8 0/6] Support writable CPU ID registers from userspace
2023-05-16 13:11 ` Marc Zyngier
2023-05-16 13:44 ` Shameerali Kolothum Thodi
@ 2023-05-16 14:19 ` Cornelia Huck
2023-05-16 16:01 ` Marc Zyngier
2023-05-16 16:31 ` Oliver Upton
2 siblings, 1 reply; 34+ messages in thread
From: Cornelia Huck @ 2023-05-16 14:19 UTC (permalink / raw)
To: Marc Zyngier
Cc: Shameerali Kolothum Thodi, Jing Zhang, KVM, KVMARM, ARMLinux,
Oliver Upton, Will Deacon, Paolo Bonzini, James Morse,
Alexandru Elisei, Suzuki K Poulose, Fuad Tabba, Reiji Watanabe,
Raghavendra Rao Ananta
On Tue, May 16 2023, Marc Zyngier <maz@kernel.org> wrote:
> On Tue, 16 May 2023 12:55:14 +0100,
> Cornelia Huck <cohuck@redhat.com> wrote:
>>
>> Do you have more concrete ideas for QEMU CPU models already? Asking
>> because I wanted to talk about this at KVM Forum, so collecting what
>> others would like to do seems like a good idea :)
>
> I'm not being asked, but I'll share my thoughts anyway! ;-)
>
> I don't think CPU models are necessarily the most important thing.
> Specially when you look at the diversity of the ecosystem (and even
> the same CPU can be configured in different ways at integration
> time). Case in point, Neoverse N1 which can have its I/D caches made
> coherent or not. And the guest really wants to know which one it is
> (you can only lie in one direction).
>
> But being able to control the feature set exposed to the guest from
> userspace is a huge benefit in terms of migration.
Certainly; the important part is that we can keep the guest ABI
stable... which parts match to a "CPU model" in the way other
architectures use it is an interesting question. It almost certainly
will look different from e.g. s390, where we only have to deal with a
single manufacturer.
I'm wondering whether we'll end up building frankenmonster CPUs.
Another interesting aspect is how KVM ends up influencing what the guest
sees on the CPU level, as in the case where we migrate across matching
CPUs, but with a different software level. I think we want userspace to
control that to some extent, but I'm not sure if this fully matches the
CPU model context.
>
> Now, this is only half of the problem (and we're back to the CPU
> model): most of these CPUs have various degrees of brokenness. Most of
> the workarounds have to be implemented by the guest, and are keyed on
> the MIDR values. So somehow, you need to be able to expose *all* the
> possible MIDR values that a guest can observe in its lifetime.
Fun is to be had...
>
> I have a vague prototype for that that I'd need to dust off and
> finish, because that's also needed for this very silly construct
> called big-little...
That would be cool to see. Or at least interesting ;)
^ permalink raw reply [flat|nested] 34+ messages in thread
* RE: [PATCH v8 0/6] Support writable CPU ID registers from userspace
2023-05-16 13:44 ` Shameerali Kolothum Thodi
@ 2023-05-16 14:21 ` Cornelia Huck
0 siblings, 0 replies; 34+ messages in thread
From: Cornelia Huck @ 2023-05-16 14:21 UTC (permalink / raw)
To: Shameerali Kolothum Thodi, Marc Zyngier
Cc: Jing Zhang, KVM, KVMARM, ARMLinux, Oliver Upton, Will Deacon,
Paolo Bonzini, James Morse, Alexandru Elisei, Suzuki K Poulose,
Fuad Tabba, Reiji Watanabe, Raghavendra Rao Ananta
On Tue, May 16 2023, Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> wrote:
>> -----Original Message-----
>> From: Marc Zyngier [mailto:maz@kernel.org]
>> Sent: 16 May 2023 14:12
>> To: Cornelia Huck <cohuck@redhat.com>
>> Cc: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
>> Jing Zhang <jingzhangos@google.com>; KVM <kvm@vger.kernel.org>;
>> KVMARM <kvmarm@lists.linux.dev>; ARMLinux
>> <linux-arm-kernel@lists.infradead.org>; Oliver Upton <oupton@google.com>;
>> Will Deacon <will@kernel.org>; Paolo Bonzini <pbonzini@redhat.com>;
>> James Morse <james.morse@arm.com>; Alexandru Elisei
>> <alexandru.elisei@arm.com>; Suzuki K Poulose <suzuki.poulose@arm.com>;
>> Fuad Tabba <tabba@google.com>; Reiji Watanabe <reijiw@google.com>;
>> Raghavendra Rao Ananta <rananta@google.com>
>> Subject: Re: [PATCH v8 0/6] Support writable CPU ID registers from
>> userspace
>>
>> On Tue, 16 May 2023 12:55:14 +0100,
>> Cornelia Huck <cohuck@redhat.com> wrote:
>> >
>> > Do you have more concrete ideas for QEMU CPU models already? Asking
>> > because I wanted to talk about this at KVM Forum, so collecting what
>> > others would like to do seems like a good idea :)
>>
>> I'm not being asked, but I'll share my thoughts anyway! ;-)
>>
>> I don't think CPU models are necessarily the most important thing.
>> Specially when you look at the diversity of the ecosystem (and even
>> the same CPU can be configured in different ways at integration
>> time). Case in point, Neoverse N1 which can have its I/D caches made
>> coherent or not. And the guest really wants to know which one it is
>> (you can only lie in one direction).
>>
>> But being able to control the feature set exposed to the guest from
>> userspace is a huge benefit in terms of migration.
>
> Yes, this is what we also need and was thinking of adding a named CPU with
> common min feature set exposed to Guest. There were some previous
> attempts to add the basic support in Qemu here,
>
> https://lists.gnu.org/archive/html/qemu-devel/2020-11/msg00087.html
Thanks for the link.
>
>> Now, this is only half of the problem (and we're back to the CPU
>> model): most of these CPUs have various degrees of brokenness. Most of
>> the workarounds have to be implemented by the guest, and are keyed on
>> the MIDR values. So somehow, you need to be able to expose *all* the
>> possible MIDR values that a guest can observe in its lifetime.
>
> Ok. This will be a problem and I am not sure this has an impact on our
> platforms or not.
Oh, I see that the MIDR fun had already been mentioned in a reply to the
first version of that patchset; this needs to be addressed for the
general case, I guess...
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v8 0/6] Support writable CPU ID registers from userspace
2023-05-16 14:19 ` Cornelia Huck
@ 2023-05-16 16:01 ` Marc Zyngier
2023-05-17 15:36 ` Cornelia Huck
0 siblings, 1 reply; 34+ messages in thread
From: Marc Zyngier @ 2023-05-16 16:01 UTC (permalink / raw)
To: Cornelia Huck
Cc: Shameerali Kolothum Thodi, Jing Zhang, KVM, KVMARM, ARMLinux,
Oliver Upton, Will Deacon, Paolo Bonzini, James Morse,
Alexandru Elisei, Suzuki K Poulose, Fuad Tabba, Reiji Watanabe,
Raghavendra Rao Ananta
On Tue, 16 May 2023 15:19:00 +0100,
Cornelia Huck <cohuck@redhat.com> wrote:
>
> On Tue, May 16 2023, Marc Zyngier <maz@kernel.org> wrote:
>
> > On Tue, 16 May 2023 12:55:14 +0100,
> > Cornelia Huck <cohuck@redhat.com> wrote:
> >>
> >> Do you have more concrete ideas for QEMU CPU models already? Asking
> >> because I wanted to talk about this at KVM Forum, so collecting what
> >> others would like to do seems like a good idea :)
> >
> > I'm not being asked, but I'll share my thoughts anyway! ;-)
> >
> > I don't think CPU models are necessarily the most important thing.
> > Specially when you look at the diversity of the ecosystem (and even
> > the same CPU can be configured in different ways at integration
> > time). Case in point, Neoverse N1 which can have its I/D caches made
> > coherent or not. And the guest really wants to know which one it is
> > (you can only lie in one direction).
> >
> > But being able to control the feature set exposed to the guest from
> > userspace is a huge benefit in terms of migration.
>
> Certainly; the important part is that we can keep the guest ABI
> stable... which parts match to a "CPU model" in the way other
> architectures use it is an interesting question. It almost certainly
> will look different from e.g. s390, where we only have to deal with a
> single manufacturer.
>
> I'm wondering whether we'll end up building frankenmonster CPUs.
We already do. KVM hides a bunch of things we don't want the guest to
see, either because we don't support the feature, or that we want to
present it with a different shape (cache topology, for example), and
these combination don't really exist in any physical implementation.
Which is why I don't really buy the "CPU model" concept as defined by
x86 and s390. We already are in a vastly different place.
The way I see it, you get a bunch of architectural features that can
be enabled/disabled depending on the underlying HW, hypervisor's
capabilities and userspace input. On top of that, there is a layer of
paint that tells you what is the overall implementation you could be
running on (that's what MIDR+REVIDR+AIDR tell you) so that you can
apply some unspeakable, uarch-specific hacks that keep the machine
going (got to love these CPU errata).
> Another interesting aspect is how KVM ends up influencing what the guest
> sees on the CPU level, as in the case where we migrate across matching
> CPUs, but with a different software level. I think we want userspace to
> control that to some extent, but I'm not sure if this fully matches the
> CPU model context.
I'm not sure I get the "different software level" part. Do you mean
VMM revisions?
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v8 1/6] KVM: arm64: Move CPU ID feature registers emulation into a separate file
2023-05-03 17:16 ` [PATCH v8 1/6] KVM: arm64: Move CPU ID feature registers emulation into a separate file Jing Zhang
@ 2023-05-16 16:11 ` Marc Zyngier
2023-05-16 19:14 ` Jing Zhang
0 siblings, 1 reply; 34+ messages in thread
From: Marc Zyngier @ 2023-05-16 16:11 UTC (permalink / raw)
To: Jing Zhang
Cc: KVM, KVMARM, ARMLinux, Oliver Upton, Will Deacon, Paolo Bonzini,
James Morse, Alexandru Elisei, Suzuki K Poulose, Fuad Tabba,
Reiji Watanabe, Raghavendra Rao Ananta
On Wed, 03 May 2023 18:16:13 +0100,
Jing Zhang <jingzhangos@google.com> wrote:
>
> Create a new file id_regs.c for CPU ID feature registers emulation code,
> which are moved from sys_regs.c and tweak sys_regs code accordingly.
>
> No functional change intended.
>
> Signed-off-by: Jing Zhang <jingzhangos@google.com>
> ---
> arch/arm64/kvm/Makefile | 2 +-
> arch/arm64/kvm/id_regs.c | 460 +++++++++++++++++++++++++++++++++++++
> arch/arm64/kvm/sys_regs.c | 464 ++++----------------------------------
> arch/arm64/kvm/sys_regs.h | 19 ++
> 4 files changed, 519 insertions(+), 426 deletions(-)
> create mode 100644 arch/arm64/kvm/id_regs.c
>
> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
> index c0c050e53157..a6a315fcd81e 100644
> --- a/arch/arm64/kvm/Makefile
> +++ b/arch/arm64/kvm/Makefile
> @@ -13,7 +13,7 @@ obj-$(CONFIG_KVM) += hyp/
> kvm-y += arm.o mmu.o mmio.o psci.o hypercalls.o pvtime.o \
> inject_fault.o va_layout.o handle_exit.o \
> guest.o debug.o reset.o sys_regs.o stacktrace.o \
> - vgic-sys-reg-v3.o fpsimd.o pkvm.o \
> + vgic-sys-reg-v3.o fpsimd.o pkvm.o id_regs.o \
> arch_timer.o trng.o vmid.o emulate-nested.o nested.o \
> vgic/vgic.o vgic/vgic-init.o \
> vgic/vgic-irqfd.o vgic/vgic-v2.o \
> diff --git a/arch/arm64/kvm/id_regs.c b/arch/arm64/kvm/id_regs.c
> new file mode 100644
> index 000000000000..96b4c43a5100
> --- /dev/null
> +++ b/arch/arm64/kvm/id_regs.c
> @@ -0,0 +1,460 @@
[...]
I really wonder why we move this to another file. It only creates
noise, and doesn't add much. Yes, the file is big. But now you need to
expose all the internal machinery that was private until now.
If you look at the global diffstat, this is "only" another 200
lines. I don't think this is worth the churn.
M.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v8 0/6] Support writable CPU ID registers from userspace
2023-05-16 13:11 ` Marc Zyngier
2023-05-16 13:44 ` Shameerali Kolothum Thodi
2023-05-16 14:19 ` Cornelia Huck
@ 2023-05-16 16:31 ` Oliver Upton
2023-05-16 16:44 ` Marc Zyngier
2 siblings, 1 reply; 34+ messages in thread
From: Oliver Upton @ 2023-05-16 16:31 UTC (permalink / raw)
To: Marc Zyngier
Cc: Cornelia Huck, Shameerali Kolothum Thodi, Jing Zhang, KVM,
KVMARM, ARMLinux, Oliver Upton, Will Deacon, Paolo Bonzini,
James Morse, Alexandru Elisei, Suzuki K Poulose, Fuad Tabba,
Reiji Watanabe, Raghavendra Rao Ananta
On Tue, May 16, 2023 at 02:11:30PM +0100, Marc Zyngier wrote:
> On Tue, 16 May 2023 12:55:14 +0100,
> Cornelia Huck <cohuck@redhat.com> wrote:
> >
> > Do you have more concrete ideas for QEMU CPU models already? Asking
> > because I wanted to talk about this at KVM Forum, so collecting what
> > others would like to do seems like a good idea :)
>
> I'm not being asked, but I'll share my thoughts anyway! ;-)
>
> I don't think CPU models are necessarily the most important thing.
> Specially when you look at the diversity of the ecosystem (and even
> the same CPU can be configured in different ways at integration
> time). Case in point, Neoverse N1 which can have its I/D caches made
> coherent or not. And the guest really wants to know which one it is
> (you can only lie in one direction).
>
> But being able to control the feature set exposed to the guest from
> userspace is a huge benefit in terms of migration.
>
> Now, this is only half of the problem (and we're back to the CPU
> model): most of these CPUs have various degrees of brokenness. Most of
> the workarounds have to be implemented by the guest, and are keyed on
> the MIDR values. So somehow, you need to be able to expose *all* the
> possible MIDR values that a guest can observe in its lifetime.
>
> I have a vague prototype for that that I'd need to dust off and
> finish, because that's also needed for this very silly construct
> called big-little...
And the third half of the problem is all of the other IP bits that get
strung together into an SOC :) Errata that live beyond the CPU can
become guest-visible (interconnect for example) and that becomes a bit
difficult to express to the guest OS. So, beyond something like a
big-little VM where the rest of the IP should be shared, I'm a bit
fearful of migrating a VM cross-system.
But hey, userspace is in the drivers seat and it can do as it pleases.
Hopefully we wouldn't need a KVM-specific PV interface for MIDR
enumeration. Perhaps the errata management spec could be expanded to
describe a set of CPU implementations and associated errata...
--
Thanks,
Oliver
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v8 0/6] Support writable CPU ID registers from userspace
2023-05-16 16:31 ` Oliver Upton
@ 2023-05-16 16:44 ` Marc Zyngier
2023-05-16 16:57 ` Oliver Upton
0 siblings, 1 reply; 34+ messages in thread
From: Marc Zyngier @ 2023-05-16 16:44 UTC (permalink / raw)
To: Oliver Upton
Cc: Cornelia Huck, Shameerali Kolothum Thodi, Jing Zhang, KVM,
KVMARM, ARMLinux, Oliver Upton, Will Deacon, Paolo Bonzini,
James Morse, Alexandru Elisei, Suzuki K Poulose, Fuad Tabba,
Reiji Watanabe, Raghavendra Rao Ananta
On Tue, 16 May 2023 17:31:29 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
>
> On Tue, May 16, 2023 at 02:11:30PM +0100, Marc Zyngier wrote:
> > On Tue, 16 May 2023 12:55:14 +0100,
> > Cornelia Huck <cohuck@redhat.com> wrote:
> > >
> > > Do you have more concrete ideas for QEMU CPU models already? Asking
> > > because I wanted to talk about this at KVM Forum, so collecting what
> > > others would like to do seems like a good idea :)
> >
> > I'm not being asked, but I'll share my thoughts anyway! ;-)
> >
> > I don't think CPU models are necessarily the most important thing.
> > Specially when you look at the diversity of the ecosystem (and even
> > the same CPU can be configured in different ways at integration
> > time). Case in point, Neoverse N1 which can have its I/D caches made
> > coherent or not. And the guest really wants to know which one it is
> > (you can only lie in one direction).
> >
> > But being able to control the feature set exposed to the guest from
> > userspace is a huge benefit in terms of migration.
> >
> > Now, this is only half of the problem (and we're back to the CPU
> > model): most of these CPUs have various degrees of brokenness. Most of
> > the workarounds have to be implemented by the guest, and are keyed on
> > the MIDR values. So somehow, you need to be able to expose *all* the
> > possible MIDR values that a guest can observe in its lifetime.
> >
> > I have a vague prototype for that that I'd need to dust off and
> > finish, because that's also needed for this very silly construct
> > called big-little...
>
> And the third half of the problem is all of the other IP bits that get
> strung together into an SOC :) Errata that live beyond the CPU can
> become guest-visible (interconnect for example) and that becomes a bit
> difficult to express to the guest OS. So, beyond something like a
> big-little VM where the rest of the IP should be shared, I'm a bit
> fearful of migrating a VM cross-system.
Indeed. But there isn't much we can do about that, and it should be
clear to anyone who's remotely involved in this crap that migration to
different systems is risky business.
> But hey, userspace is in the drivers seat and it can do as it pleases.
Exactly. We just need to give it enough of the proverbial rope...
> Hopefully we wouldn't need a KVM-specific PV interface for MIDR
> enumeration. Perhaps the errata management spec could be expanded to
> describe a set of CPU implementations and associated errata...
Hence finally making it clear the big-little is a large scale,
industry wide erratum? Sign me up! :D
More seriously, I'd expect this to be an ARM spec. But it wouldn't
hurt having a prototype that serves as a draft for the spec. Better
doing that than leaving it to... someone else.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v8 0/6] Support writable CPU ID registers from userspace
2023-05-16 16:44 ` Marc Zyngier
@ 2023-05-16 16:57 ` Oliver Upton
0 siblings, 0 replies; 34+ messages in thread
From: Oliver Upton @ 2023-05-16 16:57 UTC (permalink / raw)
To: Marc Zyngier
Cc: Cornelia Huck, Shameerali Kolothum Thodi, Jing Zhang, KVM,
KVMARM, ARMLinux, Oliver Upton, Will Deacon, Paolo Bonzini,
James Morse, Alexandru Elisei, Suzuki K Poulose, Fuad Tabba,
Reiji Watanabe, Raghavendra Rao Ananta
On Tue, May 16, 2023 at 05:44:53PM +0100, Marc Zyngier wrote:
[...]
> More seriously, I'd expect this to be an ARM spec. But it wouldn't
> hurt having a prototype that serves as a draft for the spec. Better
> doing that than leaving it to... someone else.
Completely agree. My suggestion was not meant to discourage prototyping,
just wanted to make sure we have line of sight on making this someone
else's problem to standardize the interface :)
--
Thanks,
Oliver
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v8 5/6] KVM: arm64: Reuse fields of sys_reg_desc for idreg
2023-05-16 10:26 ` Cornelia Huck
@ 2023-05-16 19:10 ` Jing Zhang
0 siblings, 0 replies; 34+ messages in thread
From: Jing Zhang @ 2023-05-16 19:10 UTC (permalink / raw)
To: Cornelia Huck
Cc: KVM, KVMARM, ARMLinux, Marc Zyngier, Oliver Upton, Will Deacon,
Paolo Bonzini, James Morse, Alexandru Elisei, Suzuki K Poulose,
Fuad Tabba, Reiji Watanabe, Raghavendra Rao Ananta
Hi Cornelia,
On Tue, May 16, 2023 at 3:26 AM Cornelia Huck <cohuck@redhat.com> wrote:
>
> On Wed, May 03 2023, Jing Zhang <jingzhangos@google.com> wrote:
>
> > Since reset() and val are not used for idreg in sys_reg_desc, they would
> > be used with other purposes for idregs.
> > The callback reset() would be used to return KVM sanitised id register
> > values. The u64 val would be used as mask for writable fields in idregs.
> > Only bits with 1 in val are writable from userspace.
> >
> > Signed-off-by: Jing Zhang <jingzhangos@google.com>
> > ---
> > arch/arm64/kvm/id_regs.c | 44 +++++++++++++++++++----------
> > arch/arm64/kvm/sys_regs.c | 59 +++++++++++++++++++++++++++------------
> > arch/arm64/kvm/sys_regs.h | 10 ++++---
> > 3 files changed, 77 insertions(+), 36 deletions(-)
> >
>
> (...)
>
> > diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
> > index e88fd77309b2..21869319f6e1 100644
> > --- a/arch/arm64/kvm/sys_regs.h
> > +++ b/arch/arm64/kvm/sys_regs.h
> > @@ -65,12 +65,12 @@ struct sys_reg_desc {
> > const struct sys_reg_desc *);
> >
> > /* Initialization for vcpu. */
>
> Maybe be a bit more verbose here?
>
> /* Initialization for vcpu. Return initialized value, or KVM sanitized
> value for id registers. */
Sure. Thanks.
>
> > - void (*reset)(struct kvm_vcpu *, const struct sys_reg_desc *);
> > + u64 (*reset)(struct kvm_vcpu *, const struct sys_reg_desc *);
> >
> > /* Index into sys_reg[], or 0 if we don't need to save it. */
> > int reg;
> >
> > - /* Value (usually reset value) */
> > + /* Value (usually reset value), or write mask for idregs */
> > u64 val;
> >
> > /* Custom get/set_user functions, fallback to generic if NULL */
>
Jing
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v8 1/6] KVM: arm64: Move CPU ID feature registers emulation into a separate file
2023-05-16 16:11 ` Marc Zyngier
@ 2023-05-16 19:14 ` Jing Zhang
0 siblings, 0 replies; 34+ messages in thread
From: Jing Zhang @ 2023-05-16 19:14 UTC (permalink / raw)
To: Marc Zyngier
Cc: KVM, KVMARM, ARMLinux, Oliver Upton, Will Deacon, Paolo Bonzini,
James Morse, Alexandru Elisei, Suzuki K Poulose, Fuad Tabba,
Reiji Watanabe, Raghavendra Rao Ananta
Hi Marc,
On Tue, May 16, 2023 at 9:11 AM Marc Zyngier <maz@kernel.org> wrote:
>
> On Wed, 03 May 2023 18:16:13 +0100,
> Jing Zhang <jingzhangos@google.com> wrote:
> >
> > Create a new file id_regs.c for CPU ID feature registers emulation code,
> > which are moved from sys_regs.c and tweak sys_regs code accordingly.
> >
> > No functional change intended.
> >
> > Signed-off-by: Jing Zhang <jingzhangos@google.com>
> > ---
> > arch/arm64/kvm/Makefile | 2 +-
> > arch/arm64/kvm/id_regs.c | 460 +++++++++++++++++++++++++++++++++++++
> > arch/arm64/kvm/sys_regs.c | 464 ++++----------------------------------
> > arch/arm64/kvm/sys_regs.h | 19 ++
> > 4 files changed, 519 insertions(+), 426 deletions(-)
> > create mode 100644 arch/arm64/kvm/id_regs.c
> >
> > diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
> > index c0c050e53157..a6a315fcd81e 100644
> > --- a/arch/arm64/kvm/Makefile
> > +++ b/arch/arm64/kvm/Makefile
> > @@ -13,7 +13,7 @@ obj-$(CONFIG_KVM) += hyp/
> > kvm-y += arm.o mmu.o mmio.o psci.o hypercalls.o pvtime.o \
> > inject_fault.o va_layout.o handle_exit.o \
> > guest.o debug.o reset.o sys_regs.o stacktrace.o \
> > - vgic-sys-reg-v3.o fpsimd.o pkvm.o \
> > + vgic-sys-reg-v3.o fpsimd.o pkvm.o id_regs.o \
> > arch_timer.o trng.o vmid.o emulate-nested.o nested.o \
> > vgic/vgic.o vgic/vgic-init.o \
> > vgic/vgic-irqfd.o vgic/vgic-v2.o \
> > diff --git a/arch/arm64/kvm/id_regs.c b/arch/arm64/kvm/id_regs.c
> > new file mode 100644
> > index 000000000000..96b4c43a5100
> > --- /dev/null
> > +++ b/arch/arm64/kvm/id_regs.c
> > @@ -0,0 +1,460 @@
>
> [...]
>
> I really wonder why we move this to another file. It only creates
> noise, and doesn't add much. Yes, the file is big. But now you need to
> expose all the internal machinery that was private until now.
>
> If you look at the global diffstat, this is "only" another 200
> lines. I don't think this is worth the churn.
Yes, the global diff is not that big as in the first patch series now.
Will do all the changes in the same file in the next version.
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
Thanks,
Jing
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v8 2/6] KVM: arm64: Save ID registers' sanitized value per guest
2023-05-03 17:16 ` [PATCH v8 2/6] KVM: arm64: Save ID registers' sanitized value per guest Jing Zhang
@ 2023-05-17 7:41 ` Marc Zyngier
2023-05-17 16:28 ` Jing Zhang
0 siblings, 1 reply; 34+ messages in thread
From: Marc Zyngier @ 2023-05-17 7:41 UTC (permalink / raw)
To: Jing Zhang
Cc: KVM, KVMARM, ARMLinux, Oliver Upton, Will Deacon, Paolo Bonzini,
James Morse, Alexandru Elisei, Suzuki K Poulose, Fuad Tabba,
Reiji Watanabe, Raghavendra Rao Ananta
On Wed, 03 May 2023 18:16:14 +0100,
Jing Zhang <jingzhangos@google.com> wrote:
>
> Introduce id_regs[] in kvm_arch as a storage of guest's ID registers,
> and save ID registers' sanitized value in the array at KVM_CREATE_VM.
> Use the saved ones when ID registers are read by the guest or
> userspace (via KVM_GET_ONE_REG).
>
> No functional change intended.
>
> Co-developed-by: Reiji Watanabe <reijiw@google.com>
> Signed-off-by: Reiji Watanabe <reijiw@google.com>
> Signed-off-by: Jing Zhang <jingzhangos@google.com>
> ---
> arch/arm64/include/asm/kvm_host.h | 20 ++++++++++++++
> arch/arm64/kvm/arm.c | 1 +
> arch/arm64/kvm/id_regs.c | 46 +++++++++++++++++++++++++------
> arch/arm64/kvm/sys_regs.c | 11 +++++++-
> arch/arm64/kvm/sys_regs.h | 3 +-
> 5 files changed, 69 insertions(+), 12 deletions(-)
>
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index bcd774d74f34..a7d4d9e093e3 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -177,6 +177,21 @@ struct kvm_smccc_features {
> unsigned long vendor_hyp_bmap;
> };
>
> +/*
> + * Emulated CPU ID registers per VM
> + * (Op0, Op1, CRn, CRm, Op2) of the ID registers to be saved in it
> + * is (3, 0, 0, crm, op2), where 1<=crm<8, 0<=op2<8.
> + *
> + * These emulated idregs are VM-wide, but accessed from the context of a vCPU.
> + * Access to id regs are guarded by kvm_arch.config_lock.
> + */
> +#define KVM_ARM_ID_REG_NUM 56
You already have this as part of patch #1 in another include file, and
then move it here. Surely you can do that in one go. I'd also like it
to be defined in terms of encodings, and not as a raw value.
> +#define IDREG_IDX(id) (((sys_reg_CRm(id) - 1) << 3) | sys_reg_Op2(id))
> +#define IDREG(kvm, id) kvm->arch.idregs.regs[IDREG_IDX(id)]
Missing brackets around 'kvm'.
> +struct kvm_idregs {
> + u64 regs[KVM_ARM_ID_REG_NUM];
> +};
> +
> typedef unsigned int pkvm_handle_t;
>
> struct kvm_protected_vm {
> @@ -243,6 +258,9 @@ struct kvm_arch {
> /* Hypercall features firmware registers' descriptor */
> struct kvm_smccc_features smccc_feat;
>
> + /* Emulated CPU ID registers */
> + struct kvm_idregs idregs;
> +
> /*
> * For an untrusted host VM, 'pkvm.handle' is used to lookup
> * the associated pKVM instance in the hypervisor.
> @@ -1008,6 +1026,8 @@ int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu,
> long kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm,
> struct kvm_arm_copy_mte_tags *copy_tags);
>
> +void kvm_arm_init_id_regs(struct kvm *kvm);
> +
> /* Guest/host FPSIMD coordination helpers */
> int kvm_arch_vcpu_run_map_fp(struct kvm_vcpu *vcpu);
> void kvm_arch_vcpu_load_fp(struct kvm_vcpu *vcpu);
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 4b2e16e696a8..e34744c36406 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -153,6 +153,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
>
> set_default_spectre(kvm);
> kvm_arm_init_hypercalls(kvm);
> + kvm_arm_init_id_regs(kvm);
>
> /*
> * Initialise the default PMUver before there is a chance to
> diff --git a/arch/arm64/kvm/id_regs.c b/arch/arm64/kvm/id_regs.c
> index 96b4c43a5100..e769223bcee2 100644
> --- a/arch/arm64/kvm/id_regs.c
> +++ b/arch/arm64/kvm/id_regs.c
> @@ -52,16 +52,9 @@ static u8 pmuver_to_perfmon(u8 pmuver)
> }
> }
>
> -/* Read a sanitised cpufeature ID register by sys_reg_desc */
Why getting rid of this comment instead of moving it next to the
(re-implemented) function?
> -static u64 read_id_reg(const struct kvm_vcpu *vcpu, struct sys_reg_desc const *r)
> +u64 kvm_arm_read_id_reg(const struct kvm_vcpu *vcpu, u32 id)
> {
> - u32 id = reg_to_encoding(r);
> - u64 val;
> -
> - if (sysreg_visible_as_raz(vcpu, r))
> - return 0;
> -
> - val = read_sanitised_ftr_reg(id);
> + u64 val = IDREG(vcpu->kvm, id);
>
> switch (id) {
> case SYS_ID_AA64PFR0_EL1:
> @@ -126,6 +119,14 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu, struct sys_reg_desc const *r
> return val;
> }
>
> +static u64 read_id_reg(const struct kvm_vcpu *vcpu, struct sys_reg_desc const *r)
> +{
> + if (sysreg_visible_as_raz(vcpu, r))
> + return 0;
> +
> + return kvm_arm_read_id_reg(vcpu, reg_to_encoding(r));
> +}
> +
> /* cpufeature ID register access trap handlers */
>
> static bool access_id_reg(struct kvm_vcpu *vcpu,
> @@ -458,3 +459,30 @@ int emulate_id_reg(struct kvm_vcpu *vcpu, struct sys_reg_params *params)
>
> return 1;
> }
> +
> +/*
> + * Set the guest's ID registers that are defined in id_reg_descs[]
> + * with ID_SANITISED() to the host's sanitized value.
> + */
> +void kvm_arm_init_id_regs(struct kvm *kvm)
> +{
> + u64 val;
> + u32 id;
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(id_reg_descs); i++) {
> + id = reg_to_encoding(&id_reg_descs[i]);
> +
> + /*
> + * Some hidden ID registers which are not in arm64_ftr_regs[]
> + * would cause warnings from read_sanitised_ftr_reg().
> + * Skip those ID registers to avoid the warnings.
> + */
> + if (id_reg_descs[i].visibility == raz_visibility)
> + /* Hidden or reserved ID register */
> + continue;
Are you sure? What about other visibility attributes that are normally
evaluated at runtime? This may work as a short term hack, but I'm not
sure this is the correct long-term solution...
M.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v8 0/6] Support writable CPU ID registers from userspace
2023-05-16 16:01 ` Marc Zyngier
@ 2023-05-17 15:36 ` Cornelia Huck
2023-05-17 15:53 ` Marc Zyngier
0 siblings, 1 reply; 34+ messages in thread
From: Cornelia Huck @ 2023-05-17 15:36 UTC (permalink / raw)
To: Marc Zyngier
Cc: Shameerali Kolothum Thodi, Jing Zhang, KVM, KVMARM, ARMLinux,
Oliver Upton, Will Deacon, Paolo Bonzini, James Morse,
Alexandru Elisei, Suzuki K Poulose, Fuad Tabba, Reiji Watanabe,
Raghavendra Rao Ananta
On Tue, May 16 2023, Marc Zyngier <maz@kernel.org> wrote:
> On Tue, 16 May 2023 15:19:00 +0100,
> Cornelia Huck <cohuck@redhat.com> wrote:
>>
>> On Tue, May 16 2023, Marc Zyngier <maz@kernel.org> wrote:
>>
>> > On Tue, 16 May 2023 12:55:14 +0100,
>> > Cornelia Huck <cohuck@redhat.com> wrote:
>> >>
>> >> Do you have more concrete ideas for QEMU CPU models already? Asking
>> >> because I wanted to talk about this at KVM Forum, so collecting what
>> >> others would like to do seems like a good idea :)
>> >
>> > I'm not being asked, but I'll share my thoughts anyway! ;-)
>> >
>> > I don't think CPU models are necessarily the most important thing.
>> > Specially when you look at the diversity of the ecosystem (and even
>> > the same CPU can be configured in different ways at integration
>> > time). Case in point, Neoverse N1 which can have its I/D caches made
>> > coherent or not. And the guest really wants to know which one it is
>> > (you can only lie in one direction).
>> >
>> > But being able to control the feature set exposed to the guest from
>> > userspace is a huge benefit in terms of migration.
>>
>> Certainly; the important part is that we can keep the guest ABI
>> stable... which parts match to a "CPU model" in the way other
>> architectures use it is an interesting question. It almost certainly
>> will look different from e.g. s390, where we only have to deal with a
>> single manufacturer.
>>
>> I'm wondering whether we'll end up building frankenmonster CPUs.
>
> We already do. KVM hides a bunch of things we don't want the guest to
> see, either because we don't support the feature, or that we want to
> present it with a different shape (cache topology, for example), and
> these combination don't really exist in any physical implementation.
>
> Which is why I don't really buy the "CPU model" concept as defined by
> x86 and s390. We already are in a vastly different place.
Yes, I agree that the "named cpu models" approach probably won't work on
Arm (especially if you add other accelerators into the mix -- cpu 'foo'
with tcg is unlikely to be 100% identical to cpu 'foo' with KVM.) OTOH,
"these two cpus are not that different from each other, so we support
migration between them with a least common denominator feature/behaviour
set" seems more reasonable.
>
> The way I see it, you get a bunch of architectural features that can
> be enabled/disabled depending on the underlying HW, hypervisor's
> capabilities and userspace input. On top of that, there is a layer of
> paint that tells you what is the overall implementation you could be
> running on (that's what MIDR+REVIDR+AIDR tell you) so that you can
> apply some unspeakable, uarch-specific hacks that keep the machine
> going (got to love these CPU errata).
>
>> Another interesting aspect is how KVM ends up influencing what the guest
>> sees on the CPU level, as in the case where we migrate across matching
>> CPUs, but with a different software level. I think we want userspace to
>> control that to some extent, but I'm not sure if this fully matches the
>> CPU model context.
>
> I'm not sure I get the "different software level" part. Do you mean
> VMM revisions?
Yes. Basically, two (for migration purposes) identical machines with
different kernel/QEMU versions, but using the same QEMU compat
machine. Migrate from old to new, get more regs: works. Migrate from
new to old, get less regs: boom. Expectation would be for this to
work, and handling it from machine compat code seems very awkward.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v8 0/6] Support writable CPU ID registers from userspace
2023-05-17 15:36 ` Cornelia Huck
@ 2023-05-17 15:53 ` Marc Zyngier
0 siblings, 0 replies; 34+ messages in thread
From: Marc Zyngier @ 2023-05-17 15:53 UTC (permalink / raw)
To: Cornelia Huck
Cc: Shameerali Kolothum Thodi, Jing Zhang, KVM, KVMARM, ARMLinux,
Oliver Upton, Will Deacon, Paolo Bonzini, James Morse,
Alexandru Elisei, Suzuki K Poulose, Fuad Tabba, Reiji Watanabe,
Raghavendra Rao Ananta
On Wed, 17 May 2023 16:36:49 +0100,
Cornelia Huck <cohuck@redhat.com> wrote:
>
> On Tue, May 16 2023, Marc Zyngier <maz@kernel.org> wrote:
>
> > On Tue, 16 May 2023 15:19:00 +0100,
> > Cornelia Huck <cohuck@redhat.com> wrote:
> >>
> >> On Tue, May 16 2023, Marc Zyngier <maz@kernel.org> wrote:
> >>
> >> > On Tue, 16 May 2023 12:55:14 +0100,
> >> > Cornelia Huck <cohuck@redhat.com> wrote:
> >> >>
> >> >> Do you have more concrete ideas for QEMU CPU models already? Asking
> >> >> because I wanted to talk about this at KVM Forum, so collecting what
> >> >> others would like to do seems like a good idea :)
> >> >
> >> > I'm not being asked, but I'll share my thoughts anyway! ;-)
> >> >
> >> > I don't think CPU models are necessarily the most important thing.
> >> > Specially when you look at the diversity of the ecosystem (and even
> >> > the same CPU can be configured in different ways at integration
> >> > time). Case in point, Neoverse N1 which can have its I/D caches made
> >> > coherent or not. And the guest really wants to know which one it is
> >> > (you can only lie in one direction).
> >> >
> >> > But being able to control the feature set exposed to the guest from
> >> > userspace is a huge benefit in terms of migration.
> >>
> >> Certainly; the important part is that we can keep the guest ABI
> >> stable... which parts match to a "CPU model" in the way other
> >> architectures use it is an interesting question. It almost certainly
> >> will look different from e.g. s390, where we only have to deal with a
> >> single manufacturer.
> >>
> >> I'm wondering whether we'll end up building frankenmonster CPUs.
> >
> > We already do. KVM hides a bunch of things we don't want the guest to
> > see, either because we don't support the feature, or that we want to
> > present it with a different shape (cache topology, for example), and
> > these combination don't really exist in any physical implementation.
> >
> > Which is why I don't really buy the "CPU model" concept as defined by
> > x86 and s390. We already are in a vastly different place.
>
> Yes, I agree that the "named cpu models" approach probably won't work on
> Arm (especially if you add other accelerators into the mix -- cpu 'foo'
> with tcg is unlikely to be 100% identical to cpu 'foo' with KVM.) OTOH,
> "these two cpus are not that different from each other, so we support
> migration between them with a least common denominator feature/behaviour
> set" seems more reasonable.
It does, assuming the platform is "compatible". I realise I didn't
mention the small issue of the counter frequency, which cannot be
scaled. ARMv8.6 tried to specify a 1GHz counter frequency, but I still
haven't see a single system having adopted it, and everyone is doing
something different.
We *could* always trap/emulate the counters/timers, but that quickly
becomes ridiculously bad. Isn't ARM virtualisation fun?
> > The way I see it, you get a bunch of architectural features that can
> > be enabled/disabled depending on the underlying HW, hypervisor's
> > capabilities and userspace input. On top of that, there is a layer of
> > paint that tells you what is the overall implementation you could be
> > running on (that's what MIDR+REVIDR+AIDR tell you) so that you can
> > apply some unspeakable, uarch-specific hacks that keep the machine
> > going (got to love these CPU errata).
> >
> >> Another interesting aspect is how KVM ends up influencing what the guest
> >> sees on the CPU level, as in the case where we migrate across matching
> >> CPUs, but with a different software level. I think we want userspace to
> >> control that to some extent, but I'm not sure if this fully matches the
> >> CPU model context.
> >
> > I'm not sure I get the "different software level" part. Do you mean
> > VMM revisions?
>
> Yes. Basically, two (for migration purposes) identical machines with
> different kernel/QEMU versions, but using the same QEMU compat
> machine. Migrate from old to new, get more regs: works. Migrate from
> new to old, get less regs: boom. Expectation would be for this to
> work, and handling it from machine compat code seems very awkward.
Old to new, fine. I'm not sure how we make the other way work.
Actually, scratch that. I'm pretty sure we can't make it work. You'd
have constraint your guest at creation time so that it doesn't use any
feature that isn't available to older VMM/kernel revisions.
I sense some interesting discussions next month! :D
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v8 2/6] KVM: arm64: Save ID registers' sanitized value per guest
2023-05-17 7:41 ` Marc Zyngier
@ 2023-05-17 16:28 ` Jing Zhang
0 siblings, 0 replies; 34+ messages in thread
From: Jing Zhang @ 2023-05-17 16:28 UTC (permalink / raw)
To: Marc Zyngier
Cc: KVM, KVMARM, ARMLinux, Oliver Upton, Will Deacon, Paolo Bonzini,
James Morse, Alexandru Elisei, Suzuki K Poulose, Fuad Tabba,
Reiji Watanabe, Raghavendra Rao Ananta
Hi Marc,
On Wed, May 17, 2023 at 12:41 AM Marc Zyngier <maz@kernel.org> wrote:
>
> On Wed, 03 May 2023 18:16:14 +0100,
> Jing Zhang <jingzhangos@google.com> wrote:
> >
> > Introduce id_regs[] in kvm_arch as a storage of guest's ID registers,
> > and save ID registers' sanitized value in the array at KVM_CREATE_VM.
> > Use the saved ones when ID registers are read by the guest or
> > userspace (via KVM_GET_ONE_REG).
> >
> > No functional change intended.
> >
> > Co-developed-by: Reiji Watanabe <reijiw@google.com>
> > Signed-off-by: Reiji Watanabe <reijiw@google.com>
> > Signed-off-by: Jing Zhang <jingzhangos@google.com>
> > ---
> > arch/arm64/include/asm/kvm_host.h | 20 ++++++++++++++
> > arch/arm64/kvm/arm.c | 1 +
> > arch/arm64/kvm/id_regs.c | 46 +++++++++++++++++++++++++------
> > arch/arm64/kvm/sys_regs.c | 11 +++++++-
> > arch/arm64/kvm/sys_regs.h | 3 +-
> > 5 files changed, 69 insertions(+), 12 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index bcd774d74f34..a7d4d9e093e3 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -177,6 +177,21 @@ struct kvm_smccc_features {
> > unsigned long vendor_hyp_bmap;
> > };
> >
> > +/*
> > + * Emulated CPU ID registers per VM
> > + * (Op0, Op1, CRn, CRm, Op2) of the ID registers to be saved in it
> > + * is (3, 0, 0, crm, op2), where 1<=crm<8, 0<=op2<8.
> > + *
> > + * These emulated idregs are VM-wide, but accessed from the context of a vCPU.
> > + * Access to id regs are guarded by kvm_arch.config_lock.
> > + */
> > +#define KVM_ARM_ID_REG_NUM 56
>
> You already have this as part of patch #1 in another include file, and
> then move it here. Surely you can do that in one go. I'd also like it
> to be defined in terms of encodings, and not as a raw value.
Sure, will do.
>
> > +#define IDREG_IDX(id) (((sys_reg_CRm(id) - 1) << 3) | sys_reg_Op2(id))
> > +#define IDREG(kvm, id) kvm->arch.idregs.regs[IDREG_IDX(id)]
>
> Missing brackets around 'kvm'.
Thanks, will fix.
>
> > +struct kvm_idregs {
> > + u64 regs[KVM_ARM_ID_REG_NUM];
> > +};
> > +
> > typedef unsigned int pkvm_handle_t;
> >
> > struct kvm_protected_vm {
> > @@ -243,6 +258,9 @@ struct kvm_arch {
> > /* Hypercall features firmware registers' descriptor */
> > struct kvm_smccc_features smccc_feat;
> >
> > + /* Emulated CPU ID registers */
> > + struct kvm_idregs idregs;
> > +
> > /*
> > * For an untrusted host VM, 'pkvm.handle' is used to lookup
> > * the associated pKVM instance in the hypervisor.
> > @@ -1008,6 +1026,8 @@ int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu,
> > long kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm,
> > struct kvm_arm_copy_mte_tags *copy_tags);
> >
> > +void kvm_arm_init_id_regs(struct kvm *kvm);
> > +
> > /* Guest/host FPSIMD coordination helpers */
> > int kvm_arch_vcpu_run_map_fp(struct kvm_vcpu *vcpu);
> > void kvm_arch_vcpu_load_fp(struct kvm_vcpu *vcpu);
> > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> > index 4b2e16e696a8..e34744c36406 100644
> > --- a/arch/arm64/kvm/arm.c
> > +++ b/arch/arm64/kvm/arm.c
> > @@ -153,6 +153,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
> >
> > set_default_spectre(kvm);
> > kvm_arm_init_hypercalls(kvm);
> > + kvm_arm_init_id_regs(kvm);
> >
> > /*
> > * Initialise the default PMUver before there is a chance to
> > diff --git a/arch/arm64/kvm/id_regs.c b/arch/arm64/kvm/id_regs.c
> > index 96b4c43a5100..e769223bcee2 100644
> > --- a/arch/arm64/kvm/id_regs.c
> > +++ b/arch/arm64/kvm/id_regs.c
> > @@ -52,16 +52,9 @@ static u8 pmuver_to_perfmon(u8 pmuver)
> > }
> > }
> >
> > -/* Read a sanitised cpufeature ID register by sys_reg_desc */
>
> Why getting rid of this comment instead of moving it next to the
> (re-implemented) function?
>
Right, will move it to the re-implemented function.
> > -static u64 read_id_reg(const struct kvm_vcpu *vcpu, struct sys_reg_desc const *r)
> > +u64 kvm_arm_read_id_reg(const struct kvm_vcpu *vcpu, u32 id)
> > {
> > - u32 id = reg_to_encoding(r);
> > - u64 val;
> > -
> > - if (sysreg_visible_as_raz(vcpu, r))
> > - return 0;
> > -
> > - val = read_sanitised_ftr_reg(id);
> > + u64 val = IDREG(vcpu->kvm, id);
> >
> > switch (id) {
> > case SYS_ID_AA64PFR0_EL1:
> > @@ -126,6 +119,14 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu, struct sys_reg_desc const *r
> > return val;
> > }
> >
> > +static u64 read_id_reg(const struct kvm_vcpu *vcpu, struct sys_reg_desc const *r)
> > +{
> > + if (sysreg_visible_as_raz(vcpu, r))
> > + return 0;
> > +
> > + return kvm_arm_read_id_reg(vcpu, reg_to_encoding(r));
> > +}
> > +
> > /* cpufeature ID register access trap handlers */
> >
> > static bool access_id_reg(struct kvm_vcpu *vcpu,
> > @@ -458,3 +459,30 @@ int emulate_id_reg(struct kvm_vcpu *vcpu, struct sys_reg_params *params)
> >
> > return 1;
> > }
> > +
> > +/*
> > + * Set the guest's ID registers that are defined in id_reg_descs[]
> > + * with ID_SANITISED() to the host's sanitized value.
> > + */
> > +void kvm_arm_init_id_regs(struct kvm *kvm)
> > +{
> > + u64 val;
> > + u32 id;
> > + int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(id_reg_descs); i++) {
> > + id = reg_to_encoding(&id_reg_descs[i]);
> > +
> > + /*
> > + * Some hidden ID registers which are not in arm64_ftr_regs[]
> > + * would cause warnings from read_sanitised_ftr_reg().
> > + * Skip those ID registers to avoid the warnings.
> > + */
> > + if (id_reg_descs[i].visibility == raz_visibility)
> > + /* Hidden or reserved ID register */
> > + continue;
>
> Are you sure? What about other visibility attributes that are normally
> evaluated at runtime? This may work as a short term hack, but I'm not
> sure this is the correct long-term solution...
Yes, this is a short term hack. It would be replaced by checking the
reset() function of idregs in patch #5 in this series.
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
Thanks,
Jing
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v8 6/6] KVM: arm64: Refactor writings for PMUVer/CSV2/CSV3
2023-05-03 17:16 ` [PATCH v8 6/6] KVM: arm64: Refactor writings for PMUVer/CSV2/CSV3 Jing Zhang
@ 2023-05-17 22:00 ` Jitindar Singh, Suraj
2023-05-17 22:55 ` Jing Zhang
2023-05-19 23:25 ` Suraj Jitindar Singh
1 sibling, 1 reply; 34+ messages in thread
From: Jitindar Singh, Suraj @ 2023-05-17 22:00 UTC (permalink / raw)
To: jingzhangos, kvm, kvmarm, linux-arm-kernel, maz, oupton
Cc: james.morse, suzuki.poulose, rananta, tabba, pbonzini,
alexandru.elisei, will, reijiw
On Wed, 2023-05-03 at 17:16 +0000, Jing Zhang wrote:
> Refactor writings for ID_AA64PFR0_EL1.[CSV2|CSV3],
> ID_AA64DFR0_EL1.PMUVer and ID_DFR0_ELF.PerfMon based on utilities
> introduced by ID register descriptor array.
>
> Signed-off-by: Jing Zhang <jingzhangos@google.com>
> ---
> arch/arm64/include/asm/cpufeature.h | 1 +
> arch/arm64/kernel/cpufeature.c | 2 +-
> arch/arm64/kvm/id_regs.c | 361 ++++++++++++++++++--------
> --
> 3 files changed, 242 insertions(+), 122 deletions(-)
>
>
[ SNIP ]
>
> +static u64 read_sanitised_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
> + const struct sys_reg_desc
> *rd)
> +{
> + u64 val;
> + u32 id = reg_to_encoding(rd);
> +
> + val = read_sanitised_ftr_reg(id);
> + /*
> + * The default is to expose CSV2 == 1 if the HW isn't
> affected.
> + * Although this is a per-CPU feature, we make it global
> because
> + * asymmetric systems are just a nuisance.
> + *
> + * Userspace can override this as long as it doesn't promise
> + * the impossible.
> + */
> + if (arm64_get_spectre_v2_state() == SPECTRE_UNAFFECTED) {
> + val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV2);
> + val |=
> FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV2), 1);
> + }
> + if (arm64_get_meltdown_state() == SPECTRE_UNAFFECTED) {
> + val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV3);
> + val |=
> FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV3), 1);
> + }
> +
> + val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_AMU);
> +
> + return val;
> +}
> +
> static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
> const struct sys_reg_desc *rd,
> u64 val)
> {
> - struct kvm_arch *arch = &vcpu->kvm->arch;
> - u64 sval = val;
> u8 csv2, csv3;
> - int ret = 0;
>
> /*
> * Allow AA64PFR0_EL1.CSV2 to be set from userspace as long
> as
> @@ -226,26 +338,30 @@ static int set_id_aa64pfr0_el1(struct kvm_vcpu
> *vcpu,
> if (csv3 > 1 || (csv3 && arm64_get_meltdown_state() !=
> SPECTRE_UNAFFECTED))
> return -EINVAL;
Can't we remove the checking of csv[23] here as it will be checked by
arm64_check_features()?
i.e. in arm64_check_features() we will load the "limit" value from the
"reset" function (read_sanitised_id_aa64pfr0_el1()) which has csv[23]
set appropriately and limit it to a safe value basically performing the
same check as we are here.
>
> - mutex_lock(&arch->config_lock);
> - /* We can only differ with CSV[23], and anything else is an
> error */
> - val ^= read_id_reg(vcpu, rd);
> - val &= ~(ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV2) |
> - ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV3));
> - if (val) {
> - ret = -EINVAL;
> - goto out;
> - }
> + return set_id_reg(vcpu, rd, val);
> +}
>
> - /* Only allow userspace to change the idregs before VM
> running */
> - if (test_bit(KVM_ARCH_FLAG_HAS_RAN_ONCE, &vcpu->kvm-
> >arch.flags)) {
> - if (sval != read_id_reg(vcpu, rd))
> - ret = -EBUSY;
> - } else {
> - IDREG(vcpu->kvm, reg_to_encoding(rd)) = sval;
> - }
> -out:
> - mutex_unlock(&arch->config_lock);
> - return ret;
> +static u64 read_sanitised_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
> + const struct sys_reg_desc
> *rd)
> +{
> + u64 val;
> + u32 id = reg_to_encoding(rd);
> +
> + val = read_sanitised_ftr_reg(id);
> + /* Limit debug to ARMv8.0 */
> + val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_DebugVer);
> + val |=
> FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_DebugVer), 6);
> + /*
> + * Initialise the default PMUver before there is a chance to
> + * create an actual PMU.
> + */
> + val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer);
> + val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer),
> + kvm_arm_pmu_get_pmuver_limit());
> + /* Hide SPE from guests */
> + val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMSVer);
> +
> + return val;
> }
>
> static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
> @@ -255,7 +371,6 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu
> *vcpu,
> struct kvm_arch *arch = &vcpu->kvm->arch;
> u8 pmuver, host_pmuver;
> bool valid_pmu;
> - u64 sval = val;
> int ret = 0;
>
> host_pmuver = kvm_arm_pmu_get_pmuver_limit();
> @@ -277,40 +392,61 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu
> *vcpu,
> return -EINVAL;
>
> mutex_lock(&arch->config_lock);
> - /* We can only differ with PMUver, and anything else is an
> error */
> - val ^= read_id_reg(vcpu, rd);
> - val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer);
> - if (val) {
> - ret = -EINVAL;
> - goto out;
> - }
> -
> /* Only allow userspace to change the idregs before VM
> running */
> if (test_bit(KVM_ARCH_FLAG_HAS_RAN_ONCE, &vcpu->kvm-
> >arch.flags)) {
> - if (sval != read_id_reg(vcpu, rd))
> + if (val != read_id_reg(vcpu, rd))
> ret = -EBUSY;
> - } else {
> - if (valid_pmu) {
> - val = IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1);
> - val &= ~ID_AA64DFR0_EL1_PMUVer_MASK;
> - val |=
> FIELD_PREP(ID_AA64DFR0_EL1_PMUVer_MASK, pmuver);
> - IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1) = val;
> -
> - val = IDREG(vcpu->kvm, SYS_ID_DFR0_EL1);
> - val &= ~ID_DFR0_EL1_PerfMon_MASK;
> - val |= FIELD_PREP(ID_DFR0_EL1_PerfMon_MASK,
> pmuver_to_perfmon(pmuver));
> - IDREG(vcpu->kvm, SYS_ID_DFR0_EL1) = val;
> - } else {
> -
> assign_bit(KVM_ARCH_FLAG_VCPU_HAS_IMP_DEF_PMU,
> &vcpu->kvm->arch.flags,
> - pmuver ==
> ID_AA64DFR0_EL1_PMUVer_IMP_DEF);
> - }
> + goto out;
> + }
> +
> + if (!valid_pmu) {
> + /*
> + * Ignore the PMUVer filed in @val. The PMUVer would
Nit s/filed/field
> be determined
> + * by arch flags bit
> KVM_ARCH_FLAG_VCPU_HAS_IMP_DEF_PMU,
> + */
> + pmuver = FIELD_GET(ID_AA64DFR0_EL1_PMUVer_MASK,
> + IDREG(vcpu->kvm,
> SYS_ID_AA64DFR0_EL1));
> + val &= ~ID_AA64DFR0_EL1_PMUVer_MASK;
> + val |= FIELD_PREP(ID_AA64DFR0_EL1_PMUVer_MASK,
> pmuver);
> }
>
> + ret = arm64_check_features(vcpu, rd, val);
> + if (ret)
> + goto out;
> +
> + IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1) = val;
> +
> + val = IDREG(vcpu->kvm, SYS_ID_DFR0_EL1);
> + val &= ~ID_DFR0_EL1_PerfMon_MASK;
> + val |= FIELD_PREP(ID_DFR0_EL1_PerfMon_MASK,
> pmuver_to_perfmon(pmuver));
> + IDREG(vcpu->kvm, SYS_ID_DFR0_EL1) = val;
> +
> + if (!valid_pmu)
> + assign_bit(KVM_ARCH_FLAG_VCPU_HAS_IMP_DEF_PMU, &vcpu-
> >kvm->arch.flags,
> + pmuver == ID_AA64DFR0_EL1_PMUVer_IMP_DEF);
> +
> out:
> mutex_unlock(&arch->config_lock);
> return ret;
> }
>
> +static u64 read_sanitised_id_dfr0_el1(struct kvm_vcpu *vcpu,
> + const struct sys_reg_desc *rd)
> +{
> + u64 val;
> + u32 id = reg_to_encoding(rd);
> +
> + val = read_sanitised_ftr_reg(id);
> + /*
> + * Initialise the default PMUver before there is a chance to
> + * create an actual PMU.
> + */
> + val &= ~ARM64_FEATURE_MASK(ID_DFR0_EL1_PerfMon);
> + val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_DFR0_EL1_PerfMon),
> kvm_arm_pmu_get_pmuver_limit());
> +
> + return val;
> +}
> +
> static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
> const struct sys_reg_desc *rd,
> u64 val)
> @@ -318,7 +454,6 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
> struct kvm_arch *arch = &vcpu->kvm->arch;
> u8 perfmon, host_perfmon;
> bool valid_pmu;
> - u64 sval = val;
> int ret = 0;
>
> host_perfmon =
> pmuver_to_perfmon(kvm_arm_pmu_get_pmuver_limit());
> @@ -341,35 +476,39 @@ static int set_id_dfr0_el1(struct kvm_vcpu
> *vcpu,
> return -EINVAL;
>
> mutex_lock(&arch->config_lock);
> - /* We can only differ with PerfMon, and anything else is an
> error */
> - val ^= read_id_reg(vcpu, rd);
> - val &= ~ARM64_FEATURE_MASK(ID_DFR0_EL1_PerfMon);
> - if (val) {
> - ret = -EINVAL;
> - goto out;
> - }
> -
> /* Only allow userspace to change the idregs before VM
> running */
> if (test_bit(KVM_ARCH_FLAG_HAS_RAN_ONCE, &vcpu->kvm-
> >arch.flags)) {
> - if (sval != read_id_reg(vcpu, rd))
> + if (val != read_id_reg(vcpu, rd))
> ret = -EBUSY;
> - } else {
> - if (valid_pmu) {
> - val = IDREG(vcpu->kvm, SYS_ID_DFR0_EL1);
> - val &= ~ID_DFR0_EL1_PerfMon_MASK;
> - val |= FIELD_PREP(ID_DFR0_EL1_PerfMon_MASK,
> perfmon);
> - IDREG(vcpu->kvm, SYS_ID_DFR0_EL1) = val;
> -
> - val = IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1);
> - val &= ~ID_AA64DFR0_EL1_PMUVer_MASK;
> - val |=
> FIELD_PREP(ID_AA64DFR0_EL1_PMUVer_MASK, perfmon_to_pmuver(perfmon));
> - IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1) = val;
> - } else {
> -
> assign_bit(KVM_ARCH_FLAG_VCPU_HAS_IMP_DEF_PMU,
> &vcpu->kvm->arch.flags,
> - perfmon ==
> ID_DFR0_EL1_PerfMon_IMPDEF);
> - }
> + goto out;
> + }
> +
> + if (!valid_pmu) {
> + /*
> + * Ignore the PerfMon filed in @val. The PerfMon
Nit s/filed/field
> would be determined
> + * by arch flags bit
> KVM_ARCH_FLAG_VCPU_HAS_IMP_DEF_PMU,
> + */
> + perfmon = FIELD_GET(ID_DFR0_EL1_PerfMon_MASK,
> + IDREG(vcpu->kvm,
> SYS_ID_DFR0_EL1));
> + val &= ~ID_DFR0_EL1_PerfMon_MASK;
> + val |= FIELD_PREP(ID_DFR0_EL1_PerfMon_MASK, perfmon);
> }
>
> + ret = arm64_check_features(vcpu, rd, val);
> + if (ret)
> + goto out;
> +
> + IDREG(vcpu->kvm, SYS_ID_DFR0_EL1) = val;
> +
> + val = IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1);
> + val &= ~ID_AA64DFR0_EL1_PMUVer_MASK;
> + val |= FIELD_PREP(ID_AA64DFR0_EL1_PMUVer_MASK,
> perfmon_to_pmuver(perfmon));
> + IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1) = val;
> +
> + if (!valid_pmu)
> + assign_bit(KVM_ARCH_FLAG_VCPU_HAS_IMP_DEF_PMU, &vcpu-
> >kvm->arch.flags,
> + perfmon == ID_DFR0_EL1_PerfMon_IMPDEF);
> +
> out:
> mutex_unlock(&arch->config_lock);
> return ret;
Otherwise looks good!
Thanks,
Suraj
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v8 6/6] KVM: arm64: Refactor writings for PMUVer/CSV2/CSV3
2023-05-17 22:00 ` Jitindar Singh, Suraj
@ 2023-05-17 22:55 ` Jing Zhang
2023-05-18 21:08 ` Jitindar Singh, Suraj
0 siblings, 1 reply; 34+ messages in thread
From: Jing Zhang @ 2023-05-17 22:55 UTC (permalink / raw)
To: Jitindar Singh, Suraj
Cc: kvm, kvmarm, linux-arm-kernel, maz, oupton, james.morse,
suzuki.poulose, rananta, tabba, pbonzini, alexandru.elisei, will,
reijiw
Hi Suraj,
On Wed, May 17, 2023 at 3:00 PM Jitindar Singh, Suraj
<surajjs@amazon.com> wrote:
>
> On Wed, 2023-05-03 at 17:16 +0000, Jing Zhang wrote:
> > Refactor writings for ID_AA64PFR0_EL1.[CSV2|CSV3],
> > ID_AA64DFR0_EL1.PMUVer and ID_DFR0_ELF.PerfMon based on utilities
> > introduced by ID register descriptor array.
> >
> > Signed-off-by: Jing Zhang <jingzhangos@google.com>
> > ---
> > arch/arm64/include/asm/cpufeature.h | 1 +
> > arch/arm64/kernel/cpufeature.c | 2 +-
> > arch/arm64/kvm/id_regs.c | 361 ++++++++++++++++++--------
> > --
> > 3 files changed, 242 insertions(+), 122 deletions(-)
> >
> >
>
> [ SNIP ]
>
> >
> > +static u64 read_sanitised_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
> > + const struct sys_reg_desc
> > *rd)
> > +{
> > + u64 val;
> > + u32 id = reg_to_encoding(rd);
> > +
> > + val = read_sanitised_ftr_reg(id);
> > + /*
> > + * The default is to expose CSV2 == 1 if the HW isn't
> > affected.
> > + * Although this is a per-CPU feature, we make it global
> > because
> > + * asymmetric systems are just a nuisance.
> > + *
> > + * Userspace can override this as long as it doesn't promise
> > + * the impossible.
> > + */
> > + if (arm64_get_spectre_v2_state() == SPECTRE_UNAFFECTED) {
> > + val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV2);
> > + val |=
> > FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV2), 1);
> > + }
> > + if (arm64_get_meltdown_state() == SPECTRE_UNAFFECTED) {
> > + val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV3);
> > + val |=
> > FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV3), 1);
> > + }
> > +
> > + val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_AMU);
> > +
> > + return val;
> > +}
> > +
> > static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
> > const struct sys_reg_desc *rd,
> > u64 val)
> > {
> > - struct kvm_arch *arch = &vcpu->kvm->arch;
> > - u64 sval = val;
> > u8 csv2, csv3;
> > - int ret = 0;
> >
> > /*
> > * Allow AA64PFR0_EL1.CSV2 to be set from userspace as long
> > as
> > @@ -226,26 +338,30 @@ static int set_id_aa64pfr0_el1(struct kvm_vcpu
> > *vcpu,
> > if (csv3 > 1 || (csv3 && arm64_get_meltdown_state() !=
> > SPECTRE_UNAFFECTED))
> > return -EINVAL;
>
> Can't we remove the checking of csv[23] here as it will be checked by
> arm64_check_features()?
>
> i.e. in arm64_check_features() we will load the "limit" value from the
> "reset" function (read_sanitised_id_aa64pfr0_el1()) which has csv[23]
> set appropriately and limit it to a safe value basically performing the
> same check as we are here.
The limit and the check here might be different if like
arm64_get_meltdown_state() is not SPECTRE_UNAFFECTED.
i.e. if we remove the check here, theoretically, the csv3 can be set a
value greater 1 if arm64_get_meltdown_state() is not
SPECTRE_UNAFFECTED.
>
> >
> > - mutex_lock(&arch->config_lock);
> > - /* We can only differ with CSV[23], and anything else is an
> > error */
> > - val ^= read_id_reg(vcpu, rd);
> > - val &= ~(ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV2) |
> > - ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV3));
> > - if (val) {
> > - ret = -EINVAL;
> > - goto out;
> > - }
> > + return set_id_reg(vcpu, rd, val);
> > +}
> >
> > - /* Only allow userspace to change the idregs before VM
> > running */
> > - if (test_bit(KVM_ARCH_FLAG_HAS_RAN_ONCE, &vcpu->kvm-
> > >arch.flags)) {
> > - if (sval != read_id_reg(vcpu, rd))
> > - ret = -EBUSY;
> > - } else {
> > - IDREG(vcpu->kvm, reg_to_encoding(rd)) = sval;
> > - }
> > -out:
> > - mutex_unlock(&arch->config_lock);
> > - return ret;
> > +static u64 read_sanitised_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
> > + const struct sys_reg_desc
> > *rd)
> > +{
> > + u64 val;
> > + u32 id = reg_to_encoding(rd);
> > +
> > + val = read_sanitised_ftr_reg(id);
> > + /* Limit debug to ARMv8.0 */
> > + val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_DebugVer);
> > + val |=
> > FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_DebugVer), 6);
> > + /*
> > + * Initialise the default PMUver before there is a chance to
> > + * create an actual PMU.
> > + */
> > + val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer);
> > + val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer),
> > + kvm_arm_pmu_get_pmuver_limit());
> > + /* Hide SPE from guests */
> > + val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMSVer);
> > +
> > + return val;
> > }
> >
> > static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
> > @@ -255,7 +371,6 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu
> > *vcpu,
> > struct kvm_arch *arch = &vcpu->kvm->arch;
> > u8 pmuver, host_pmuver;
> > bool valid_pmu;
> > - u64 sval = val;
> > int ret = 0;
> >
> > host_pmuver = kvm_arm_pmu_get_pmuver_limit();
> > @@ -277,40 +392,61 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu
> > *vcpu,
> > return -EINVAL;
> >
> > mutex_lock(&arch->config_lock);
> > - /* We can only differ with PMUver, and anything else is an
> > error */
> > - val ^= read_id_reg(vcpu, rd);
> > - val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer);
> > - if (val) {
> > - ret = -EINVAL;
> > - goto out;
> > - }
> > -
> > /* Only allow userspace to change the idregs before VM
> > running */
> > if (test_bit(KVM_ARCH_FLAG_HAS_RAN_ONCE, &vcpu->kvm-
> > >arch.flags)) {
> > - if (sval != read_id_reg(vcpu, rd))
> > + if (val != read_id_reg(vcpu, rd))
> > ret = -EBUSY;
> > - } else {
> > - if (valid_pmu) {
> > - val = IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1);
> > - val &= ~ID_AA64DFR0_EL1_PMUVer_MASK;
> > - val |=
> > FIELD_PREP(ID_AA64DFR0_EL1_PMUVer_MASK, pmuver);
> > - IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1) = val;
> > -
> > - val = IDREG(vcpu->kvm, SYS_ID_DFR0_EL1);
> > - val &= ~ID_DFR0_EL1_PerfMon_MASK;
> > - val |= FIELD_PREP(ID_DFR0_EL1_PerfMon_MASK,
> > pmuver_to_perfmon(pmuver));
> > - IDREG(vcpu->kvm, SYS_ID_DFR0_EL1) = val;
> > - } else {
> > -
> > assign_bit(KVM_ARCH_FLAG_VCPU_HAS_IMP_DEF_PMU,
> > &vcpu->kvm->arch.flags,
> > - pmuver ==
> > ID_AA64DFR0_EL1_PMUVer_IMP_DEF);
> > - }
> > + goto out;
> > + }
> > +
> > + if (!valid_pmu) {
> > + /*
> > + * Ignore the PMUVer filed in @val. The PMUVer would
>
> Nit s/filed/field
Will fix.
>
> > be determined
> > + * by arch flags bit
> > KVM_ARCH_FLAG_VCPU_HAS_IMP_DEF_PMU,
> > + */
> > + pmuver = FIELD_GET(ID_AA64DFR0_EL1_PMUVer_MASK,
> > + IDREG(vcpu->kvm,
> > SYS_ID_AA64DFR0_EL1));
> > + val &= ~ID_AA64DFR0_EL1_PMUVer_MASK;
> > + val |= FIELD_PREP(ID_AA64DFR0_EL1_PMUVer_MASK,
> > pmuver);
> > }
> >
> > + ret = arm64_check_features(vcpu, rd, val);
> > + if (ret)
> > + goto out;
> > +
> > + IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1) = val;
> > +
> > + val = IDREG(vcpu->kvm, SYS_ID_DFR0_EL1);
> > + val &= ~ID_DFR0_EL1_PerfMon_MASK;
> > + val |= FIELD_PREP(ID_DFR0_EL1_PerfMon_MASK,
> > pmuver_to_perfmon(pmuver));
> > + IDREG(vcpu->kvm, SYS_ID_DFR0_EL1) = val;
> > +
> > + if (!valid_pmu)
> > + assign_bit(KVM_ARCH_FLAG_VCPU_HAS_IMP_DEF_PMU, &vcpu-
> > >kvm->arch.flags,
> > + pmuver == ID_AA64DFR0_EL1_PMUVer_IMP_DEF);
> > +
> > out:
> > mutex_unlock(&arch->config_lock);
> > return ret;
> > }
> >
> > +static u64 read_sanitised_id_dfr0_el1(struct kvm_vcpu *vcpu,
> > + const struct sys_reg_desc *rd)
> > +{
> > + u64 val;
> > + u32 id = reg_to_encoding(rd);
> > +
> > + val = read_sanitised_ftr_reg(id);
> > + /*
> > + * Initialise the default PMUver before there is a chance to
> > + * create an actual PMU.
> > + */
> > + val &= ~ARM64_FEATURE_MASK(ID_DFR0_EL1_PerfMon);
> > + val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_DFR0_EL1_PerfMon),
> > kvm_arm_pmu_get_pmuver_limit());
> > +
> > + return val;
> > +}
> > +
> > static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
> > const struct sys_reg_desc *rd,
> > u64 val)
> > @@ -318,7 +454,6 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
> > struct kvm_arch *arch = &vcpu->kvm->arch;
> > u8 perfmon, host_perfmon;
> > bool valid_pmu;
> > - u64 sval = val;
> > int ret = 0;
> >
> > host_perfmon =
> > pmuver_to_perfmon(kvm_arm_pmu_get_pmuver_limit());
> > @@ -341,35 +476,39 @@ static int set_id_dfr0_el1(struct kvm_vcpu
> > *vcpu,
> > return -EINVAL;
> >
> > mutex_lock(&arch->config_lock);
> > - /* We can only differ with PerfMon, and anything else is an
> > error */
> > - val ^= read_id_reg(vcpu, rd);
> > - val &= ~ARM64_FEATURE_MASK(ID_DFR0_EL1_PerfMon);
> > - if (val) {
> > - ret = -EINVAL;
> > - goto out;
> > - }
> > -
> > /* Only allow userspace to change the idregs before VM
> > running */
> > if (test_bit(KVM_ARCH_FLAG_HAS_RAN_ONCE, &vcpu->kvm-
> > >arch.flags)) {
> > - if (sval != read_id_reg(vcpu, rd))
> > + if (val != read_id_reg(vcpu, rd))
> > ret = -EBUSY;
> > - } else {
> > - if (valid_pmu) {
> > - val = IDREG(vcpu->kvm, SYS_ID_DFR0_EL1);
> > - val &= ~ID_DFR0_EL1_PerfMon_MASK;
> > - val |= FIELD_PREP(ID_DFR0_EL1_PerfMon_MASK,
> > perfmon);
> > - IDREG(vcpu->kvm, SYS_ID_DFR0_EL1) = val;
> > -
> > - val = IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1);
> > - val &= ~ID_AA64DFR0_EL1_PMUVer_MASK;
> > - val |=
> > FIELD_PREP(ID_AA64DFR0_EL1_PMUVer_MASK, perfmon_to_pmuver(perfmon));
> > - IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1) = val;
> > - } else {
> > -
> > assign_bit(KVM_ARCH_FLAG_VCPU_HAS_IMP_DEF_PMU,
> > &vcpu->kvm->arch.flags,
> > - perfmon ==
> > ID_DFR0_EL1_PerfMon_IMPDEF);
> > - }
> > + goto out;
> > + }
> > +
> > + if (!valid_pmu) {
> > + /*
> > + * Ignore the PerfMon filed in @val. The PerfMon
>
> Nit s/filed/field
Thanks, will fix it.
>
> > would be determined
> > + * by arch flags bit
> > KVM_ARCH_FLAG_VCPU_HAS_IMP_DEF_PMU,
> > + */
> > + perfmon = FIELD_GET(ID_DFR0_EL1_PerfMon_MASK,
> > + IDREG(vcpu->kvm,
> > SYS_ID_DFR0_EL1));
> > + val &= ~ID_DFR0_EL1_PerfMon_MASK;
> > + val |= FIELD_PREP(ID_DFR0_EL1_PerfMon_MASK, perfmon);
> > }
> >
> > + ret = arm64_check_features(vcpu, rd, val);
> > + if (ret)
> > + goto out;
> > +
> > + IDREG(vcpu->kvm, SYS_ID_DFR0_EL1) = val;
> > +
> > + val = IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1);
> > + val &= ~ID_AA64DFR0_EL1_PMUVer_MASK;
> > + val |= FIELD_PREP(ID_AA64DFR0_EL1_PMUVer_MASK,
> > perfmon_to_pmuver(perfmon));
> > + IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1) = val;
> > +
> > + if (!valid_pmu)
> > + assign_bit(KVM_ARCH_FLAG_VCPU_HAS_IMP_DEF_PMU, &vcpu-
> > >kvm->arch.flags,
> > + perfmon == ID_DFR0_EL1_PerfMon_IMPDEF);
> > +
> > out:
> > mutex_unlock(&arch->config_lock);
> > return ret;
>
> Otherwise looks good!
>
> Thanks,
> Suraj
Thanks,
Jing
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v8 6/6] KVM: arm64: Refactor writings for PMUVer/CSV2/CSV3
2023-05-17 22:55 ` Jing Zhang
@ 2023-05-18 21:08 ` Jitindar Singh, Suraj
2023-05-19 9:16 ` Marc Zyngier
0 siblings, 1 reply; 34+ messages in thread
From: Jitindar Singh, Suraj @ 2023-05-18 21:08 UTC (permalink / raw)
To: jingzhangos
Cc: pbonzini, reijiw, james.morse, suzuki.poulose, kvmarm, rananta,
linux-arm-kernel, kvm, tabba, maz, oupton, alexandru.elisei,
will
On Wed, 2023-05-17 at 15:55 -0700, Jing Zhang wrote:
> CAUTION: This email originated from outside of the organization. Do
> not click links or open attachments unless you can confirm the sender
> and know the content is safe.
>
>
>
> Hi Suraj,
>
> On Wed, May 17, 2023 at 3:00 PM Jitindar Singh, Suraj
> <surajjs@amazon.com> wrote:
> >
> > On Wed, 2023-05-03 at 17:16 +0000, Jing Zhang wrote:
> > > Refactor writings for ID_AA64PFR0_EL1.[CSV2|CSV3],
> > > ID_AA64DFR0_EL1.PMUVer and ID_DFR0_ELF.PerfMon based on utilities
> > > introduced by ID register descriptor array.
> > >
> > > Signed-off-by: Jing Zhang <jingzhangos@google.com>
> > > ---
> > > arch/arm64/include/asm/cpufeature.h | 1 +
> > > arch/arm64/kernel/cpufeature.c | 2 +-
> > > arch/arm64/kvm/id_regs.c | 361 ++++++++++++++++++----
> > > ----
> > > --
> > > 3 files changed, 242 insertions(+), 122 deletions(-)
> > >
> > >
> >
> > [ SNIP ]
> >
> > >
> > > +static u64 read_sanitised_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
> > > + const struct
> > > sys_reg_desc
> > > *rd)
> > > +{
> > > + u64 val;
> > > + u32 id = reg_to_encoding(rd);
> > > +
> > > + val = read_sanitised_ftr_reg(id);
> > > + /*
> > > + * The default is to expose CSV2 == 1 if the HW isn't
> > > affected.
> > > + * Although this is a per-CPU feature, we make it global
> > > because
> > > + * asymmetric systems are just a nuisance.
> > > + *
> > > + * Userspace can override this as long as it doesn't
> > > promise
> > > + * the impossible.
> > > + */
> > > + if (arm64_get_spectre_v2_state() == SPECTRE_UNAFFECTED) {
> > > + val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV2);
> > > + val |=
> > > FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV2), 1);
> > > + }
> > > + if (arm64_get_meltdown_state() == SPECTRE_UNAFFECTED) {
> > > + val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV3);
> > > + val |=
> > > FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV3), 1);
> > > + }
> > > +
> > > + val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_AMU);
> > > +
> > > + return val;
> > > +}
> > > +
> > > static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
> > > const struct sys_reg_desc *rd,
> > > u64 val)
> > > {
> > > - struct kvm_arch *arch = &vcpu->kvm->arch;
> > > - u64 sval = val;
> > > u8 csv2, csv3;
> > > - int ret = 0;
> > >
> > > /*
> > > * Allow AA64PFR0_EL1.CSV2 to be set from userspace as
> > > long
> > > as
> > > @@ -226,26 +338,30 @@ static int set_id_aa64pfr0_el1(struct
> > > kvm_vcpu
> > > *vcpu,
> > > if (csv3 > 1 || (csv3 && arm64_get_meltdown_state() !=
> > > SPECTRE_UNAFFECTED))
> > > return -EINVAL;
> >
> > Can't we remove the checking of csv[23] here as it will be checked
> > by
> > arm64_check_features()?
> >
> > i.e. in arm64_check_features() we will load the "limit" value from
> > the
> > "reset" function (read_sanitised_id_aa64pfr0_el1()) which has
> > csv[23]
> > set appropriately and limit it to a safe value basically performing
> > the
> > same check as we are here.
> The limit and the check here might be different if like
> arm64_get_meltdown_state() is not SPECTRE_UNAFFECTED.
> i.e. if we remove the check here, theoretically, the csv3 can be set
> a
> value greater 1 if arm64_get_meltdown_state() is not
> SPECTRE_UNAFFECTED.
Reading init_cpu_ftr_reg() is hurting my head :p but don't we have
basically 2 cases here?
1. We are unaffected by spectre|meltdown i.e.
arm64_get_[spectre|meltdown]_v2_state() will return SPECTRE_UNAFFECTED
and we will set the limit to 1 for the csv[1|2] bit fields in
read_sanitised_id_aa64pfr0_el1()
or
2. We ARE affected by spectre|meltdown in which case
arm64_get_[spectre|meltdown]_v2_state() will be
SPECTRE_VULNERABLE|SPECTRE_MITIGATED in which case
read_sanitised_ftr_reg() will return a value with csv[1|2] set to 0
essentially setting the limit for either of these bit fields to 0 in
read_sanitised_id_aa64pfr0_el1().
Are we trying to catch the case where csv[1|2] is 0 on the host but we
are unaffected as detected by e.g. cpuid and that cpu happens to
incorrectly not set csv[1|2] in the ID register but we still want to
allow the guest to see those bits as set?
This isn't really related to the CR as I know this is existing code
which has just been moved around and sorry if I'm missing something
obvious.
Thanks,
Suraj
> >
> > >
> > > - mutex_lock(&arch->config_lock);
> > > - /* We can only differ with CSV[23], and anything else is
> > > an
> > > error */
> > > - val ^= read_id_reg(vcpu, rd);
> > > - val &= ~(ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV2) |
> > > - ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV3));
> > > - if (val) {
> > > - ret = -EINVAL;
> > > - goto out;
> > > - }
> > > + return set_id_reg(vcpu, rd, val);
> > > +}
> > >
> > > - /* Only allow userspace to change the idregs before VM
> > > running */
> > > - if (test_bit(KVM_ARCH_FLAG_HAS_RAN_ONCE, &vcpu->kvm-
> > > > arch.flags)) {
> > > - if (sval != read_id_reg(vcpu, rd))
> > > - ret = -EBUSY;
> > > - } else {
> > > - IDREG(vcpu->kvm, reg_to_encoding(rd)) = sval;
> > > - }
> > > -out:
> > > - mutex_unlock(&arch->config_lock);
> > > - return ret;
> > > +static u64 read_sanitised_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
> > > + const struct
> > > sys_reg_desc
> > > *rd)
> > > +{
> > > + u64 val;
> > > + u32 id = reg_to_encoding(rd);
> > > +
> > > + val = read_sanitised_ftr_reg(id);
> > > + /* Limit debug to ARMv8.0 */
> > > + val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_DebugVer);
> > > + val |=
> > > FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_DebugVer), 6);
> > > + /*
> > > + * Initialise the default PMUver before there is a chance
> > > to
> > > + * create an actual PMU.
> > > + */
> > > + val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer);
> > > + val |=
> > > FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer),
> > > + kvm_arm_pmu_get_pmuver_limit());
> > > + /* Hide SPE from guests */
> > > + val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMSVer);
> > > +
> > > + return val;
> > > }
> > >
> > > static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
> > > @@ -255,7 +371,6 @@ static int set_id_aa64dfr0_el1(struct
> > > kvm_vcpu
> > > *vcpu,
> > > struct kvm_arch *arch = &vcpu->kvm->arch;
> > > u8 pmuver, host_pmuver;
> > > bool valid_pmu;
> > > - u64 sval = val;
> > > int ret = 0;
> > >
> > > host_pmuver = kvm_arm_pmu_get_pmuver_limit();
> > > @@ -277,40 +392,61 @@ static int set_id_aa64dfr0_el1(struct
> > > kvm_vcpu
> > > *vcpu,
> > > return -EINVAL;
> > >
> > > mutex_lock(&arch->config_lock);
> > > - /* We can only differ with PMUver, and anything else is
> > > an
> > > error */
> > > - val ^= read_id_reg(vcpu, rd);
> > > - val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer);
> > > - if (val) {
> > > - ret = -EINVAL;
> > > - goto out;
> > > - }
> > > -
> > > /* Only allow userspace to change the idregs before VM
> > > running */
> > > if (test_bit(KVM_ARCH_FLAG_HAS_RAN_ONCE, &vcpu->kvm-
> > > > arch.flags)) {
> > > - if (sval != read_id_reg(vcpu, rd))
> > > + if (val != read_id_reg(vcpu, rd))
> > > ret = -EBUSY;
> > > - } else {
> > > - if (valid_pmu) {
> > > - val = IDREG(vcpu->kvm,
> > > SYS_ID_AA64DFR0_EL1);
> > > - val &= ~ID_AA64DFR0_EL1_PMUVer_MASK;
> > > - val |=
> > > FIELD_PREP(ID_AA64DFR0_EL1_PMUVer_MASK, pmuver);
> > > - IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1) =
> > > val;
> > > -
> > > - val = IDREG(vcpu->kvm, SYS_ID_DFR0_EL1);
> > > - val &= ~ID_DFR0_EL1_PerfMon_MASK;
> > > - val |=
> > > FIELD_PREP(ID_DFR0_EL1_PerfMon_MASK,
> > > pmuver_to_perfmon(pmuver));
> > > - IDREG(vcpu->kvm, SYS_ID_DFR0_EL1) = val;
> > > - } else {
> > > -
> > >
> > > assign_bit(KVM_ARCH_FLAG_VCPU_HAS_IMP_DEF_PMU,
> > > &vcpu->kvm->arch.flags,
> > > - pmuver ==
> > > ID_AA64DFR0_EL1_PMUVer_IMP_DEF);
> > > - }
> > > + goto out;
> > > + }
> > > +
> > > + if (!valid_pmu) {
> > > + /*
> > > + * Ignore the PMUVer filed in @val. The PMUVer
> > > would
> >
> > Nit s/filed/field
> Will fix.
> >
> > > be determined
> > > + * by arch flags bit
> > > KVM_ARCH_FLAG_VCPU_HAS_IMP_DEF_PMU,
> > > + */
> > > + pmuver = FIELD_GET(ID_AA64DFR0_EL1_PMUVer_MASK,
> > > + IDREG(vcpu->kvm,
> > > SYS_ID_AA64DFR0_EL1));
> > > + val &= ~ID_AA64DFR0_EL1_PMUVer_MASK;
> > > + val |= FIELD_PREP(ID_AA64DFR0_EL1_PMUVer_MASK,
> > > pmuver);
> > > }
> > >
> > > + ret = arm64_check_features(vcpu, rd, val);
> > > + if (ret)
> > > + goto out;
> > > +
> > > + IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1) = val;
> > > +
> > > + val = IDREG(vcpu->kvm, SYS_ID_DFR0_EL1);
> > > + val &= ~ID_DFR0_EL1_PerfMon_MASK;
> > > + val |= FIELD_PREP(ID_DFR0_EL1_PerfMon_MASK,
> > > pmuver_to_perfmon(pmuver));
> > > + IDREG(vcpu->kvm, SYS_ID_DFR0_EL1) = val;
> > > +
> > > + if (!valid_pmu)
> > > + assign_bit(KVM_ARCH_FLAG_VCPU_HAS_IMP_DEF_PMU,
> > > &vcpu-
> > > > kvm->arch.flags,
> > > + pmuver ==
> > > ID_AA64DFR0_EL1_PMUVer_IMP_DEF);
> > > +
> > > out:
> > > mutex_unlock(&arch->config_lock);
> > > return ret;
> > > }
> > >
> > > +static u64 read_sanitised_id_dfr0_el1(struct kvm_vcpu *vcpu,
> > > + const struct sys_reg_desc
> > > *rd)
> > > +{
> > > + u64 val;
> > > + u32 id = reg_to_encoding(rd);
> > > +
> > > + val = read_sanitised_ftr_reg(id);
> > > + /*
> > > + * Initialise the default PMUver before there is a chance
> > > to
> > > + * create an actual PMU.
> > > + */
> > > + val &= ~ARM64_FEATURE_MASK(ID_DFR0_EL1_PerfMon);
> > > + val |=
> > > FIELD_PREP(ARM64_FEATURE_MASK(ID_DFR0_EL1_PerfMon),
> > > kvm_arm_pmu_get_pmuver_limit());
> > > +
> > > + return val;
> > > +}
> > > +
> > > static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
> > > const struct sys_reg_desc *rd,
> > > u64 val)
> > > @@ -318,7 +454,6 @@ static int set_id_dfr0_el1(struct kvm_vcpu
> > > *vcpu,
> > > struct kvm_arch *arch = &vcpu->kvm->arch;
> > > u8 perfmon, host_perfmon;
> > > bool valid_pmu;
> > > - u64 sval = val;
> > > int ret = 0;
> > >
> > > host_perfmon =
> > > pmuver_to_perfmon(kvm_arm_pmu_get_pmuver_limit());
> > > @@ -341,35 +476,39 @@ static int set_id_dfr0_el1(struct kvm_vcpu
> > > *vcpu,
> > > return -EINVAL;
> > >
> > > mutex_lock(&arch->config_lock);
> > > - /* We can only differ with PerfMon, and anything else is
> > > an
> > > error */
> > > - val ^= read_id_reg(vcpu, rd);
> > > - val &= ~ARM64_FEATURE_MASK(ID_DFR0_EL1_PerfMon);
> > > - if (val) {
> > > - ret = -EINVAL;
> > > - goto out;
> > > - }
> > > -
> > > /* Only allow userspace to change the idregs before VM
> > > running */
> > > if (test_bit(KVM_ARCH_FLAG_HAS_RAN_ONCE, &vcpu->kvm-
> > > > arch.flags)) {
> > > - if (sval != read_id_reg(vcpu, rd))
> > > + if (val != read_id_reg(vcpu, rd))
> > > ret = -EBUSY;
> > > - } else {
> > > - if (valid_pmu) {
> > > - val = IDREG(vcpu->kvm, SYS_ID_DFR0_EL1);
> > > - val &= ~ID_DFR0_EL1_PerfMon_MASK;
> > > - val |=
> > > FIELD_PREP(ID_DFR0_EL1_PerfMon_MASK,
> > > perfmon);
> > > - IDREG(vcpu->kvm, SYS_ID_DFR0_EL1) = val;
> > > -
> > > - val = IDREG(vcpu->kvm,
> > > SYS_ID_AA64DFR0_EL1);
> > > - val &= ~ID_AA64DFR0_EL1_PMUVer_MASK;
> > > - val |=
> > > FIELD_PREP(ID_AA64DFR0_EL1_PMUVer_MASK,
> > > perfmon_to_pmuver(perfmon));
> > > - IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1) =
> > > val;
> > > - } else {
> > > -
> > >
> > > assign_bit(KVM_ARCH_FLAG_VCPU_HAS_IMP_DEF_PMU,
> > > &vcpu->kvm->arch.flags,
> > > - perfmon ==
> > > ID_DFR0_EL1_PerfMon_IMPDEF);
> > > - }
> > > + goto out;
> > > + }
> > > +
> > > + if (!valid_pmu) {
> > > + /*
> > > + * Ignore the PerfMon filed in @val. The PerfMon
> >
> > Nit s/filed/field
> Thanks, will fix it.
> >
> > > would be determined
> > > + * by arch flags bit
> > > KVM_ARCH_FLAG_VCPU_HAS_IMP_DEF_PMU,
> > > + */
> > > + perfmon = FIELD_GET(ID_DFR0_EL1_PerfMon_MASK,
> > > + IDREG(vcpu->kvm,
> > > SYS_ID_DFR0_EL1));
> > > + val &= ~ID_DFR0_EL1_PerfMon_MASK;
> > > + val |= FIELD_PREP(ID_DFR0_EL1_PerfMon_MASK,
> > > perfmon);
> > > }
> > >
> > > + ret = arm64_check_features(vcpu, rd, val);
> > > + if (ret)
> > > + goto out;
> > > +
> > > + IDREG(vcpu->kvm, SYS_ID_DFR0_EL1) = val;
> > > +
> > > + val = IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1);
> > > + val &= ~ID_AA64DFR0_EL1_PMUVer_MASK;
> > > + val |= FIELD_PREP(ID_AA64DFR0_EL1_PMUVer_MASK,
> > > perfmon_to_pmuver(perfmon));
> > > + IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1) = val;
> > > +
> > > + if (!valid_pmu)
> > > + assign_bit(KVM_ARCH_FLAG_VCPU_HAS_IMP_DEF_PMU,
> > > &vcpu-
> > > > kvm->arch.flags,
> > > + perfmon ==
> > > ID_DFR0_EL1_PerfMon_IMPDEF);
> > > +
> > > out:
> > > mutex_unlock(&arch->config_lock);
> > > return ret;
> >
> > Otherwise looks good!
> >
> > Thanks,
> > Suraj
>
> Thanks,
> Jing
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v8 6/6] KVM: arm64: Refactor writings for PMUVer/CSV2/CSV3
2023-05-18 21:08 ` Jitindar Singh, Suraj
@ 2023-05-19 9:16 ` Marc Zyngier
2023-05-19 23:04 ` Jitindar Singh, Suraj
0 siblings, 1 reply; 34+ messages in thread
From: Marc Zyngier @ 2023-05-19 9:16 UTC (permalink / raw)
To: Jitindar Singh, Suraj
Cc: jingzhangos, pbonzini, reijiw, james.morse, suzuki.poulose,
kvmarm, rananta, linux-arm-kernel, kvm, tabba, oupton,
alexandru.elisei, will
On Thu, 18 May 2023 22:08:15 +0100,
"Jitindar Singh, Suraj" <surajjs@amazon.com> wrote:
>
> Reading init_cpu_ftr_reg() is hurting my head :p but don't we have
> basically 2 cases here?
>
> 1. We are unaffected by spectre|meltdown i.e.
> arm64_get_[spectre|meltdown]_v2_state() will return SPECTRE_UNAFFECTED
> and we will set the limit to 1 for the csv[1|2] bit fields in
> read_sanitised_id_aa64pfr0_el1()
>
> or
>
> 2. We ARE affected by spectre|meltdown in which case
> arm64_get_[spectre|meltdown]_v2_state() will be
> SPECTRE_VULNERABLE|SPECTRE_MITIGATED in which case
> read_sanitised_ftr_reg() will return a value with csv[1|2] set to 0
> essentially setting the limit for either of these bit fields to 0 in
> read_sanitised_id_aa64pfr0_el1().
What is "WE"?
>
> Are we trying to catch the case where csv[1|2] is 0 on the host but we
> are unaffected as detected by e.g. cpuid and that cpu happens to
> incorrectly not set csv[1|2] in the ID register but we still want to
> allow the guest to see those bits as set?
>
> This isn't really related to the CR as I know this is existing code
> which has just been moved around and sorry if I'm missing something
> obvious.
This code is called from *userspace*, and tries to cope with a VM
being restored. So we have 3 (not 2 cases):
- both the source and the destination have the same level of CSVx
support, and all is great in the world
- the source has CSVx==0, while the destination has CSVx=1. All good,
as we won't be lying to the guest, and the extra mitigation
executed by the guest isn't a functional problem. The guest will
still see CSVx=0 after migration.
- the source has CSVx=1, while the destination has CSVx=0. This isn't
an acceptable situation, and we return an error
Is that clearer?
M.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v8 6/6] KVM: arm64: Refactor writings for PMUVer/CSV2/CSV3
2023-05-19 9:16 ` Marc Zyngier
@ 2023-05-19 23:04 ` Jitindar Singh, Suraj
2023-05-20 8:45 ` Marc Zyngier
0 siblings, 1 reply; 34+ messages in thread
From: Jitindar Singh, Suraj @ 2023-05-19 23:04 UTC (permalink / raw)
To: maz
Cc: jingzhangos, oupton, alexandru.elisei, james.morse,
suzuki.poulose, kvmarm, rananta, linux-arm-kernel, pbonzini,
reijiw, will, kvm, tabba
On Fri, 2023-05-19 at 10:16 +0100, Marc Zyngier wrote:
> CAUTION: This email originated from outside of the organization. Do
> not click links or open attachments unless you can confirm the sender
> and know the content is safe.
>
>
>
> On Thu, 18 May 2023 22:08:15 +0100,
> "Jitindar Singh, Suraj" <surajjs@amazon.com> wrote:
> >
> > Reading init_cpu_ftr_reg() is hurting my head :p but don't we have
> > basically 2 cases here?
> >
> > 1. We are unaffected by spectre|meltdown i.e.
> > arm64_get_[spectre|meltdown]_v2_state() will return
> > SPECTRE_UNAFFECTED
> > and we will set the limit to 1 for the csv[1|2] bit fields in
> > read_sanitised_id_aa64pfr0_el1()
> >
> > or
> >
> > 2. We ARE affected by spectre|meltdown in which case
> > arm64_get_[spectre|meltdown]_v2_state() will be
> > SPECTRE_VULNERABLE|SPECTRE_MITIGATED in which case
> > read_sanitised_ftr_reg() will return a value with csv[1|2] set to 0
> > essentially setting the limit for either of these bit fields to 0
> > in
> > read_sanitised_id_aa64pfr0_el1().
>
> What is "WE"?
The CPU
>
> >
> > Are we trying to catch the case where csv[1|2] is 0 on the host but
> > we
> > are unaffected as detected by e.g. cpuid and that cpu happens to
> > incorrectly not set csv[1|2] in the ID register but we still want
> > to
> > allow the guest to see those bits as set?
> >
> > This isn't really related to the CR as I know this is existing code
> > which has just been moved around and sorry if I'm missing something
> > obvious.
>
> This code is called from *userspace*, and tries to cope with a VM
> being restored. So we have 3 (not 2 cases):
>
> - both the source and the destination have the same level of CSVx
> support, and all is great in the world
>
> - the source has CSVx==0, while the destination has CSVx=1. All good,
> as we won't be lying to the guest, and the extra mitigation
> executed by the guest isn't a functional problem. The guest will
> still see CSVx=0 after migration.
>
> - the source has CSVx=1, while the destination has CSVx=0. This isn't
> an acceptable situation, and we return an error
>
> Is that clearer?
Yes thanks, that all makes sense.
My initial question was why we needed to enforce the limit essentially
twice in set_id_aa64pfr0_el1().
Once with:
/*
* Allow AA64PFR0_EL1.CSV2 to be set from userspace as long as
* it doesn't promise more than what is actually provided (the
* guest could otherwise be covered in ectoplasmic residue).
*/
csv2 = cpuid_feature_extract_unsigned_field(val,
ID_AA64PFR0_EL1_CSV2_SHIFT);
if (csv2 > 1 ||
(csv2 && arm64_get_spectre_v2_state() !=
SPECTRE_UNAFFECTED))
return -EINVAL;
/* Same thing for CSV3 */
csv3 = cpuid_feature_extract_unsigned_field(val,
ID_AA64PFR0_EL1_CSV3_SHIFT);
if (csv3 > 1 ||
(csv3 && arm64_get_meltdown_state() != SPECTRE_UNAFFECTED))
return -EINVAL;
And then again with the check in arm64_check_features().
Thanks,
Suraj
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v8 6/6] KVM: arm64: Refactor writings for PMUVer/CSV2/CSV3
2023-05-03 17:16 ` [PATCH v8 6/6] KVM: arm64: Refactor writings for PMUVer/CSV2/CSV3 Jing Zhang
2023-05-17 22:00 ` Jitindar Singh, Suraj
@ 2023-05-19 23:25 ` Suraj Jitindar Singh
1 sibling, 0 replies; 34+ messages in thread
From: Suraj Jitindar Singh @ 2023-05-19 23:25 UTC (permalink / raw)
To: Jing Zhang, KVM, KVMARM, ARMLinux, Marc Zyngier, Oliver Upton
Cc: Will Deacon, Paolo Bonzini, James Morse, Alexandru Elisei,
Suzuki K Poulose, Fuad Tabba, Reiji Watanabe,
Raghavendra Rao Ananta
On Wed, 2023-05-03 at 17:16 +0000, Jing Zhang wrote:
> Refactor writings for ID_AA64PFR0_EL1.[CSV2|CSV3],
> ID_AA64DFR0_EL1.PMUVer and ID_DFR0_ELF.PerfMon based on utilities
> introduced by ID register descriptor array.
>
> Signed-off-by: Jing Zhang <jingzhangos@google.com>
Hi,
> ---
> arch/arm64/include/asm/cpufeature.h | 1 +
> arch/arm64/kernel/cpufeature.c | 2 +-
> arch/arm64/kvm/id_regs.c | 361 ++++++++++++++++++--------
> --
> 3 files changed, 242 insertions(+), 122 deletions(-)
>
>
[ snip ]
> diff --git a/arch/arm64/kvm/id_regs.c b/arch/arm64/kvm/id_regs.c
> index 64d40aa395be..6cd56c9e6428 100644
> --- a/arch/arm64/kvm/id_regs.c
> +++ b/arch/arm64/kvm/id_regs.c
> @@ -18,6 +18,86 @@
>
> #include "sys_regs.h"
>
> [ snip ]
>
> +static u64 read_sanitised_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
> + const struct sys_reg_desc
> *rd)
> +{
> + u64 val;
> + u32 id = reg_to_encoding(rd);
> +
> + val = read_sanitised_ftr_reg(id);
> + /*
> + * The default is to expose CSV2 == 1 if the HW isn't
> affected.
> + * Although this is a per-CPU feature, we make it global
> because
> + * asymmetric systems are just a nuisance.
> + *
> + * Userspace can override this as long as it doesn't promise
> + * the impossible.
> + */
> + if (arm64_get_spectre_v2_state() == SPECTRE_UNAFFECTED) {
> + val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV2);
> + val |=
> FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV2), 1);
> + }
> + if (arm64_get_meltdown_state() == SPECTRE_UNAFFECTED) {
> + val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV3);
> + val |=
> FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV3), 1);
> + }
> +
I think the virtual GIC check also needs to be moved here from
kvm_arm_read_id_reg()
if (kvm_vgic_global_state.type == VGIC_V3) {
val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_GIC);
val |=
FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_GIC), 1);
}
Since the host supports GICv4.1 ID_AA64PFR0_EL1_GIC == 3 when qemu
tries to read this register then overwrite it with the same value it
read previously an error occurs. ID_AA64PFR0_EL1_GIC == 3 is the
"limit" value however this field will read as ID_AA64PFR0_EL1_GIC == 1
when a virtual GICv3 is in use. Thus when qemu tries to set
ID_AA64PFR0_EL1_GIC == 1, arm64_check_features() fails as those bits
aren't set in id_reg.val meaning that modifications aren't allowed.
Additionally this means it's possible to set ID_AA64PFR0_EL1_GIC == 3
from userspace however any reads will see this field as
ID_AA64PFR0_EL1_GIC == 1.
This all means the smp kvm_unit_tests failed. With the above change
they pass.
Thanks,
Suraj
> + val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_AMU);
> +
> + return val;
> +}
> +
> static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
> const struct sys_reg_desc *rd,
> u64 val)
[ snip ]
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v8 6/6] KVM: arm64: Refactor writings for PMUVer/CSV2/CSV3
2023-05-19 23:04 ` Jitindar Singh, Suraj
@ 2023-05-20 8:45 ` Marc Zyngier
0 siblings, 0 replies; 34+ messages in thread
From: Marc Zyngier @ 2023-05-20 8:45 UTC (permalink / raw)
To: Jitindar Singh, Suraj
Cc: jingzhangos, oupton, alexandru.elisei, james.morse,
suzuki.poulose, kvmarm, rananta, linux-arm-kernel, pbonzini,
reijiw, will, kvm, tabba
On Sat, 20 May 2023 00:04:53 +0100,
"Jitindar Singh, Suraj" <surajjs@amazon.com> wrote:
>
> On Fri, 2023-05-19 at 10:16 +0100, Marc Zyngier wrote:
> > CAUTION: This email originated from outside of the organization. Do
> > not click links or open attachments unless you can confirm the sender
> > and know the content is safe.
> >
> >
> >
> > On Thu, 18 May 2023 22:08:15 +0100,
> > "Jitindar Singh, Suraj" <surajjs@amazon.com> wrote:
> > >
> > > Reading init_cpu_ftr_reg() is hurting my head :p but don't we have
> > > basically 2 cases here?
> > >
> > > 1. We are unaffected by spectre|meltdown i.e.
> > > arm64_get_[spectre|meltdown]_v2_state() will return
> > > SPECTRE_UNAFFECTED
> > > and we will set the limit to 1 for the csv[1|2] bit fields in
> > > read_sanitised_id_aa64pfr0_el1()
> > >
> > > or
> > >
> > > 2. We ARE affected by spectre|meltdown in which case
> > > arm64_get_[spectre|meltdown]_v2_state() will be
> > > SPECTRE_VULNERABLE|SPECTRE_MITIGATED in which case
> > > read_sanitised_ftr_reg() will return a value with csv[1|2] set to 0
> > > essentially setting the limit for either of these bit fields to 0
> > > in
> > > read_sanitised_id_aa64pfr0_el1().
> >
> > What is "WE"?
>
> The CPU
Hilarious.
>
> >
> > >
> > > Are we trying to catch the case where csv[1|2] is 0 on the host but
> > > we
> > > are unaffected as detected by e.g. cpuid and that cpu happens to
> > > incorrectly not set csv[1|2] in the ID register but we still want
> > > to
> > > allow the guest to see those bits as set?
> > >
> > > This isn't really related to the CR as I know this is existing code
> > > which has just been moved around and sorry if I'm missing something
> > > obvious.
> >
> > This code is called from *userspace*, and tries to cope with a VM
> > being restored. So we have 3 (not 2 cases):
> >
> > - both the source and the destination have the same level of CSVx
> > support, and all is great in the world
> >
> > - the source has CSVx==0, while the destination has CSVx=1. All good,
> > as we won't be lying to the guest, and the extra mitigation
> > executed by the guest isn't a functional problem. The guest will
> > still see CSVx=0 after migration.
> >
> > - the source has CSVx=1, while the destination has CSVx=0. This isn't
> > an acceptable situation, and we return an error
> >
> > Is that clearer?
>
> Yes thanks, that all makes sense.
>
> My initial question was why we needed to enforce the limit essentially
> twice in set_id_aa64pfr0_el1().
>
> Once with:
> /*
> * Allow AA64PFR0_EL1.CSV2 to be set from userspace as long as
> * it doesn't promise more than what is actually provided (the
> * guest could otherwise be covered in ectoplasmic residue).
> */
> csv2 = cpuid_feature_extract_unsigned_field(val,
> ID_AA64PFR0_EL1_CSV2_SHIFT);
> if (csv2 > 1 ||
> (csv2 && arm64_get_spectre_v2_state() !=
> SPECTRE_UNAFFECTED))
> return -EINVAL;
>
> /* Same thing for CSV3 */
> csv3 = cpuid_feature_extract_unsigned_field(val,
> ID_AA64PFR0_EL1_CSV3_SHIFT);
> if (csv3 > 1 ||
> (csv3 && arm64_get_meltdown_state() != SPECTRE_UNAFFECTED))
> return -EINVAL;
>
> And then again with the check in arm64_check_features().
Ah, I see what you mean. Yes, this isn't right. I asked for the
generic code to be used in the past, but the old stuff was left
in. Which is obviously not great.
M.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 34+ messages in thread
end of thread, other threads:[~2023-05-20 8:45 UTC | newest]
Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-03 17:16 [PATCH v8 0/6] Support writable CPU ID registers from userspace Jing Zhang
2023-05-03 17:16 ` [PATCH v8 1/6] KVM: arm64: Move CPU ID feature registers emulation into a separate file Jing Zhang
2023-05-16 16:11 ` Marc Zyngier
2023-05-16 19:14 ` Jing Zhang
2023-05-03 17:16 ` [PATCH v8 2/6] KVM: arm64: Save ID registers' sanitized value per guest Jing Zhang
2023-05-17 7:41 ` Marc Zyngier
2023-05-17 16:28 ` Jing Zhang
2023-05-03 17:16 ` [PATCH v8 3/6] KVM: arm64: Use per guest ID register for ID_AA64PFR0_EL1.[CSV2|CSV3] Jing Zhang
2023-05-03 23:43 ` kernel test robot
2023-05-03 17:16 ` [PATCH v8 4/6] KVM: arm64: Use per guest ID register for ID_AA64DFR0_EL1.PMUVer Jing Zhang
2023-05-03 17:16 ` [PATCH v8 5/6] KVM: arm64: Reuse fields of sys_reg_desc for idreg Jing Zhang
2023-05-16 10:26 ` Cornelia Huck
2023-05-16 19:10 ` Jing Zhang
2023-05-03 17:16 ` [PATCH v8 6/6] KVM: arm64: Refactor writings for PMUVer/CSV2/CSV3 Jing Zhang
2023-05-17 22:00 ` Jitindar Singh, Suraj
2023-05-17 22:55 ` Jing Zhang
2023-05-18 21:08 ` Jitindar Singh, Suraj
2023-05-19 9:16 ` Marc Zyngier
2023-05-19 23:04 ` Jitindar Singh, Suraj
2023-05-20 8:45 ` Marc Zyngier
2023-05-19 23:25 ` Suraj Jitindar Singh
2023-05-16 10:37 ` [PATCH v8 0/6] Support writable CPU ID registers from userspace Shameerali Kolothum Thodi
[not found] ` <868rdomtfo.wl-maz@kernel.org>
2023-05-16 11:11 ` Shameerali Kolothum Thodi
2023-05-16 11:55 ` Cornelia Huck
2023-05-16 13:11 ` Marc Zyngier
2023-05-16 13:44 ` Shameerali Kolothum Thodi
2023-05-16 14:21 ` Cornelia Huck
2023-05-16 14:19 ` Cornelia Huck
2023-05-16 16:01 ` Marc Zyngier
2023-05-17 15:36 ` Cornelia Huck
2023-05-17 15:53 ` Marc Zyngier
2023-05-16 16:31 ` Oliver Upton
2023-05-16 16:44 ` Marc Zyngier
2023-05-16 16:57 ` Oliver Upton
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).