Before this change, the cache configuration of the physical CPU was
exposed to vcpus. This is problematic because the cache configuration a
vcpu sees varies when it migrates between vcpus with different cache
configurations.
Fabricate cache configuration from the sanitized value, which holds the
CTR_EL0 value the userspace sees regardless of which physical CPU it
resides on.
V4 -> V5:
- Noted why cache level existence check is unnecessary when fabricating
CCSIDR_EL1 value.
- Removed FWB check. It is necessary as CLIDR_EL1.{LoUU, LoIUS} on the
host are {0, 0} if FWB is enabled, and such a CLIDR_EL1 value sets
the IDC bit of the sanitized CTR_EL0 value, which is already checked.
- Removed UNDEF injection when reading CCSIDR_EL1 with an invalid
CSSELR_EL1 value.
- Added a check for CLIDR_EL1.{LoUU,LoC,LoUIS} values set from the
userspace.
V3 -> V4:
- Implemented UNKNOWN system register definition for CCSIDR_EL1
- Added a comment about the relation between CCSIDR_EL1 and FEAT_CCIDX
- Squashed "Normalize cache configuration" and "Allow user to set
CCSIDR_EL1"
The intermediate state between them did not make much sense.
- Introduced FIELD_GET to extract CCSIDR_EL1_LineSize.
V2 -> V3:
- Corrected message for patch "Normalize cache configuration"
- Split patch "Normalize cache configuration"
- Added handling for CSSELR_EL1.TnD
- Added code to ignore RES0 in CSSELR_EL1
- Replaced arm64_ftr_reg_ctrel0.sys_val with
read_sanitised_ftr_reg(SYS_CTR_EL0)
- Fixed vcpu->arch.ccsidr initialziation
- Added CCSIDR_EL1 sanitization
- Added FWB check
- Added a comment for CACHE_TYPE_SEPARATE
- Added MTE tag cache creation code for CLIDR_EL1 fabrication
- Removed CLIDR_EL1 reset code for reset caused by guest
- Added a comment for CCSIDR2
V2: https://lore.kernel.org/lkml/[email protected]/
V1: https://lore.kernel.org/lkml/[email protected]/
Akihiko Odaki (6):
arm64/sysreg: Convert CCSIDR_EL1 to automatic generation
arm64/sysreg: Add CCSIDR2_EL1
arm64/cache: Move CLIDR macro definitions
KVM: arm64: Always set HCR_TID2
KVM: arm64: Mask FEAT_CCIDX
KVM: arm64: Normalize cache configuration
Marc Zyngier (1):
arm64: Allow the definition of UNKNOWN system register fields
arch/arm64/include/asm/cache.h | 9 +
arch/arm64/include/asm/kvm_arm.h | 3 +-
arch/arm64/include/asm/kvm_emulate.h | 4 -
arch/arm64/include/asm/kvm_host.h | 6 +-
arch/arm64/include/asm/sysreg.h | 1 -
arch/arm64/kernel/cacheinfo.c | 5 -
arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h | 2 -
arch/arm64/kvm/reset.c | 1 +
arch/arm64/kvm/sys_regs.c | 256 +++++++++++++--------
arch/arm64/tools/gen-sysreg.awk | 20 +-
arch/arm64/tools/sysreg | 17 ++
11 files changed, 212 insertions(+), 112 deletions(-)
--
2.38.1
Convert CCSIDR_EL1 to automatic generation as per DDI0487I.a.
Signed-off-by: Akihiko Odaki <[email protected]>
Reviewed-by: Mark Brown <[email protected]>
---
arch/arm64/include/asm/sysreg.h | 1 -
arch/arm64/tools/sysreg | 10 ++++++++++
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
index 7d301700d1a9..910e960661d3 100644
--- a/arch/arm64/include/asm/sysreg.h
+++ b/arch/arm64/include/asm/sysreg.h
@@ -425,7 +425,6 @@
#define SYS_CNTKCTL_EL1 sys_reg(3, 0, 14, 1, 0)
-#define SYS_CCSIDR_EL1 sys_reg(3, 1, 0, 0, 0)
#define SYS_AIDR_EL1 sys_reg(3, 1, 0, 0, 7)
#define SYS_RNDR_EL0 sys_reg(3, 3, 2, 4, 0)
diff --git a/arch/arm64/tools/sysreg b/arch/arm64/tools/sysreg
index 8f26fe1bedc6..097d6faafc87 100644
--- a/arch/arm64/tools/sysreg
+++ b/arch/arm64/tools/sysreg
@@ -873,6 +873,16 @@ Sysreg SCXTNUM_EL1 3 0 13 0 7
Field 63:0 SoftwareContextNumber
EndSysreg
+# The bit layout for CCSIDR_EL1 depends on whether FEAT_CCIDX is implemented.
+# The following is for case when FEAT_CCIDX is not implemented.
+Sysreg CCSIDR_EL1 3 1 0 0 0
+Res0 63:32
+Unkn 31:28
+Field 27:13 NumSets
+Field 12:3 Associativity
+Field 2:0 LineSize
+EndSysreg
+
Sysreg CLIDR_EL1 3 1 0 0 1
Res0 63:47
Field 46:33 Ttypen
--
2.38.1
The CCSIDR access handler masks the associativity bits according to the
bit layout for processors without FEAT_CCIDX. KVM also assumes CCSIDR is
32-bit where it will be 64-bit if FEAT_CCIDX is enabled. Mask FEAT_CCIDX
so that these assumptions hold.
Suggested-by: Marc Zyngier <[email protected]>
Signed-off-by: Akihiko Odaki <[email protected]>
---
arch/arm64/kvm/sys_regs.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index f4a7c5abcbca..aeabf1f3370b 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1124,6 +1124,12 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu, struct sys_reg_desc const *r
ID_DFR0_PERFMON_SHIFT,
kvm_vcpu_has_pmu(vcpu) ? ID_DFR0_PERFMON_8_4 : 0);
break;
+ case SYS_ID_AA64MMFR2_EL1:
+ val &= ~ID_AA64MMFR2_EL1_CCIDX_MASK;
+ break;
+ case SYS_ID_MMFR4_EL1:
+ val &= ~ARM64_FEATURE_MASK(ID_MMFR4_CCIDX);
+ break;
}
return val;
@@ -1605,6 +1611,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
{ SYS_DESC(SYS_CCSIDR_EL1), access_ccsidr },
{ SYS_DESC(SYS_CLIDR_EL1), access_clidr },
+ { SYS_DESC(SYS_CCSIDR2_EL1), undef_access },
{ SYS_DESC(SYS_SMIDR_EL1), undef_access },
{ SYS_DESC(SYS_CSSELR_EL1), access_csselr, reset_unknown, CSSELR_EL1 },
{ SYS_DESC(SYS_CTR_EL0), access_ctr },
@@ -2106,6 +2113,10 @@ static const struct sys_reg_desc cp15_regs[] = {
{ Op1(1), CRn( 0), CRm( 0), Op2(0), access_ccsidr },
{ Op1(1), CRn( 0), CRm( 0), Op2(1), access_clidr },
+
+ /* CCSIDR2 */
+ { Op1(1), CRn( 0), CRm( 0), Op2(2), undef_access },
+
{ Op1(2), CRn( 0), CRm( 0), Op2(0), access_csselr, NULL, CSSELR_EL1 },
};
--
2.38.1
Before this change, the cache configuration of the physical CPU was
exposed to vcpus. This is problematic because the cache configuration a
vcpu sees varies when it migrates between vcpus with different cache
configurations.
Fabricate cache configuration from the sanitized value, which holds the
CTR_EL0 value the userspace sees regardless of which physical CPU it
resides on.
CLIDR_EL1 and CCSIDR_EL1 are now writable from the userspace so that
the VMM can restore the values saved with the old kernel.
Suggested-by: Marc Zyngier <[email protected]>
Signed-off-by: Akihiko Odaki <[email protected]>
---
arch/arm64/include/asm/cache.h | 3 +
arch/arm64/include/asm/kvm_host.h | 4 +
arch/arm64/kvm/reset.c | 1 +
arch/arm64/kvm/sys_regs.c | 245 ++++++++++++++++++------------
4 files changed, 157 insertions(+), 96 deletions(-)
diff --git a/arch/arm64/include/asm/cache.h b/arch/arm64/include/asm/cache.h
index ab7133654a72..a51e6e8f3171 100644
--- a/arch/arm64/include/asm/cache.h
+++ b/arch/arm64/include/asm/cache.h
@@ -22,6 +22,9 @@
#define CLIDR_CTYPE(clidr, level) \
(((clidr) & CLIDR_CTYPE_MASK(level)) >> CLIDR_CTYPE_SHIFT(level))
+/* Ttypen, bits [2(n - 1) + 34 : 2(n - 1) + 33], for n = 1 to 7 */
+#define CLIDR_TTYPE_SHIFT(level) (2 * ((level) - 1) + CLIDR_EL1_Ttypen_SHIFT)
+
/*
* Memory returned by kmalloc() may be used for DMA, so we must make
* sure that all such allocations are cache aligned. Otherwise,
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index cc2ede0eaed4..27abf81c6910 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -178,6 +178,7 @@ struct kvm_vcpu_fault_info {
enum vcpu_sysreg {
__INVALID_SYSREG__, /* 0 is reserved as an invalid value */
MPIDR_EL1, /* MultiProcessor Affinity Register */
+ CLIDR_EL1, /* Cache Level ID Register */
CSSELR_EL1, /* Cache Size Selection Register */
SCTLR_EL1, /* System Control Register */
ACTLR_EL1, /* Auxiliary Control Register */
@@ -417,6 +418,9 @@ struct kvm_vcpu_arch {
u64 last_steal;
gpa_t base;
} steal;
+
+ /* Per-vcpu CCSIDR override or NULL */
+ u32 *ccsidr;
};
/*
diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
index 5ae18472205a..7980983dbad7 100644
--- a/arch/arm64/kvm/reset.c
+++ b/arch/arm64/kvm/reset.c
@@ -157,6 +157,7 @@ void kvm_arm_vcpu_destroy(struct kvm_vcpu *vcpu)
if (sve_state)
kvm_unshare_hyp(sve_state, sve_state + vcpu_sve_state_size(vcpu));
kfree(sve_state);
+ kfree(vcpu->arch.ccsidr);
}
static void kvm_vcpu_reset_sve(struct kvm_vcpu *vcpu)
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index aeabf1f3370b..47601806636a 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -11,6 +11,7 @@
#include <linux/bitfield.h>
#include <linux/bsearch.h>
+#include <linux/cacheinfo.h>
#include <linux/kvm_host.h>
#include <linux/mm.h>
#include <linux/printk.h>
@@ -81,25 +82,78 @@ void vcpu_write_sys_reg(struct kvm_vcpu *vcpu, u64 val, int reg)
__vcpu_sys_reg(vcpu, reg) = val;
}
-/* 3 bits per cache level, as per CLIDR, but non-existent caches always 0 */
-static u32 cache_levels;
-
/* CSSELR values; used to index KVM_REG_ARM_DEMUX_ID_CCSIDR */
#define CSSELR_MAX 14
+static u8 get_min_cache_line_size(u32 csselr)
+{
+ u64 ctr_el0;
+ int field;
+
+ ctr_el0 = read_sanitised_ftr_reg(SYS_CTR_EL0);
+ field = csselr & CSSELR_EL1_InD ? CTR_EL0_IminLine_SHIFT : CTR_EL0_DminLine_SHIFT;
+
+ return cpuid_feature_extract_unsigned_field(ctr_el0, field) - 2;
+}
+
/* Which cache CCSIDR represents depends on CSSELR value. */
-static u32 get_ccsidr(u32 csselr)
+static u32 get_ccsidr(struct kvm_vcpu *vcpu, u32 csselr)
{
- u32 ccsidr;
+ if (vcpu->arch.ccsidr)
+ return vcpu->arch.ccsidr[csselr];
- /* Make sure noone else changes CSSELR during this! */
- local_irq_disable();
- write_sysreg(csselr, csselr_el1);
- isb();
- ccsidr = read_sysreg(ccsidr_el1);
- local_irq_enable();
+ /*
+ * Fabricate a CCSIDR value as the overriding value does not exist.
+ * The real CCSIDR value will not be used as it can vary by the
+ * physical CPU which the vcpu currently resides in.
+ *
+ * The line size is determined with get_min_cache_line_size(), which
+ * should be valid for all CPUs even if they have different cache
+ * configuration.
+ *
+ * The associativity bits are cleared, meaning the geometry of all data
+ * and unified caches (which are guaranteed to be PIPT and thus
+ * non-aliasing) are 1 set and 1 way.
+ * Guests should not be doing cache operations by set/way at all, and
+ * for this reason, we trap them and attempt to infer the intent, so
+ * that we can flush the entire guest's address space at the appropriate
+ * time. The exposed geometry minimizes the number of the traps.
+ * [If guests should attempt to infer aliasing properties from the
+ * geometry (which is not permitted by the architecture), they would
+ * only do so for virtually indexed caches.]
+ *
+ * We don't check if the cache level exists as it is allowed to return
+ * an UNKNOWN value if not.
+ */
+ return get_min_cache_line_size(csselr) << CCSIDR_EL1_LineSize_SHIFT;
+}
- return ccsidr;
+static int set_ccsidr(struct kvm_vcpu *vcpu, u32 csselr, u32 val)
+{
+ u8 line_size = FIELD_GET(CCSIDR_EL1_LineSize, val);
+ u32 *ccsidr = vcpu->arch.ccsidr;
+ u32 i;
+
+ if ((val & CCSIDR_EL1_RES0) || line_size < get_min_cache_line_size(csselr))
+ return -EINVAL;
+
+ if (!ccsidr) {
+ if (val == get_ccsidr(vcpu, csselr))
+ return 0;
+
+ ccsidr = kmalloc_array(CSSELR_MAX, sizeof(u32), GFP_KERNEL);
+ if (!ccsidr)
+ return -ENOMEM;
+
+ for (i = 0; i < CSSELR_MAX; i++)
+ ccsidr[i] = get_ccsidr(vcpu, i);
+
+ vcpu->arch.ccsidr = ccsidr;
+ }
+
+ ccsidr[csselr] = val;
+
+ return 0;
}
/*
@@ -1281,10 +1335,78 @@ static bool access_clidr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
if (p->is_write)
return write_to_read_only(vcpu, p, r);
- p->regval = read_sysreg(clidr_el1);
+ p->regval = __vcpu_sys_reg(vcpu, r->reg);
return true;
}
+/*
+ * 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)
+{
+ u64 ctr_el0 = read_sanitised_ftr_reg(SYS_CTR_EL0);
+ u64 clidr;
+ u8 loc;
+
+ if ((ctr_el0 & CTR_EL0_IDC)) {
+ /*
+ * Data cache clean to the PoU is not required so LoUU and LoUIS
+ * will not be set and a unified cache, which will be marked as
+ * LoC, will be added.
+ *
+ * If not DIC, let the unified cache L2 so that an instruction
+ * cache can be added as L1 later.
+ */
+ loc = (ctr_el0 & CTR_EL0_DIC) ? 1 : 2;
+ clidr = CACHE_TYPE_UNIFIED << CLIDR_CTYPE_SHIFT(loc);
+ } else {
+ /*
+ * Data cache clean to the PoU is required so let L1 have a data
+ * cache and mark it as LoUU and LoUIS. As L1 has a data cache,
+ * it can be marked as LoC too.
+ */
+ loc = 1;
+ clidr = 1 << CLIDR_LOUU_SHIFT;
+ clidr |= 1 << CLIDR_LOUIS_SHIFT;
+ clidr |= CACHE_TYPE_DATA << CLIDR_CTYPE_SHIFT(1);
+ }
+
+ /*
+ * Instruction cache invalidation to the PoU is required so let L1 have
+ * an instruction cache. If L1 already has a data cache, it will be
+ * CACHE_TYPE_SEPARATE.
+ */
+ if (!(ctr_el0 & CTR_EL0_DIC))
+ clidr |= CACHE_TYPE_INST << CLIDR_CTYPE_SHIFT(1);
+
+ clidr |= loc << CLIDR_LOC_SHIFT;
+
+ /*
+ * Add tag cache unified to data cache. Allocation tags and data are
+ * unified in a cache line so that it looks valid even if there is only
+ * one cache line.
+ */
+ if (kvm_has_mte(vcpu->kvm))
+ clidr |= 2 << CLIDR_TTYPE_SHIFT(loc);
+
+ __vcpu_sys_reg(vcpu, r->reg) = clidr;
+}
+
+static int set_clidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
+ u64 val)
+{
+ u64 ctr_el0 = read_sanitised_ftr_reg(SYS_CTR_EL0);
+ u64 idc = !CLIDR_LOC(val) || (!CLIDR_LOUIS(val) && !CLIDR_LOUU(val));
+
+ if ((val & CLIDR_EL1_RES0) || (!(ctr_el0 & CTR_EL0_IDC) && idc))
+ return -EINVAL;
+
+ __vcpu_sys_reg(vcpu, rd->reg) = val;
+
+ return 0;
+}
+
static bool access_csselr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
const struct sys_reg_desc *r)
{
@@ -1306,22 +1428,10 @@ static bool access_ccsidr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
return write_to_read_only(vcpu, p, r);
csselr = vcpu_read_sys_reg(vcpu, CSSELR_EL1);
- p->regval = get_ccsidr(csselr);
+ csselr &= CSSELR_EL1_Level | CSSELR_EL1_InD;
+ if (csselr < CSSELR_MAX)
+ p->regval = get_ccsidr(vcpu, csselr);
- /*
- * Guests should not be doing cache operations by set/way at all, and
- * for this reason, we trap them and attempt to infer the intent, so
- * that we can flush the entire guest's address space at the appropriate
- * time.
- * To prevent this trapping from causing performance problems, let's
- * expose the geometry of all data and unified caches (which are
- * guaranteed to be PIPT and thus non-aliasing) as 1 set and 1 way.
- * [If guests should attempt to infer aliasing properties from the
- * geometry (which is not permitted by the architecture), they would
- * only do so for virtually indexed caches.]
- */
- if (!(csselr & 1)) // data or unified cache
- p->regval &= ~GENMASK(27, 3);
return true;
}
@@ -1610,7 +1720,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
{ SYS_DESC(SYS_CNTKCTL_EL1), NULL, reset_val, CNTKCTL_EL1, 0},
{ SYS_DESC(SYS_CCSIDR_EL1), access_ccsidr },
- { SYS_DESC(SYS_CLIDR_EL1), access_clidr },
+ { SYS_DESC(SYS_CLIDR_EL1), access_clidr, reset_clidr, CLIDR_EL1,
+ .set_user = set_clidr },
{ SYS_DESC(SYS_CCSIDR2_EL1), undef_access },
{ SYS_DESC(SYS_SMIDR_EL1), undef_access },
{ SYS_DESC(SYS_CSSELR_EL1), access_csselr, reset_unknown, CSSELR_EL1 },
@@ -2622,7 +2733,6 @@ id_to_sys_reg_desc(struct kvm_vcpu *vcpu, u64 id,
FUNCTION_INVARIANT(midr_el1)
FUNCTION_INVARIANT(revidr_el1)
-FUNCTION_INVARIANT(clidr_el1)
FUNCTION_INVARIANT(aidr_el1)
static void get_ctr_el0(struct kvm_vcpu *v, const struct sys_reg_desc *r)
@@ -2634,7 +2744,6 @@ static void get_ctr_el0(struct kvm_vcpu *v, const struct sys_reg_desc *r)
static struct sys_reg_desc invariant_sys_regs[] = {
{ SYS_DESC(SYS_MIDR_EL1), NULL, get_midr_el1 },
{ SYS_DESC(SYS_REVIDR_EL1), NULL, get_revidr_el1 },
- { SYS_DESC(SYS_CLIDR_EL1), NULL, get_clidr_el1 },
{ SYS_DESC(SYS_AIDR_EL1), NULL, get_aidr_el1 },
{ SYS_DESC(SYS_CTR_EL0), NULL, get_ctr_el0 },
};
@@ -2671,33 +2780,7 @@ static int set_invariant_sys_reg(u64 id, u64 __user *uaddr)
return 0;
}
-static bool is_valid_cache(u32 val)
-{
- u32 level, ctype;
-
- if (val >= CSSELR_MAX)
- return false;
-
- /* Bottom bit is Instruction or Data bit. Next 3 bits are level. */
- level = (val >> 1);
- ctype = (cache_levels >> (level * 3)) & 7;
-
- switch (ctype) {
- case 0: /* No cache */
- return false;
- case 1: /* Instruction cache only */
- return (val & 1);
- case 2: /* Data cache only */
- case 4: /* Unified cache */
- return !(val & 1);
- case 3: /* Separate instruction and data caches */
- return true;
- default: /* Reserved: we can't know instruction or data. */
- return false;
- }
-}
-
-static int demux_c15_get(u64 id, void __user *uaddr)
+static int demux_c15_get(struct kvm_vcpu *vcpu, u64 id, void __user *uaddr)
{
u32 val;
u32 __user *uval = uaddr;
@@ -2713,16 +2796,16 @@ static int demux_c15_get(u64 id, void __user *uaddr)
return -ENOENT;
val = (id & KVM_REG_ARM_DEMUX_VAL_MASK)
>> KVM_REG_ARM_DEMUX_VAL_SHIFT;
- if (!is_valid_cache(val))
+ if (val >= CSSELR_MAX)
return -ENOENT;
- return put_user(get_ccsidr(val), uval);
+ return put_user(get_ccsidr(vcpu, val), uval);
default:
return -ENOENT;
}
}
-static int demux_c15_set(u64 id, void __user *uaddr)
+static int demux_c15_set(struct kvm_vcpu *vcpu, u64 id, void __user *uaddr)
{
u32 val, newval;
u32 __user *uval = uaddr;
@@ -2738,16 +2821,13 @@ static int demux_c15_set(u64 id, void __user *uaddr)
return -ENOENT;
val = (id & KVM_REG_ARM_DEMUX_VAL_MASK)
>> KVM_REG_ARM_DEMUX_VAL_SHIFT;
- if (!is_valid_cache(val))
+ if (val >= CSSELR_MAX)
return -ENOENT;
if (get_user(newval, uval))
return -EFAULT;
- /* This is also invariant: you can't change it. */
- if (newval != get_ccsidr(val))
- return -EINVAL;
- return 0;
+ return set_ccsidr(vcpu, val, newval);
default:
return -ENOENT;
}
@@ -2784,7 +2864,7 @@ int kvm_arm_sys_reg_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg
int err;
if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_DEMUX)
- return demux_c15_get(reg->id, uaddr);
+ return demux_c15_get(vcpu, reg->id, uaddr);
err = get_invariant_sys_reg(reg->id, uaddr);
if (err != -ENOENT)
@@ -2828,7 +2908,7 @@ int kvm_arm_sys_reg_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg
int err;
if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_DEMUX)
- return demux_c15_set(reg->id, uaddr);
+ return demux_c15_set(vcpu, reg->id, uaddr);
err = set_invariant_sys_reg(reg->id, uaddr);
if (err != -ENOENT)
@@ -2840,13 +2920,7 @@ int kvm_arm_sys_reg_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg
static unsigned int num_demux_regs(void)
{
- unsigned int i, count = 0;
-
- for (i = 0; i < CSSELR_MAX; i++)
- if (is_valid_cache(i))
- count++;
-
- return count;
+ return CSSELR_MAX;
}
static int write_demux_regids(u64 __user *uindices)
@@ -2856,8 +2930,6 @@ static int write_demux_regids(u64 __user *uindices)
val |= KVM_REG_ARM_DEMUX_ID_CCSIDR;
for (i = 0; i < CSSELR_MAX; i++) {
- if (!is_valid_cache(i))
- continue;
if (put_user(val | i, uindices))
return -EFAULT;
uindices++;
@@ -2959,7 +3031,6 @@ int kvm_sys_reg_table_init(void)
{
bool valid = true;
unsigned int i;
- struct sys_reg_desc clidr;
/* Make sure tables are unique and in order. */
valid &= check_sysreg_table(sys_reg_descs, ARRAY_SIZE(sys_reg_descs), false);
@@ -2976,23 +3047,5 @@ int 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]);
- /*
- * CLIDR format is awkward, so clean it up. See ARM B4.1.20:
- *
- * If software reads the Cache Type fields from Ctype1
- * upwards, once it has seen a value of 0b000, no caches
- * exist at further-out levels of the hierarchy. So, for
- * example, if Ctype3 is the first Cache Type field with a
- * value of 0b000, the values of Ctype4 to Ctype7 must be
- * ignored.
- */
- get_clidr_el1(NULL, &clidr); /* Ugly... */
- cache_levels = clidr.val;
- for (i = 0; i < 7; i++)
- if (((cache_levels >> (i*3)) & 7) == 0)
- break;
- /* Clear all higher bits. */
- cache_levels &= (1 << (i*3))-1;
-
return 0;
}
--
2.38.1
The macros are useful for KVM which needs to manage how CLIDR is exposed
to vcpu so move them to include/asm/cache.h, which KVM can refer to.
Signed-off-by: Akihiko Odaki <[email protected]>
---
arch/arm64/include/asm/cache.h | 6 ++++++
arch/arm64/kernel/cacheinfo.c | 5 -----
2 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/arch/arm64/include/asm/cache.h b/arch/arm64/include/asm/cache.h
index c0b178d1bb4f..ab7133654a72 100644
--- a/arch/arm64/include/asm/cache.h
+++ b/arch/arm64/include/asm/cache.h
@@ -16,6 +16,12 @@
#define CLIDR_LOC(clidr) (((clidr) >> CLIDR_LOC_SHIFT) & 0x7)
#define CLIDR_LOUIS(clidr) (((clidr) >> CLIDR_LOUIS_SHIFT) & 0x7)
+/* Ctypen, bits[3(n - 1) + 2 : 3(n - 1)], for n = 1 to 7 */
+#define CLIDR_CTYPE_SHIFT(level) (3 * (level - 1))
+#define CLIDR_CTYPE_MASK(level) (7 << CLIDR_CTYPE_SHIFT(level))
+#define CLIDR_CTYPE(clidr, level) \
+ (((clidr) & CLIDR_CTYPE_MASK(level)) >> CLIDR_CTYPE_SHIFT(level))
+
/*
* Memory returned by kmalloc() may be used for DMA, so we must make
* sure that all such allocations are cache aligned. Otherwise,
diff --git a/arch/arm64/kernel/cacheinfo.c b/arch/arm64/kernel/cacheinfo.c
index 97c42be71338..daa7b3f55997 100644
--- a/arch/arm64/kernel/cacheinfo.c
+++ b/arch/arm64/kernel/cacheinfo.c
@@ -11,11 +11,6 @@
#include <linux/of.h>
#define MAX_CACHE_LEVEL 7 /* Max 7 level supported */
-/* Ctypen, bits[3(n - 1) + 2 : 3(n - 1)], for n = 1 to 7 */
-#define CLIDR_CTYPE_SHIFT(level) (3 * (level - 1))
-#define CLIDR_CTYPE_MASK(level) (7 << CLIDR_CTYPE_SHIFT(level))
-#define CLIDR_CTYPE(clidr, level) \
- (((clidr) & CLIDR_CTYPE_MASK(level)) >> CLIDR_CTYPE_SHIFT(level))
int cache_line_size(void)
{
--
2.38.1
CCSIDR2_EL1 is available if FEAT_CCIDX is implemented as per
DDI0487I.a.
Signed-off-by: Akihiko Odaki <[email protected]>
Reviewed-by: Mark Brown <[email protected]>
---
arch/arm64/tools/sysreg | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/arch/arm64/tools/sysreg b/arch/arm64/tools/sysreg
index 097d6faafc87..01d592cbc0ba 100644
--- a/arch/arm64/tools/sysreg
+++ b/arch/arm64/tools/sysreg
@@ -899,6 +899,11 @@ Field 5:3 Ctype2
Field 2:0 Ctype1
EndSysreg
+Sysreg CCSIDR2_EL1 3 1 0 0 2
+Res0 63:24
+Field 23:0 NumSets
+EndSysreg
+
Sysreg GMID_EL1 3 1 0 0 4
Res0 63:4
Field 3:0 BS
--
2.38.1
Always set HCR_TID2 to trap CTR_EL0, CCSIDR2_EL1, CLIDR_EL1, and
CSSELR_EL1. This saves a few lines of code and allows to employ their
access trap handlers for more purposes anticipated by the old
condition for setting HCR_TID2.
Suggested-by: Marc Zyngier <[email protected]>
Signed-off-by: Akihiko Odaki <[email protected]>
---
arch/arm64/include/asm/kvm_arm.h | 3 ++-
arch/arm64/include/asm/kvm_emulate.h | 4 ----
arch/arm64/include/asm/kvm_host.h | 2 --
arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h | 2 --
4 files changed, 2 insertions(+), 9 deletions(-)
diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
index 8aa8492dafc0..44be46c280c1 100644
--- a/arch/arm64/include/asm/kvm_arm.h
+++ b/arch/arm64/include/asm/kvm_arm.h
@@ -81,11 +81,12 @@
* SWIO: Turn set/way invalidates into set/way clean+invalidate
* PTW: Take a stage2 fault if a stage1 walk steps in device memory
* TID3: Trap EL1 reads of group 3 ID registers
+ * TID2: Trap CTR_EL0, CCSIDR2_EL1, CLIDR_EL1, and CSSELR_EL1
*/
#define HCR_GUEST_FLAGS (HCR_TSC | HCR_TSW | HCR_TWE | HCR_TWI | HCR_VM | \
HCR_BSU_IS | HCR_FB | HCR_TACR | \
HCR_AMO | HCR_SWIO | HCR_TIDCP | HCR_RW | HCR_TLOR | \
- HCR_FMO | HCR_IMO | HCR_PTW | HCR_TID3 )
+ HCR_FMO | HCR_IMO | HCR_PTW | HCR_TID3 | HCR_TID2)
#define HCR_VIRT_EXCP_MASK (HCR_VSE | HCR_VI | HCR_VF)
#define HCR_HOST_NVHE_FLAGS (HCR_RW | HCR_API | HCR_APK | HCR_ATA)
#define HCR_HOST_NVHE_PROTECTED_FLAGS (HCR_HOST_NVHE_FLAGS | HCR_TSC)
diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
index 9bdba47f7e14..30c4598d643b 100644
--- a/arch/arm64/include/asm/kvm_emulate.h
+++ b/arch/arm64/include/asm/kvm_emulate.h
@@ -88,10 +88,6 @@ static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
if (vcpu_el1_is_32bit(vcpu))
vcpu->arch.hcr_el2 &= ~HCR_RW;
- if (cpus_have_const_cap(ARM64_MISMATCHED_CACHE_TYPE) ||
- vcpu_el1_is_32bit(vcpu))
- vcpu->arch.hcr_el2 |= HCR_TID2;
-
if (kvm_has_mte(vcpu->kvm))
vcpu->arch.hcr_el2 |= HCR_ATA;
}
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 45e2136322ba..cc2ede0eaed4 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -621,7 +621,6 @@ static inline bool __vcpu_read_sys_reg_from_cpu(int reg, u64 *val)
return false;
switch (reg) {
- case CSSELR_EL1: *val = read_sysreg_s(SYS_CSSELR_EL1); break;
case SCTLR_EL1: *val = read_sysreg_s(SYS_SCTLR_EL12); break;
case CPACR_EL1: *val = read_sysreg_s(SYS_CPACR_EL12); break;
case TTBR0_EL1: *val = read_sysreg_s(SYS_TTBR0_EL12); break;
@@ -666,7 +665,6 @@ static inline bool __vcpu_write_sys_reg_to_cpu(u64 val, int reg)
return false;
switch (reg) {
- case CSSELR_EL1: write_sysreg_s(val, SYS_CSSELR_EL1); break;
case SCTLR_EL1: write_sysreg_s(val, SYS_SCTLR_EL12); break;
case CPACR_EL1: write_sysreg_s(val, SYS_CPACR_EL12); break;
case TTBR0_EL1: write_sysreg_s(val, SYS_TTBR0_EL12); break;
diff --git a/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h b/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
index baa5b9b3dde5..147cb4c846c6 100644
--- a/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
+++ b/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
@@ -39,7 +39,6 @@ static inline bool ctxt_has_mte(struct kvm_cpu_context *ctxt)
static inline void __sysreg_save_el1_state(struct kvm_cpu_context *ctxt)
{
- ctxt_sys_reg(ctxt, CSSELR_EL1) = read_sysreg(csselr_el1);
ctxt_sys_reg(ctxt, SCTLR_EL1) = read_sysreg_el1(SYS_SCTLR);
ctxt_sys_reg(ctxt, CPACR_EL1) = read_sysreg_el1(SYS_CPACR);
ctxt_sys_reg(ctxt, TTBR0_EL1) = read_sysreg_el1(SYS_TTBR0);
@@ -95,7 +94,6 @@ static inline void __sysreg_restore_user_state(struct kvm_cpu_context *ctxt)
static inline void __sysreg_restore_el1_state(struct kvm_cpu_context *ctxt)
{
write_sysreg(ctxt_sys_reg(ctxt, MPIDR_EL1), vmpidr_el2);
- write_sysreg(ctxt_sys_reg(ctxt, CSSELR_EL1), csselr_el1);
if (has_vhe() ||
!cpus_have_final_cap(ARM64_WORKAROUND_SPECULATIVE_AT)) {
--
2.38.1
From: Marc Zyngier <[email protected]>
The CCSIDR_EL1 register contains an UNKNOWN field (which replaces
fields that were actually defined in previous revisions of the
architecture).
Define an 'Unkn' field type modeled after the Res0/Res1 types
to allow such description. This allows the generation of
#define CCSIDR_EL1_UNKN (UL(0) | GENMASK_ULL(31, 28))
which may have its use one day. Hopefully the architecture doesn't
add too many of those in the future.
Signed-off-by: Marc Zyngier <[email protected]>
Signed-off-by: Akihiko Odaki <[email protected]>
Reviewed-by: Mark Brown <[email protected]>
---
arch/arm64/tools/gen-sysreg.awk | 20 +++++++++++++++++++-
arch/arm64/tools/sysreg | 2 ++
2 files changed, 21 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/tools/gen-sysreg.awk b/arch/arm64/tools/gen-sysreg.awk
index db461921d256..f6909a6b8380 100755
--- a/arch/arm64/tools/gen-sysreg.awk
+++ b/arch/arm64/tools/gen-sysreg.awk
@@ -98,6 +98,7 @@ END {
res0 = "UL(0)"
res1 = "UL(0)"
+ unkn = "UL(0)"
next_bit = 63
@@ -112,11 +113,13 @@ END {
define(reg "_RES0", "(" res0 ")")
define(reg "_RES1", "(" res1 ")")
+ define(reg "_UNKN", "(" unkn ")")
print ""
reg = null
res0 = null
res1 = null
+ unkn = null
next
}
@@ -134,6 +137,7 @@ END {
res0 = "UL(0)"
res1 = "UL(0)"
+ unkn = "UL(0)"
define("REG_" reg, "S" op0 "_" op1 "_C" crn "_C" crm "_" op2)
define("SYS_" reg, "sys_reg(" op0 ", " op1 ", " crn ", " crm ", " op2 ")")
@@ -161,7 +165,9 @@ END {
define(reg "_RES0", "(" res0 ")")
if (res1 != null)
define(reg "_RES1", "(" res1 ")")
- if (res0 != null || res1 != null)
+ if (unkn != null)
+ define(reg "_UNKN", "(" unkn ")")
+ if (res0 != null || res1 != null || unkn != null)
print ""
reg = null
@@ -172,6 +178,7 @@ END {
op2 = null
res0 = null
res1 = null
+ unkn = null
next
}
@@ -190,6 +197,7 @@ END {
next_bit = 0
res0 = null
res1 = null
+ unkn = null
next
}
@@ -215,6 +223,16 @@ END {
next
}
+/^Unkn/ && (block == "Sysreg" || block == "SysregFields") {
+ expect_fields(2)
+ parse_bitdef(reg, "UNKN", $2)
+ field = "UNKN_" msb "_" lsb
+
+ unkn = unkn " | GENMASK_ULL(" msb ", " lsb ")"
+
+ next
+}
+
/^Field/ && (block == "Sysreg" || block == "SysregFields") {
expect_fields(3)
field = $3
diff --git a/arch/arm64/tools/sysreg b/arch/arm64/tools/sysreg
index 384757a7eda9..8f26fe1bedc6 100644
--- a/arch/arm64/tools/sysreg
+++ b/arch/arm64/tools/sysreg
@@ -15,6 +15,8 @@
# Res1 <msb>[:<lsb>]
+# Unkn <msb>[:<lsb>]
+
# Field <msb>[:<lsb>] <name>
# Enum <msb>[:<lsb>] <name>
--
2.38.1
Hi Akihiko,
On Fri, Dec 30, 2022 at 06:54:52PM +0900, Akihiko Odaki wrote:
[...]
> @@ -417,6 +418,9 @@ struct kvm_vcpu_arch {
> u64 last_steal;
> gpa_t base;
> } steal;
> +
> + /* Per-vcpu CCSIDR override or NULL */
> + u32 *ccsidr;
I don't believe we need to store this per-vCPU. Of course, it is
possible to userspace to observe heterogeneous cache topologies
depending on what core the KVM_GET_ONE_REG ioctl is handled on.
WDYT about keeping track of this per-VM? The value should be whatever
was last written by userspace. To avoid breaking userspace this would
also need to allow mismatched writes (i.e. vCPU0 has a different
configuration than vCPU1).
[...]
> +static u8 get_min_cache_line_size(u32 csselr)
It would be nice to have a comment indicating that it returns
Log2(line_size), not line size outright.
> +{
> + u64 ctr_el0;
> + int field;
> +
> + ctr_el0 = read_sanitised_ftr_reg(SYS_CTR_EL0);
> + field = csselr & CSSELR_EL1_InD ? CTR_EL0_IminLine_SHIFT : CTR_EL0_DminLine_SHIFT;
> +
> + return cpuid_feature_extract_unsigned_field(ctr_el0, field) - 2;
This probably deserves a comment describing that CTR_EL0 represents line
size in units of words, not bytes. Furthermore, a subtle reminder on the
log transformation being done here would help also.
> +}
> +
> /* Which cache CCSIDR represents depends on CSSELR value. */
> -static u32 get_ccsidr(u32 csselr)
> +static u32 get_ccsidr(struct kvm_vcpu *vcpu, u32 csselr)
> {
> - u32 ccsidr;
> + if (vcpu->arch.ccsidr)
> + return vcpu->arch.ccsidr[csselr];
>
> - /* Make sure noone else changes CSSELR during this! */
> - local_irq_disable();
> - write_sysreg(csselr, csselr_el1);
> - isb();
> - ccsidr = read_sysreg(ccsidr_el1);
> - local_irq_enable();
> + /*
> + * Fabricate a CCSIDR value as the overriding value does not exist.
> + * The real CCSIDR value will not be used as it can vary by the
> + * physical CPU which the vcpu currently resides in.
> + *
> + * The line size is determined with get_min_cache_line_size(), which
> + * should be valid for all CPUs even if they have different cache
> + * configuration.
> + *
> + * The associativity bits are cleared, meaning the geometry of all data
> + * and unified caches (which are guaranteed to be PIPT and thus
> + * non-aliasing) are 1 set and 1 way.
> + * Guests should not be doing cache operations by set/way at all, and
> + * for this reason, we trap them and attempt to infer the intent, so
> + * that we can flush the entire guest's address space at the appropriate
> + * time. The exposed geometry minimizes the number of the traps.
> + * [If guests should attempt to infer aliasing properties from the
> + * geometry (which is not permitted by the architecture), they would
> + * only do so for virtually indexed caches.]
> + *
> + * We don't check if the cache level exists as it is allowed to return
> + * an UNKNOWN value if not.
> + */
> + return get_min_cache_line_size(csselr) << CCSIDR_EL1_LineSize_SHIFT;
I don't believe this is correct. The value of CCSIDR_EL1.LineSize is
actually:
Log2(Number of bytes in cache line) - 4 (from DDI0487I.a D17.2.26)
With that, I'd expect the following instead:
line_size = get_min_cache_line_size(csselr);
return FIELD_PREP(CCSIDR_EL1_LineSize_MASK, line_size - 4);
--
Thanks,
Oliver
On Fri, Dec 30, 2022 at 06:54:51PM +0900, Akihiko Odaki wrote:
> The CCSIDR access handler masks the associativity bits according to the
> bit layout for processors without FEAT_CCIDX. KVM also assumes CCSIDR is
> 32-bit where it will be 64-bit if FEAT_CCIDX is enabled. Mask FEAT_CCIDX
> so that these assumptions hold.
>
> Suggested-by: Marc Zyngier <[email protected]>
> Signed-off-by: Akihiko Odaki <[email protected]>
FYI, I'm an idiot and replied to v4 of this patch... Forwarding comments
below:
> ---
> arch/arm64/kvm/sys_regs.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index f4a7c5abcbca..aeabf1f3370b 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1124,6 +1124,12 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu, struct sys_reg_desc const *r
> ID_DFR0_PERFMON_SHIFT,
> kvm_vcpu_has_pmu(vcpu) ? ID_DFR0_PERFMON_8_4 : 0);
> break;
> + case SYS_ID_AA64MMFR2_EL1:
> + val &= ~ID_AA64MMFR2_EL1_CCIDX_MASK;
> + break;
> + case SYS_ID_MMFR4_EL1:
> + val &= ~ARM64_FEATURE_MASK(ID_MMFR4_CCIDX);
> + break;
Not that it is necessarily worth addressing, but I wanted to point
something out.
This change breaks migration from older kernels on implementations w/
FEAT_CCIDX. There is most likely exactly 0 of those in the wild, but
we need to be careful changing user-visible stuff like this.
--
Thanks,
Oliver
On Fri, Dec 30, 2022 at 1:55 AM Akihiko Odaki <[email protected]> wrote:
>
> Always set HCR_TID2 to trap CTR_EL0, CCSIDR2_EL1, CLIDR_EL1, and
> CSSELR_EL1. This saves a few lines of code and allows to employ their
> access trap handlers for more purposes anticipated by the old
> condition for setting HCR_TID2.
>
> Suggested-by: Marc Zyngier <[email protected]>
> Signed-off-by: Akihiko Odaki <[email protected]>
> ---
> arch/arm64/include/asm/kvm_arm.h | 3 ++-
> arch/arm64/include/asm/kvm_emulate.h | 4 ----
> arch/arm64/include/asm/kvm_host.h | 2 --
> arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h | 2 --
> 4 files changed, 2 insertions(+), 9 deletions(-)
>
> diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
> index 8aa8492dafc0..44be46c280c1 100644
> --- a/arch/arm64/include/asm/kvm_arm.h
> +++ b/arch/arm64/include/asm/kvm_arm.h
> @@ -81,11 +81,12 @@
> * SWIO: Turn set/way invalidates into set/way clean+invalidate
> * PTW: Take a stage2 fault if a stage1 walk steps in device memory
> * TID3: Trap EL1 reads of group 3 ID registers
> + * TID2: Trap CTR_EL0, CCSIDR2_EL1, CLIDR_EL1, and CSSELR_EL1
> */
> #define HCR_GUEST_FLAGS (HCR_TSC | HCR_TSW | HCR_TWE | HCR_TWI | HCR_VM | \
> HCR_BSU_IS | HCR_FB | HCR_TACR | \
> HCR_AMO | HCR_SWIO | HCR_TIDCP | HCR_RW | HCR_TLOR | \
> - HCR_FMO | HCR_IMO | HCR_PTW | HCR_TID3 )
> + HCR_FMO | HCR_IMO | HCR_PTW | HCR_TID3 | HCR_TID2)
> #define HCR_VIRT_EXCP_MASK (HCR_VSE | HCR_VI | HCR_VF)
> #define HCR_HOST_NVHE_FLAGS (HCR_RW | HCR_API | HCR_APK | HCR_ATA)
> #define HCR_HOST_NVHE_PROTECTED_FLAGS (HCR_HOST_NVHE_FLAGS | HCR_TSC)
> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> index 9bdba47f7e14..30c4598d643b 100644
> --- a/arch/arm64/include/asm/kvm_emulate.h
> +++ b/arch/arm64/include/asm/kvm_emulate.h
> @@ -88,10 +88,6 @@ static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
> if (vcpu_el1_is_32bit(vcpu))
> vcpu->arch.hcr_el2 &= ~HCR_RW;
>
> - if (cpus_have_const_cap(ARM64_MISMATCHED_CACHE_TYPE) ||
> - vcpu_el1_is_32bit(vcpu))
> - vcpu->arch.hcr_el2 |= HCR_TID2;
> -
> if (kvm_has_mte(vcpu->kvm))
> vcpu->arch.hcr_el2 |= HCR_ATA;
> }
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 45e2136322ba..cc2ede0eaed4 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -621,7 +621,6 @@ static inline bool __vcpu_read_sys_reg_from_cpu(int reg, u64 *val)
> return false;
>
> switch (reg) {
> - case CSSELR_EL1: *val = read_sysreg_s(SYS_CSSELR_EL1); break;
> case SCTLR_EL1: *val = read_sysreg_s(SYS_SCTLR_EL12); break;
> case CPACR_EL1: *val = read_sysreg_s(SYS_CPACR_EL12); break;
> case TTBR0_EL1: *val = read_sysreg_s(SYS_TTBR0_EL12); break;
> @@ -666,7 +665,6 @@ static inline bool __vcpu_write_sys_reg_to_cpu(u64 val, int reg)
> return false;
>
> switch (reg) {
> - case CSSELR_EL1: write_sysreg_s(val, SYS_CSSELR_EL1); break;
> case SCTLR_EL1: write_sysreg_s(val, SYS_SCTLR_EL12); break;
> case CPACR_EL1: write_sysreg_s(val, SYS_CPACR_EL12); break;
> case TTBR0_EL1: write_sysreg_s(val, SYS_TTBR0_EL12); break;
> diff --git a/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h b/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
> index baa5b9b3dde5..147cb4c846c6 100644
> --- a/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
> +++ b/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
> @@ -39,7 +39,6 @@ static inline bool ctxt_has_mte(struct kvm_cpu_context *ctxt)
>
> static inline void __sysreg_save_el1_state(struct kvm_cpu_context *ctxt)
> {
> - ctxt_sys_reg(ctxt, CSSELR_EL1) = read_sysreg(csselr_el1);
> ctxt_sys_reg(ctxt, SCTLR_EL1) = read_sysreg_el1(SYS_SCTLR);
> ctxt_sys_reg(ctxt, CPACR_EL1) = read_sysreg_el1(SYS_CPACR);
> ctxt_sys_reg(ctxt, TTBR0_EL1) = read_sysreg_el1(SYS_TTBR0);
> @@ -95,7 +94,6 @@ static inline void __sysreg_restore_user_state(struct kvm_cpu_context *ctxt)
> static inline void __sysreg_restore_el1_state(struct kvm_cpu_context *ctxt)
> {
> write_sysreg(ctxt_sys_reg(ctxt, MPIDR_EL1), vmpidr_el2);
> - write_sysreg(ctxt_sys_reg(ctxt, CSSELR_EL1), csselr_el1);
>
> if (has_vhe() ||
> !cpus_have_final_cap(ARM64_WORKAROUND_SPECULATIVE_AT)) {
> --
Reviewed-by: Reiji Watanabe <[email protected]>
Nit: access_csselr() can now directly use __vcpu_sys_reg() (instead
of using it through via vcpu_{write,read}_sys_reg()), as most codes
do when there is no need to use vcpu_{write,read}_sys_reg().
The same comment is applied to access_ccsidr(), which uses
vcpu_read_sys_reg() to get CSSELR_EL1 value for the vCPU.
Thank you,
Reiji
On 2023/01/06 7:22, Oliver Upton wrote:
> On Fri, Dec 30, 2022 at 06:54:51PM +0900, Akihiko Odaki wrote:
>> The CCSIDR access handler masks the associativity bits according to the
>> bit layout for processors without FEAT_CCIDX. KVM also assumes CCSIDR is
>> 32-bit where it will be 64-bit if FEAT_CCIDX is enabled. Mask FEAT_CCIDX
>> so that these assumptions hold.
>>
>> Suggested-by: Marc Zyngier <[email protected]>
>> Signed-off-by: Akihiko Odaki <[email protected]>
>
> FYI, I'm an idiot and replied to v4 of this patch... Forwarding comments
> below:
>
>> ---
>> arch/arm64/kvm/sys_regs.c | 11 +++++++++++
>> 1 file changed, 11 insertions(+)
>>
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index f4a7c5abcbca..aeabf1f3370b 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -1124,6 +1124,12 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu, struct sys_reg_desc const *r
>> ID_DFR0_PERFMON_SHIFT,
>> kvm_vcpu_has_pmu(vcpu) ? ID_DFR0_PERFMON_8_4 : 0);
>> break;
>> + case SYS_ID_AA64MMFR2_EL1:
>> + val &= ~ID_AA64MMFR2_EL1_CCIDX_MASK;
>> + break;
>> + case SYS_ID_MMFR4_EL1:
>> + val &= ~ARM64_FEATURE_MASK(ID_MMFR4_CCIDX);
>> + break;
>
> Not that it is necessarily worth addressing, but I wanted to point
> something out.
>
> This change breaks migration from older kernels on implementations w/
> FEAT_CCIDX. There is most likely exactly 0 of those in the wild, but
> we need to be careful changing user-visible stuff like this.
>
> --
> Thanks,
> Oliver
I also don't think whether FEAT_CCIDX is visible matters for any guest
because the line size a guest would care is held in the same bits
whether FEAT_CCIDX is implemented. But if it concerns you I can write a
bit more code so that it won't mask CCIDX bit if it's set from the
userspace.
Regards,
Akihiko Odaki
On 2023/01/06 5:45, Oliver Upton wrote:
> Hi Akihiko,
>
> On Fri, Dec 30, 2022 at 06:54:52PM +0900, Akihiko Odaki wrote:
>
> [...]
>
>> @@ -417,6 +418,9 @@ struct kvm_vcpu_arch {
>> u64 last_steal;
>> gpa_t base;
>> } steal;
>> +
>> + /* Per-vcpu CCSIDR override or NULL */
>> + u32 *ccsidr;
>
> I don't believe we need to store this per-vCPU. Of course, it is
> possible to userspace to observe heterogeneous cache topologies
> depending on what core the KVM_GET_ONE_REG ioctl is handled on.
>
> WDYT about keeping track of this per-VM? The value should be whatever
> was last written by userspace. To avoid breaking userspace this would
> also need to allow mismatched writes (i.e. vCPU0 has a different
> configuration than vCPU1).
A user would expect KVM_SET_ONE_REG will be per-VM so semantically it
should be per-VM. Also, it should be noted that this change is to allow
migration from older kernels; an older kernel can set different values
for CCSIDR if the host has heterogeneous cache though I think nobody
with big.LITTLE system wants to migrate VMs.
If you care only practicality but semantics, probably you can just
ignore writes to CCSIDR from the userspace. I don't think any guest will
care even if CCSIDR values change after migration. Either way is fine
for me.
>
> [...]
>
>> +static u8 get_min_cache_line_size(u32 csselr)
>
> It would be nice to have a comment indicating that it returns
> Log2(line_size), not line size outright.
>
>> +{
>> + u64 ctr_el0;
>> + int field;
>> +
>> + ctr_el0 = read_sanitised_ftr_reg(SYS_CTR_EL0);
>> + field = csselr & CSSELR_EL1_InD ? CTR_EL0_IminLine_SHIFT : CTR_EL0_DminLine_SHIFT;
>> +
>> + return cpuid_feature_extract_unsigned_field(ctr_el0, field) - 2;
>
> This probably deserves a comment describing that CTR_EL0 represents line
> size in units of words, not bytes. Furthermore, a subtle reminder on the
> log transformation being done here would help also.
>
>> +}
>> +
>> /* Which cache CCSIDR represents depends on CSSELR value. */
>> -static u32 get_ccsidr(u32 csselr)
>> +static u32 get_ccsidr(struct kvm_vcpu *vcpu, u32 csselr)
>> {
>> - u32 ccsidr;
>> + if (vcpu->arch.ccsidr)
>> + return vcpu->arch.ccsidr[csselr];
>>
>> - /* Make sure noone else changes CSSELR during this! */
>> - local_irq_disable();
>> - write_sysreg(csselr, csselr_el1);
>> - isb();
>> - ccsidr = read_sysreg(ccsidr_el1);
>> - local_irq_enable();
>> + /*
>> + * Fabricate a CCSIDR value as the overriding value does not exist.
>> + * The real CCSIDR value will not be used as it can vary by the
>> + * physical CPU which the vcpu currently resides in.
>> + *
>> + * The line size is determined with get_min_cache_line_size(), which
>> + * should be valid for all CPUs even if they have different cache
>> + * configuration.
>> + *
>> + * The associativity bits are cleared, meaning the geometry of all data
>> + * and unified caches (which are guaranteed to be PIPT and thus
>> + * non-aliasing) are 1 set and 1 way.
>> + * Guests should not be doing cache operations by set/way at all, and
>> + * for this reason, we trap them and attempt to infer the intent, so
>> + * that we can flush the entire guest's address space at the appropriate
>> + * time. The exposed geometry minimizes the number of the traps.
>> + * [If guests should attempt to infer aliasing properties from the
>> + * geometry (which is not permitted by the architecture), they would
>> + * only do so for virtually indexed caches.]
>> + *
>> + * We don't check if the cache level exists as it is allowed to return
>> + * an UNKNOWN value if not.
>> + */
>> + return get_min_cache_line_size(csselr) << CCSIDR_EL1_LineSize_SHIFT;
>
> I don't believe this is correct. The value of CCSIDR_EL1.LineSize is
> actually:
>
> Log2(Number of bytes in cache line) - 4 (from DDI0487I.a D17.2.26)
>
>
> With that, I'd expect the following instead:
>
> line_size = get_min_cache_line_size(csselr);
> return FIELD_PREP(CCSIDR_EL1_LineSize_MASK, line_size - 4);
>
> --
> Thanks,
> Oliver
I added a comment to get_min_cache_line_size() with v6. Hopefully it
will clarify the intention.
Regards,
Akihiko Odaki
Akihiko,
On Sat, Jan 07, 2023 at 06:53:28PM +0900, Akihiko Odaki wrote:
> On 2023/01/06 7:22, Oliver Upton wrote:
> > On Fri, Dec 30, 2022 at 06:54:51PM +0900, Akihiko Odaki wrote:
> > > The CCSIDR access handler masks the associativity bits according to the
> > > bit layout for processors without FEAT_CCIDX. KVM also assumes CCSIDR is
> > > 32-bit where it will be 64-bit if FEAT_CCIDX is enabled. Mask FEAT_CCIDX
> > > so that these assumptions hold.
> > >
> > > Suggested-by: Marc Zyngier <[email protected]>
> > > Signed-off-by: Akihiko Odaki <[email protected]>
> >
> > FYI, I'm an idiot and replied to v4 of this patch... Forwarding comments
> > below:
> >
> > > ---
> > > arch/arm64/kvm/sys_regs.c | 11 +++++++++++
> > > 1 file changed, 11 insertions(+)
> > >
> > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > > index f4a7c5abcbca..aeabf1f3370b 100644
> > > --- a/arch/arm64/kvm/sys_regs.c
> > > +++ b/arch/arm64/kvm/sys_regs.c
> > > @@ -1124,6 +1124,12 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu, struct sys_reg_desc const *r
> > > ID_DFR0_PERFMON_SHIFT,
> > > kvm_vcpu_has_pmu(vcpu) ? ID_DFR0_PERFMON_8_4 : 0);
> > > break;
> > > + case SYS_ID_AA64MMFR2_EL1:
> > > + val &= ~ID_AA64MMFR2_EL1_CCIDX_MASK;
> > > + break;
> > > + case SYS_ID_MMFR4_EL1:
> > > + val &= ~ARM64_FEATURE_MASK(ID_MMFR4_CCIDX);
> > > + break;
> >
> > Not that it is necessarily worth addressing, but I wanted to point
> > something out.
> >
> > This change breaks migration from older kernels on implementations w/
> > FEAT_CCIDX. There is most likely exactly 0 of those in the wild, but
> > we need to be careful changing user-visible stuff like this.
> >
> > --
> > Thanks,
> > Oliver
>
> I also don't think whether FEAT_CCIDX is visible matters for any guest
> because the line size a guest would care is held in the same bits whether
> FEAT_CCIDX is implemented. But if it concerns you I can write a bit more
> code so that it won't mask CCIDX bit if it's set from the userspace.
The particular issue I'm alluding to is the fact that KVM treats the ID
registers as invariant. Userspace will save/restore the ID registers for
the VM as part of a migration. Existing kernels advertize FEAT_CCIDX,
whereas kernels with this patch will not. KVM will return an error
(EINVAL) when ID_AA64MMFR2_EL1 is written by userspace.
We've worked around this issue in the past by implementing set_user
calls for ID registers that have changed to tolerate the 'old' value
that KVM advertized.
In any case, it may not be worth addressing given that there are no
implementations in the wild with FEAT_CCIDX. But I wanted to bring it up
on the list for the sake of posterity and also allow anyone to scream
who might be adversely affected by the change.
--
Thanks,
Oliver