Re: [PATCH] KVM: arm64: Always mask CCSIDR associativity bits
Hey Akihiko,
Thanks for having had a look at this. A bunch of comments below.
On Fri, 02 Dec 2022 09:18:56 +,
Akihiko Odaki wrote:
>
> M2 MacBook Air has mismatched CCSIDR associativity bits among physical
> CPUs, which makes the bits a KVM vCPU sees inconsistent when migrating
> among them.
Your machine *does not* have any mismatched CCSIDR. By definition, any
CPU can have any cache hierarchy, and there is no architectural
requirement that they are all the same.
I'd rather you describe this in architectural terms, and simply point
out that KVM exposes the physical topology of the CPU the vcpu runs
on (including across migration, which is a problem), and that
userspace sees some arbitrary topology that has been sampled at boot
time. And both behaviours are a bit wrong in an asymmetric system.
This also break live migration for something that should never be a
concern of non-secure SW.
>
> While it is possible to detect CCSIDR associativity bit mismatches and
> mask them with that condition, it requires mismatch detection and
> increases complexity. Instead, always mask the CCSIDR associativity bits
> to keep the code simple.
Given the above, this paragraph doesn't make much sense.
>
> Also, allow the userspace to overwrite the bits with arbitrary values so
> that it can restore a vCPU state saved with an older kernel.
>
> Signed-off-by: Akihiko Odaki
> Suggested-by: Marc Zyngier
> ---
> arch/arm64/include/asm/kvm_arm.h | 3 +-
> arch/arm64/include/asm/kvm_emulate.h | 4 -
> arch/arm64/include/asm/kvm_host.h| 4 +
> arch/arm64/include/asm/sysreg.h | 3 +
> arch/arm64/kvm/sys_regs.c| 146 ++-
> 5 files changed, 87 insertions(+), 73 deletions(-)
>
> diff --git a/arch/arm64/include/asm/kvm_arm.h
> b/arch/arm64/include/asm/kvm_arm.h
> index 8aa8492dafc0..f69cd96a65ab 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 CCSIDR_EL1
Not only that, but also CTR_EL0, CCSIDR2_EL1, CLIDR_EL1, and
CSSELR_EL1 if the guest is using AArch64, and CCSELR if the guest is
using AArch32.
> */
> #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..cc051cd56179 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -53,6 +53,9 @@
>
> #define KVM_HAVE_MMU_RWLOCK
>
> +/* CSSELR values; used to index KVM_REG_ARM_DEMUX_ID_CCSIDR */
> +#define CSSELR_MAX 14
> +
> /*
> * Mode of operation configurable with kvm-arm.mode early param.
> * See Documentation/admin-guide/kernel-parameters.txt for more information.
> @@ -266,6 +269,7 @@ struct kvm_cpu_context {
> struct user_fpsimd_state fp_regs;
>
> u64 sys_regs[NR_SYS_REGS];
> + u32 ccsidr[CSSELR_MAX + 1];
kvm_cpu_context is the wrong location for this stuff. We use it for
things that get actively context-switched. No such thing here, as this
is RO data as far as the guest is concerned.
Also, it would probably make some sense to only allocate this memory
if the vcpu is not using the default synthesised topology, but
something that userspace has restored.
>
> struct kvm_vcpu *__hyp_running_vcpu;
> };
> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index 7d301700d1a9..0c5f3675b4c2 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -941,6 +941,9 @@
> #define HFGxTR_EL2_nSMPRI_EL1_SHIFT 54
> #define HFGxTR_EL2_nSMPRI_EL1_MASK
[PATCH] KVM: arm64: Always mask CCSIDR associativity bits
M2 MacBook Air has mismatched CCSIDR associativity bits among physical
CPUs, which makes the bits a KVM vCPU sees inconsistent when migrating
among them.
While it is possible to detect CCSIDR associativity bit mismatches and
mask them with that condition, it requires mismatch detection and
increases complexity. Instead, always mask the CCSIDR associativity bits
to keep the code simple.
Also, allow the userspace to overwrite the bits with arbitrary values so
that it can restore a vCPU state saved with an older kernel.
Signed-off-by: Akihiko Odaki
Suggested-by: Marc Zyngier
---
arch/arm64/include/asm/kvm_arm.h | 3 +-
arch/arm64/include/asm/kvm_emulate.h | 4 -
arch/arm64/include/asm/kvm_host.h| 4 +
arch/arm64/include/asm/sysreg.h | 3 +
arch/arm64/kvm/sys_regs.c| 146 ++-
5 files changed, 87 insertions(+), 73 deletions(-)
diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
index 8aa8492dafc0..f69cd96a65ab 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 CCSIDR_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..cc051cd56179 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -53,6 +53,9 @@
#define KVM_HAVE_MMU_RWLOCK
+/* CSSELR values; used to index KVM_REG_ARM_DEMUX_ID_CCSIDR */
+#define CSSELR_MAX 14
+
/*
* Mode of operation configurable with kvm-arm.mode early param.
* See Documentation/admin-guide/kernel-parameters.txt for more information.
@@ -266,6 +269,7 @@ struct kvm_cpu_context {
struct user_fpsimd_state fp_regs;
u64 sys_regs[NR_SYS_REGS];
+ u32 ccsidr[CSSELR_MAX + 1];
struct kvm_vcpu *__hyp_running_vcpu;
};
diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
index 7d301700d1a9..0c5f3675b4c2 100644
--- a/arch/arm64/include/asm/sysreg.h
+++ b/arch/arm64/include/asm/sysreg.h
@@ -941,6 +941,9 @@
#define HFGxTR_EL2_nSMPRI_EL1_SHIFT54
#define HFGxTR_EL2_nSMPRI_EL1_MASK BIT_MASK(HFGxTR_EL2_nSMPRI_EL1_SHIFT)
+/* CCSIDR_EL1 bit definitions */
+#define CCSIDR_EL1_ASSOCIATIVITY_BITS GENMASK(27, 3)
+
#define ARM64_FEATURE_FIELD_BITS 4
/* Create a mask for the feature bits of the specified feature. */
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index f4a7c5abcbca..3518d021d3a0 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -84,24 +84,6 @@ void vcpu_write_sys_reg(struct kvm_vcpu *vcpu, u64 val, int
reg)
/* 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
-
-/* Which cache CCSIDR represents depends on CSSELR value. */
-static u32 get_ccsidr(u32 csselr)
-{
- u32 ccsidr;
-
- /* 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();
-
- return ccsidr;
-}
-
/*
* See note at ARMv7 ARM B1.14.4 (TL;DR: S/W ops are not easily virtualized).
*/
@@ -1300,25 +1282,76 @@ 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);
+ p->regval = vcpu->arch.ctxt.ccsidr[csselr];
-
