[RFC] KVM: arm64: Support FEAT_CCIDX

Mon Apr 12 08:49:19 BST 2021

Hi Shaokun,

On Mon, 12 Apr 2021 03:22:03 +0100,
Shaokun Zhang <zhangshaokun at hisilicon.com> wrote:
> 
> Hi Marc,
> 
> On 2021/4/10 17:54, Marc Zyngier wrote:
> > On Sat, 10 Apr 2021 09:16:37 +0100,
> > Shaokun Zhang <zhangshaokun at hisilicon.com> wrote:
> >>
> >> CCSIDR_EL1 can be implemented as 64-bit format inferred by CCIDX field
> >> in ID_AA64MMFR2_EL1. The bits of Numsets and Associativity are different
> >> from the 32-bit format. Let's support this feature.
> >>
> >> Cc: Marc Zyngier <maz at kernel.org>
> >> Cc: James Morse <james.morse at arm.com>
> >> Cc: Alexandru Elisei <alexandru.elisei at arm.com>
> >> Cc: Suzuki K Poulose <suzuki.poulose at arm.com>
> >> Signed-off-by: Shaokun Zhang <zhangshaokun at hisilicon.com>
> >> ---
> >>  arch/arm64/kvm/sys_regs.c | 27 +++++++++++++++++++--------
> >>  1 file changed, 19 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> >> index 52fdb9a015a4..0dc822cef20b 100644
> >> --- a/arch/arm64/kvm/sys_regs.c
> >> +++ b/arch/arm64/kvm/sys_regs.c
> >> @@ -18,6 +18,7 @@
> >>  
> >>  #include <asm/cacheflush.h>
> >>  #include <asm/cputype.h>
> >> +#include <asm/cpufeature.h>
> > 
> > If you are going to add this (why?), at least add it in alphabetic order.
> 
> cpuid_feature_extract_unsigned_field will be used later.
> It shall do in alphabetic order.

We already use this function all over the place, and it is already
dragged in via many other paths. Anyway, that's not the biggest
problem.

> 
> > 
> >>  #include <asm/debug-monitors.h>
> >>  #include <asm/esr.h>
> >>  #include <asm/kvm_arm.h>
> >> @@ -95,9 +96,9 @@ static u32 cache_levels;
> >>  #define CSSELR_MAX 14
> >>  
> >>  /* Which cache CCSIDR represents depends on CSSELR value. */
> >> -static u32 get_ccsidr(u32 csselr)
> >> +static u64 get_ccsidr(u32 csselr)
> >>  {
> >> -	u32 ccsidr;
> >> +	u64 ccsidr;
> >>  
> >>  	/* Make sure noone else changes CSSELR during this! */
> >>  	local_irq_disable();
> >> @@ -1275,12 +1276,16 @@ static bool access_csselr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> >>  static bool access_ccsidr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> >>  			  const struct sys_reg_desc *r)
> >>  {
> >> -	u32 csselr;
> >> +	u32 csselr, ccidx;
> >> +	u64 mmfr2;
> >>  
> >>  	if (p->is_write)
> >>  		return write_to_read_only(vcpu, p, r);
> >>  
> >>  	csselr = vcpu_read_sys_reg(vcpu, CSSELR_EL1);
> >> +	mmfr2 = read_sysreg_s(SYS_ID_AA64MMFR2_EL1);
> >> +	ccidx = cpuid_feature_extract_unsigned_field(mmfr2,
> >> +						     ID_AA64MMFR2_CCIDX_SHIFT);
> > 
> > What happens on an asymmetric system where only some of the CPUs have
> > FEAT_CCIDX?
> 
> If other CPUs don't have FEAT_CCIDX, its value is 0b0000 while
> CCSIDR_EL1 is 32-bit format.
>

You are missing the point: CPU-A has CCIDX, CPU-B doesn't. My vcpu
runs on CPU-A, gets preempted right after the read of
ID_AA64MMFR2_EL1, and then scheduled on CPU-B. You will now compute
the guest view of CCSIDR_EL1 with CPU-B's value, but interpreting it
with CPU-A's configuration. That's totally broken.

> > 
> >>  	p->regval = get_ccsidr(csselr);
> >>  
> >>  	/*
> >> @@ -1295,8 +1300,13 @@ static bool access_ccsidr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> >>  	 * 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);
> >> +	if (!(csselr & 1)) { // data or unified cache
> >> +		if (ccidx)
> >> +			p->regval &= ~(GENMASK(23, 3) | GENMASK(55, 32));
> >> +		else
> >> +			p->regval &= ~GENMASK(27, 3);
> >> +	}
> >> +
> >>  	return true;
> >>  }
> >>  
> >> @@ -2521,7 +2531,7 @@ static bool is_valid_cache(u32 val)
> >>  static int demux_c15_get(u64 id, void __user *uaddr)
> >>  {
> >>  	u32 val;
> >> -	u32 __user *uval = uaddr;
> >> +	u64 __user *uval = uaddr;
> > 
> > What? Has the user API changed while I wasn't looking? Getting CCSIDR
> > can only return a 32bit quantity on AArch32. In fact, CCSIDR is
> > *always* 32bit, CCIDX or not. I have no idea what you are trying to do
> > here, but at best you are now corrupting 32bit of userspace.
> > 
> 
> Oops, I really missed this.
> 
> >>  
> >>  	/* Fail if we have unknown bits set. */
> >>  	if (id & ~(KVM_REG_ARCH_MASK|KVM_REG_SIZE_MASK|KVM_REG_ARM_COPROC_MASK
> >> @@ -2545,8 +2555,9 @@ static int demux_c15_get(u64 id, void __user *uaddr)
> >>  
> >>  static int demux_c15_set(u64 id, void __user *uaddr)
> >>  {
> >> -	u32 val, newval;
> >> -	u32 __user *uval = uaddr;
> >> +	u32 val;
> >> +	u64 newval;
> >> +	u64 __user *uval = uaddr;
> > 
> > Same brokenness.
> > 
> >>  
> >>  	/* Fail if we have unknown bits set. */
> >>  	if (id & ~(KVM_REG_ARCH_MASK|KVM_REG_SIZE_MASK|KVM_REG_ARM_COPROC_MASK
> > 
> > What about CCSIDR2_EL1, which is mandatory when FEAT_CCSIDX is
> > present? How about the AArch32 handling of that register? I don't
> > think you have though this one through.
> 
> To be honest, AArch32 is not considered directly and I sent this only
> as RFC.

If you don't consider AArch32 when writing a patch, please don't send
it. AArch32 guests are fully supported, and a patch that breaks them
isn't acceptable.

Also, a RFC patch doesn't mean things are allowed to break. We use RFC
as way to ask for people feedback on a design, but the proposed
implementation has to be a valid one. That's not a license to send
broken stuff.

> When I wrote some flush cache code using by set/way mode and
> noticed that CCSIDR_EL1 is used here.
>
> > 
> > Another question is: why should we care at all? Why don't we drop all
> > that and only implement a virtual cache topology? A VM shouldn't care
> > at all about this, and we are already lying about the topology anyway.
> > We could even get the VMM to set whatever stupid topology it wants for
> > the sake of it (and to restore previously created VMs).
> 
> Got it,
> 
> Thanks for your detailed comments.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.