Re: [PATCH 1/9] KVM: arm/arm64: vgic: Defer touching GICH_VMCR to vcpu_load/put

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 20/03/17 10:58, Christoffer Dall wrote:
> From: Christoffer Dall <cdall@xxxxxxxxxxxxxxx>
> 
> We don't have to save/restore the VMCR on every entry to/from the guest,
> since on GICv2 we can access the control interface from EL1 and on VHE
> systems with GICv3 we can access the control interface from KVM running
> in EL2.
> 
> GICv3 systems without VHE becomes the rare case, which has to
> save/restore the register on each round trip.
> 
> For in-context accesses to the VMCR, meaning emulation code needs access
> to the value of the VCPU that is currently running within vcpu_load
> and vcpu_put will detect that the it should load the VMCR from the
> physical GIC instead of using the cached memory value.  While this extra
> checking adds some code complexity we end up moving code out of the
> critical path.
> 
> Note that userspace accesses may see out-of-date values if the VCPU is
> running while accessing the VGIC state via the KVM device API, but this
> is already the case.
> 
> Signed-off-by: Christoffer Dall <cdall@xxxxxxxxxxxxxxx>
> Signed-off-by: Christoffer Dall <cdall@xxxxxxxxxx>
> ---
>  arch/arm/include/asm/kvm_asm.h   |  3 +++
>  arch/arm/kvm/arm.c               | 11 +++++-----
>  arch/arm64/include/asm/kvm_asm.h |  2 ++
>  include/kvm/arm_vgic.h           |  3 +++
>  virt/kvm/arm/hyp/vgic-v2-sr.c    |  3 ---
>  virt/kvm/arm/hyp/vgic-v3-sr.c    | 14 ++++++++----
>  virt/kvm/arm/vgic/vgic-init.c    | 12 +++++++++++
>  virt/kvm/arm/vgic/vgic-v2.c      | 46 ++++++++++++++++++++++++++++++++++++++--
>  virt/kvm/arm/vgic/vgic-v3.c      | 42 ++++++++++++++++++++++++++++++++++--
>  virt/kvm/arm/vgic/vgic.c         | 22 +++++++++++++++++++
>  virt/kvm/arm/vgic/vgic.h         |  6 ++++++
>  11 files changed, 148 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/arm/include/asm/kvm_asm.h b/arch/arm/include/asm/kvm_asm.h
> index 8ef0538..dd16044 100644
> --- a/arch/arm/include/asm/kvm_asm.h
> +++ b/arch/arm/include/asm/kvm_asm.h
> @@ -75,7 +75,10 @@ extern void __init_stage2_translation(void);
>  extern void __kvm_hyp_reset(unsigned long);
>  
>  extern u64 __vgic_v3_get_ich_vtr_el2(void);
> +extern u64 __vgic_v3_read_vmcr(void);
> +extern void __vgic_v3_write_vmcr(u32 vmcr);
>  extern void __vgic_v3_init_lrs(void);
> +
>  #endif
>  
>  #endif /* __ARM_KVM_ASM_H__ */
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index d775aac..cfdf2f5 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -348,15 +348,14 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>  	vcpu->arch.host_cpu_context = this_cpu_ptr(kvm_host_cpu_state);
>  
>  	kvm_arm_set_running_vcpu(vcpu);
> +
> +	kvm_vgic_load(vcpu);
>  }
>  
>  void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>  {
> -	/*
> -	 * The arch-generic KVM code expects the cpu field of a vcpu to be -1
> -	 * if the vcpu is no longer assigned to a cpu.  This is used for the
> -	 * optimized make_all_cpus_request path.
> -	 */
> +	kvm_vgic_put(vcpu);
> +
>  	vcpu->cpu = -1;
>  
>  	kvm_arm_set_running_vcpu(NULL);
> @@ -630,7 +629,9 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  		 * non-preemptible context.
>  		 */
>  		preempt_disable();
> +
>  		kvm_pmu_flush_hwstate(vcpu);
> +
>  		kvm_timer_flush_hwstate(vcpu);
>  		kvm_vgic_flush_hwstate(vcpu);
>  
> diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
> index ec3553eb..49f99cd 100644
> --- a/arch/arm64/include/asm/kvm_asm.h
> +++ b/arch/arm64/include/asm/kvm_asm.h
> @@ -59,6 +59,8 @@ extern void __kvm_tlb_flush_local_vmid(struct kvm_vcpu *vcpu);
>  extern int __kvm_vcpu_run(struct kvm_vcpu *vcpu);
>  
>  extern u64 __vgic_v3_get_ich_vtr_el2(void);
> +extern u64 __vgic_v3_read_vmcr(void);
> +extern void __vgic_v3_write_vmcr(u32 vmcr);
>  extern void __vgic_v3_init_lrs(void);
>  
>  extern u32 __kvm_get_mdcr_el2(void);
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index c0b3d99..8d7c3a7 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -307,6 +307,9 @@ bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, unsigned int virt_irq);
>  
>  int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu);
>  
> +void kvm_vgic_load(struct kvm_vcpu *vcpu);
> +void kvm_vgic_put(struct kvm_vcpu *vcpu);
> +
>  #define irqchip_in_kernel(k)	(!!((k)->arch.vgic.in_kernel))
>  #define vgic_initialized(k)	((k)->arch.vgic.initialized)
>  #define vgic_ready(k)		((k)->arch.vgic.ready)
> diff --git a/virt/kvm/arm/hyp/vgic-v2-sr.c b/virt/kvm/arm/hyp/vgic-v2-sr.c
> index c8aeb7b..d3d3b9b 100644
> --- a/virt/kvm/arm/hyp/vgic-v2-sr.c
> +++ b/virt/kvm/arm/hyp/vgic-v2-sr.c
> @@ -114,8 +114,6 @@ void __hyp_text __vgic_v2_save_state(struct kvm_vcpu *vcpu)
>  	if (!base)
>  		return;
>  
> -	cpu_if->vgic_vmcr = readl_relaxed(base + GICH_VMCR);
> -
>  	if (vcpu->arch.vgic_cpu.live_lrs) {
>  		cpu_if->vgic_apr = readl_relaxed(base + GICH_APR);
>  
> @@ -165,7 +163,6 @@ void __hyp_text __vgic_v2_restore_state(struct kvm_vcpu *vcpu)
>  		}
>  	}
>  
> -	writel_relaxed(cpu_if->vgic_vmcr, base + GICH_VMCR);
>  	vcpu->arch.vgic_cpu.live_lrs = live_lrs;
>  }
>  
> diff --git a/virt/kvm/arm/hyp/vgic-v3-sr.c b/virt/kvm/arm/hyp/vgic-v3-sr.c
> index 3947095..e51ee7e 100644
> --- a/virt/kvm/arm/hyp/vgic-v3-sr.c
> +++ b/virt/kvm/arm/hyp/vgic-v3-sr.c
> @@ -159,8 +159,6 @@ void __hyp_text __vgic_v3_save_state(struct kvm_vcpu *vcpu)
>  	if (!cpu_if->vgic_sre)
>  		dsb(st);
>  
> -	cpu_if->vgic_vmcr  = read_gicreg(ICH_VMCR_EL2);
> -
>  	if (vcpu->arch.vgic_cpu.live_lrs) {
>  		int i;
>  		u32 max_lr_idx, nr_pri_bits;
> @@ -261,8 +259,6 @@ void __hyp_text __vgic_v3_restore_state(struct kvm_vcpu *vcpu)
>  			live_lrs |= (1 << i);
>  	}
>  
> -	write_gicreg(cpu_if->vgic_vmcr, ICH_VMCR_EL2);
> -
>  	if (live_lrs) {
>  		write_gicreg(cpu_if->vgic_hcr, ICH_HCR_EL2);
>  
> @@ -326,3 +322,13 @@ u64 __hyp_text __vgic_v3_get_ich_vtr_el2(void)
>  {
>  	return read_gicreg(ICH_VTR_EL2);
>  }
> +
> +u64 __hyp_text __vgic_v3_read_vmcr(void)
> +{
> +	return read_gicreg(ICH_VMCR_EL2);
> +}
> +
> +void __hyp_text __vgic_v3_write_vmcr(u32 vmcr)
> +{
> +	write_gicreg(vmcr, ICH_VMCR_EL2);
> +}
> diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
> index 702f810..3762fd1 100644
> --- a/virt/kvm/arm/vgic/vgic-init.c
> +++ b/virt/kvm/arm/vgic/vgic-init.c
> @@ -262,6 +262,18 @@ int vgic_init(struct kvm *kvm)
>  	vgic_debug_init(kvm);
>  
>  	dist->initialized = true;
> +
> +	/*
> +	 * If we're initializing GICv2 on-demand when first running the VCPU
> +	 * then we need to load the VGIC state onto the CPU.  We can detect
> +	 * this easily by checking if we are in between vcpu_load and vcpu_put
> +	 * when we just initialized the VGIC.
> +	 */
> +	preempt_disable();
> +	vcpu = kvm_arm_get_running_vcpu();
> +	if (vcpu)
> +		kvm_vgic_load(vcpu);
> +	preempt_enable();
>  out:
>  	return ret;
>  }
> diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c
> index 94cf4b9..dfe6e5e 100644
> --- a/virt/kvm/arm/vgic/vgic-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-v2.c
> @@ -199,6 +199,9 @@ void vgic_v2_clear_lr(struct kvm_vcpu *vcpu, int lr)
>  
>  void vgic_v2_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp)
>  {
> +	struct vgic_dist *vgic = &vcpu->kvm->arch.vgic;
> +	struct vgic_v2_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v2;
> +	int cpu;
>  	u32 vmcr;
>  
>  	vmcr  = (vmcrp->ctlr << GICH_VMCR_CTRL_SHIFT) & GICH_VMCR_CTRL_MASK;
> @@ -209,12 +212,35 @@ void vgic_v2_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp)
>  	vmcr |= (vmcrp->pmr << GICH_VMCR_PRIMASK_SHIFT) &
>  		GICH_VMCR_PRIMASK_MASK;
>  
> -	vcpu->arch.vgic_cpu.vgic_v2.vgic_vmcr = vmcr;
> +	cpu_if->vgic_vmcr = vmcr;
> +
> +	cpu = get_cpu();
> +	if (vcpu->cpu == cpu)
> +		/*
> +		 * We have run vcpu_load and we're on VHE which means the VMCR
> +		 * has already been flushed to the CPU; flush it again.
> +		 */

I don't understand this comment. What has VHE to do with this?

> +		writel_relaxed(vmcr, vgic->vctrl_base + GICH_VMCR);
> +	put_cpu();

There is one thing I don't quite get: I thought we could only get there
with a userspace access. I understand that we've run vcpu_load()
already, and that we end-up with a stale configuration in the HW. But
why do we need to immediately propagate it? Surely we can rely on the
vcpu thread doing a vcpu_load itself to load the new value, right? I
must be missing something...

>  }
>  
>  void vgic_v2_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp)
>  {
> -	u32 vmcr = vcpu->arch.vgic_cpu.vgic_v2.vgic_vmcr;
> +	struct vgic_dist *vgic = &vcpu->kvm->arch.vgic;
> +	struct vgic_v2_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v2;
> +	int cpu;
> +	u32 vmcr;
> +
> +	cpu = get_cpu();
> +	if (vcpu->cpu == cpu)
> +		/*
> +		 * We have run vcpu_load and which means the VMCR
> +		 * has already been flushed to the CPU; sync it back.
> +		 */
> +		cpu_if->vgic_vmcr = readl_relaxed(vgic->vctrl_base + GICH_VMCR);
> +	put_cpu();
> +
> +	vmcr = cpu_if->vgic_vmcr;

Same comment as above.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux