Re: [PATCH v2 9/9] KVM: arm/arm64: avoid race by caching MPIDR

[Christoffer Dall <cdall@linaro.org>]

On Wed, Apr 05, 2017 at 10:50:05AM +0200, Andrew Jones wrote:
> On Tue, Apr 04, 2017 at 09:44:39PM +0200, Christoffer Dall wrote:
> > On Fri, Mar 31, 2017 at 06:06:58PM +0200, Andrew Jones wrote:
> > > Cache the MPIDR in the vcpu structure to fix potential races that
> > > can arise between vcpu reset and the extraction of the MPIDR from
> > > the sys-reg array.
> > 
> > I don't understand the race, sorry.
> > 
> > Can you be more specific in where this goes wrong and exactly what this
> > fixes?
> > 
> 
> At the start of kvm_psci_vcpu_on() we look up the vcpu struct of
> the target vcpu by MPIDR.
> 
>  vcpu = kvm_mpidr_to_vcpu(kvm, cpu_id);
> 
> This necessarily comes before the newly added
> 
>  if (!test_and_clear_bit(KVM_REQ_POWER_OFF, &vcpu->requests))
> 
> If another vcpu is trying to PSCI_ON the same target vcpu at the same
> time, but is further along, i.e. already past the test-and-clear and
> even in kvm_reset_vcpu(), then there's a chance it has already called
> into kvm_reset_sys_regs(), which does
> 
>  /* Catch someone adding a register without putting in reset entry. */
>  memset(&vcpu->arch.ctxt.sys_regs, 0x42, sizeof(vcpu->arch.ctxt.sys_regs));

Ah right, I didn't remember that we did this.

> 
> but has not yet called reset_mpidr(). In that case the kvm_mpidr_to_vcpu()
> pointed out above will fail to find the vcpu, which results in the PSCI_ON
> call returning PSCI_RET_INVALID_PARAMS, as can be seen from this snip
> of kvm_psci_vcpu_on()
> 
>  static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
>  {
>         struct kvm *kvm = source_vcpu->kvm;
>         struct kvm_vcpu *vcpu = NULL;
>         struct swait_queue_head *wq;
>         unsigned long cpu_id;
>         unsigned long context_id;
>         phys_addr_t target_pc;
> 
>         cpu_id = vcpu_get_reg(source_vcpu, 1) & MPIDR_HWID_BITMASK;
>         if (vcpu_mode_is_32bit(source_vcpu))
>                 cpu_id &= ~((u32) 0);
> 
>         vcpu = kvm_mpidr_to_vcpu(kvm, cpu_id);
> 
>         /*
>          * Make sure the caller requested a valid CPU and that the CPU is
>          * turned off.
>          */
>         if (!vcpu)
>                 return PSCI_RET_INVALID_PARAMS;
> 
>         if (!test_and_clear_bit(KVM_REQ_POWER_OFF, &vcpu->requests)) {
> ...
> 

Thanks for the explanation.  Could you add this to the commit message as
you respin:

  ...arise between vcpu reset, which fills the entire sys_regs array
  with a temporary value including the MPIDR register, and looking up
  the VCPU based on the MPIDR value.

With that:

Reviewed-by: Christoffer Dall <cdall@linaro.org>

> 
> > Thanks,
> > -Christoffer
> > 
> > > 
> > > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > > ---
> > >  arch/arm/include/asm/kvm_emulate.h   |  2 +-
> > >  arch/arm/include/asm/kvm_host.h      |  3 +++
> > >  arch/arm/kvm/coproc.c                | 20 ++++++++++++--------
> > >  arch/arm64/include/asm/kvm_emulate.h |  2 +-
> > >  arch/arm64/include/asm/kvm_host.h    |  3 +++
> > >  arch/arm64/kvm/sys_regs.c            | 27 ++++++++++++++-------------
> > >  6 files changed, 34 insertions(+), 23 deletions(-)
> > > 
> > > diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
> > > index 9a8a45aaf19a..1b922de46785 100644
> > > --- a/arch/arm/include/asm/kvm_emulate.h
> > > +++ b/arch/arm/include/asm/kvm_emulate.h
> > > @@ -213,7 +213,7 @@ static inline u32 kvm_vcpu_hvc_get_imm(struct kvm_vcpu *vcpu)
> > >  
> > >  static inline unsigned long kvm_vcpu_get_mpidr_aff(struct kvm_vcpu *vcpu)
> > >  {
> > > -	return vcpu_cp15(vcpu, c0_MPIDR) & MPIDR_HWID_BITMASK;
> > > +	return vcpu->arch.vmpidr & MPIDR_HWID_BITMASK;
> > >  }
> > >  
> > >  static inline void kvm_vcpu_set_be(struct kvm_vcpu *vcpu)
> > > diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> > > index 0b8a6d6b3cb3..e0f461f0af67 100644
> > > --- a/arch/arm/include/asm/kvm_host.h
> > > +++ b/arch/arm/include/asm/kvm_host.h
> > > @@ -151,6 +151,9 @@ struct kvm_vcpu_arch {
> > >  	/* The CPU type we expose to the VM */
> > >  	u32 midr;
> > >  
> > > +	/* vcpu MPIDR */
> > > +	u32 vmpidr;
> > > +
> > >  	/* HYP trapping configuration */
> > >  	u32 hcr;
> > >  
> > > diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
> > > index 3e5e4194ef86..c4df7c9c8ddb 100644
> > > --- a/arch/arm/kvm/coproc.c
> > > +++ b/arch/arm/kvm/coproc.c
> > > @@ -101,14 +101,18 @@ int kvm_handle_cp14_access(struct kvm_vcpu *vcpu, struct kvm_run *run)
> > >  
> > >  static void reset_mpidr(struct kvm_vcpu *vcpu, const struct coproc_reg *r)
> > >  {
> > > -	/*
> > > -	 * Compute guest MPIDR. We build a virtual cluster out of the
> > > -	 * vcpu_id, but we read the 'U' bit from the underlying
> > > -	 * hardware directly.
> > > -	 */
> > > -	vcpu_cp15(vcpu, c0_MPIDR) = ((read_cpuid_mpidr() & MPIDR_SMP_BITMASK) |
> > > -				     ((vcpu->vcpu_id >> 2) << MPIDR_LEVEL_BITS) |
> > > -				     (vcpu->vcpu_id & 3));
> > > +	if (!vcpu->arch.vmpidr) {
> > > +		/*
> > > +		 * Compute guest MPIDR. We build a virtual cluster out of the
> > > +		 * vcpu_id, but we read the 'U' bit from the underlying
> > > +		 * hardware directly.
> > > +		 */
> > > +		u32 mpidr = ((read_cpuid_mpidr() & MPIDR_SMP_BITMASK) |
> > > +			     ((vcpu->vcpu_id >> 2) << MPIDR_LEVEL_BITS) |
> > > +			     (vcpu->vcpu_id & 3));
> > > +		vcpu->arch.vmpidr = mpidr;
> > > +	}
> > > +	vcpu_cp15(vcpu, c0_MPIDR) = vcpu->arch.vmpidr;
> > >  }
> > >  
> > >  /* TRM entries A7:4.3.31 A15:4.3.28 - RO WI */
> > > diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> > > index f5ea0ba70f07..c138bb15b507 100644
> > > --- a/arch/arm64/include/asm/kvm_emulate.h
> > > +++ b/arch/arm64/include/asm/kvm_emulate.h
> > > @@ -242,7 +242,7 @@ static inline u8 kvm_vcpu_trap_get_fault_type(const struct kvm_vcpu *vcpu)
> > >  
> > >  static inline unsigned long kvm_vcpu_get_mpidr_aff(struct kvm_vcpu *vcpu)
> > >  {
> > > -	return vcpu_sys_reg(vcpu, MPIDR_EL1) & MPIDR_HWID_BITMASK;
> > > +	return vcpu->arch.vmpidr_el2 & MPIDR_HWID_BITMASK;
> > >  }
> > >  
> > >  static inline void kvm_vcpu_set_be(struct kvm_vcpu *vcpu)
> > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > > index 7057512b3474..268c10d95a79 100644
> > > --- a/arch/arm64/include/asm/kvm_host.h
> > > +++ b/arch/arm64/include/asm/kvm_host.h
> > > @@ -198,6 +198,9 @@ typedef struct kvm_cpu_context kvm_cpu_context_t;
> > >  struct kvm_vcpu_arch {
> > >  	struct kvm_cpu_context ctxt;
> > >  
> > > +	/* vcpu MPIDR */
> > > +	u64 vmpidr_el2;
> > > +
> > >  	/* HYP configuration */
> > >  	u64 hcr_el2;
> > >  	u32 mdcr_el2;
> > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > > index 0e26f8c2b56f..517aed6d8016 100644
> > > --- a/arch/arm64/kvm/sys_regs.c
> > > +++ b/arch/arm64/kvm/sys_regs.c
> > > @@ -431,19 +431,20 @@ static void reset_amair_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> > >  
> > >  static void reset_mpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> > >  {
> > > -	u64 mpidr;
> > > -
> > > -	/*
> > > -	 * Map the vcpu_id into the first three affinity level fields of
> > > -	 * the MPIDR. We limit the number of VCPUs in level 0 due to a
> > > -	 * limitation to 16 CPUs in that level in the ICC_SGIxR registers
> > > -	 * of the GICv3 to be able to address each CPU directly when
> > > -	 * sending IPIs.
> > > -	 */
> > > -	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_sys_reg(vcpu, MPIDR_EL1) = (1ULL << 31) | mpidr;
> > > +	if (!vcpu->arch.vmpidr_el2) {
> > > +		/*
> > > +		 * Map the vcpu_id into the first three affinity level fields
> > > +		 * of the MPIDR. We limit the number of VCPUs in level 0 due to
> > > +		 * a limitation of 16 CPUs in that level in the ICC_SGIxR
> > > +		 * registers of the GICv3, which are used to address each CPU
> > > +		 * directly when sending IPIs.
> > > +		 */
> > > +		u64 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->arch.vmpidr_el2 = (1ULL << 31) | mpidr;
> > > +	}
> > > +	vcpu_sys_reg(vcpu, MPIDR_EL1) = vcpu->arch.vmpidr_el2;
> > >  }
> > >  
> > >  static void reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> > > -- 
> > > 2.9.3
> > >