Re: [PATCH 3/4] KVM: arm64: vgic-v3: Expose GICR_CTLR.RWP when disabling LPIs

[Oliver Upton <oupton@google.com>]

On Mon, Mar 14, 2022 at 04:40:43PM +0000, Marc Zyngier wrote:
> When disabling LPIs, a guest needs to poll GICR_CTLR.RWP in order
> to be sure that the write has taken effect. We so far reported it
> as 0, as we didn't advertise that LPIs could be turned off the
> first place.
> 
> Start tracking this state during which LPIs are being disabled,
> and expose the 'in progress' state via the RWP bit.
> 
> We also take this opportunity to disallow enabling LPIs and programming
> GICR_{PEND,PROP}BASER while LPI disabling is in progress, as allowed by
> the architecture (UNPRED behaviour).
> 
> We don't advertise the feature to the guest yet (which is allowed by
> the architecture).
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm64/kvm/vgic/vgic-its.c     |  2 +-
>  arch/arm64/kvm/vgic/vgic-mmio-v3.c | 44 ++++++++++++++++++++----------
>  arch/arm64/kvm/vgic/vgic.h         |  1 +
>  include/kvm/arm_vgic.h             |  4 +--
>  4 files changed, 34 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> index cc62d8a8180f..9f51d624730f 100644
> --- a/arch/arm64/kvm/vgic/vgic-its.c
> +++ b/arch/arm64/kvm/vgic/vgic-its.c
> @@ -683,7 +683,7 @@ int vgic_its_resolve_lpi(struct kvm *kvm, struct vgic_its *its,
>  	if (!vcpu)
>  		return E_ITS_INT_UNMAPPED_INTERRUPT;
>  
> -	if (!vcpu->arch.vgic_cpu.lpis_enabled)
> +	if (!vgic_lpis_enabled(vcpu))
>  		return -EBUSY;
>  
>  	vgic_its_cache_translation(kvm, its, devid, eventid, ite->irq);
> diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> index 186bf35078bf..a6be403996c6 100644
> --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> @@ -221,6 +221,13 @@ static void vgic_mmio_write_irouter(struct kvm_vcpu *vcpu,
>  	vgic_put_irq(vcpu->kvm, irq);
>  }
>  
> +bool vgic_lpis_enabled(struct kvm_vcpu *vcpu)
> +{
> +	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
> +
> +	return atomic_read(&vgic_cpu->ctlr) == GICR_CTLR_ENABLE_LPIS;
> +}
> +
>  static unsigned long vgic_mmio_read_v3r_ctlr(struct kvm_vcpu *vcpu,
>  					     gpa_t addr, unsigned int len)
>  {
> @@ -229,26 +236,39 @@ static unsigned long vgic_mmio_read_v3r_ctlr(struct kvm_vcpu *vcpu,
>  	return vgic_cpu->lpis_enabled ? GICR_CTLR_ENABLE_LPIS : 0;
>  }
>  
> -
>  static void vgic_mmio_write_v3r_ctlr(struct kvm_vcpu *vcpu,
>  				     gpa_t addr, unsigned int len,
>  				     unsigned long val)
>  {
>  	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
> -	bool was_enabled = vgic_cpu->lpis_enabled;
> +	u32 ctlr;
>  
>  	if (!vgic_has_its(vcpu->kvm))
>  		return;
>  
> -	vgic_cpu->lpis_enabled = val & GICR_CTLR_ENABLE_LPIS;
> +	if (!(val & GICR_CTLR_ENABLE_LPIS)) {
> +		/*
> +		 * Don't disable if RWP is set, as there already an
> +		 * ongoing disable. Funky guest...
> +		 */
> +		ctlr = atomic_cmpxchg_acquire(&vgic_cpu->ctlr,
> +					      GICR_CTLR_ENABLE_LPIS,
> +					      GICR_CTLR_RWP);
> +		if (ctlr != GICR_CTLR_ENABLE_LPIS)
> +			return;
>  
> -	if (was_enabled && !vgic_cpu->lpis_enabled) {
>  		vgic_flush_pending_lpis(vcpu);
>  		vgic_its_invalidate_cache(vcpu->kvm);
> -	}
> +		smp_mb__before_atomic();
> +		atomic_set(&vgic_cpu->ctlr, 0);
> +	} else {
> +		ctlr = atomic_cmpxchg_acquire(&vgic_cpu->ctlr, 0,
> +					      GICR_CTLR_ENABLE_LPIS);
> +		if (ctlr != 0)
> +			return;
>  
> -	if (!was_enabled && vgic_cpu->lpis_enabled)
>  		vgic_enable_lpis(vcpu);
> +	}
>  }
>  
>  static bool vgic_mmio_vcpu_rdist_is_last(struct kvm_vcpu *vcpu)
> @@ -478,11 +498,10 @@ static void vgic_mmio_write_propbase(struct kvm_vcpu *vcpu,
>  				     unsigned long val)
>  {
>  	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> -	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>  	u64 old_propbaser, propbaser;
>  
>  	/* Storing a value with LPIs already enabled is undefined */
> -	if (vgic_cpu->lpis_enabled)
> +	if (vgic_lpis_enabled(vcpu))
>  		return;
>  
>  	do {
> @@ -513,7 +532,7 @@ static void vgic_mmio_write_pendbase(struct kvm_vcpu *vcpu,
>  	u64 old_pendbaser, pendbaser;
>  
>  	/* Storing a value with LPIs already enabled is undefined */
> -	if (vgic_cpu->lpis_enabled)
> +	if (vgic_lpis_enabled(vcpu))
>  		return;
>  
>  	do {
> @@ -546,10 +565,9 @@ static void vgic_mmio_write_invlpi(struct kvm_vcpu *vcpu,
>  				   gpa_t addr, unsigned int len,
>  				   unsigned long val)
>  {
> -	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>  	struct vgic_irq *irq;
>  
> -	if (!vgic_cpu->lpis_enabled)
> +	if (!vgic_lpis_enabled(vcpu))
>  		return;
>  
>  	vgic_make_rdist_busy(vcpu, true);
> @@ -568,9 +586,7 @@ static void vgic_mmio_write_invall(struct kvm_vcpu *vcpu,
>  				   gpa_t addr, unsigned int len,
>  				   unsigned long val)
>  {
> -	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
> -
> -	if (!vgic_cpu->lpis_enabled)
> +	if (!vgic_lpis_enabled(vcpu))
>  		return;
>  

nit: could you reorder the series to avoid rewriting parts of patch 2
again?


Otherwise:

Reviewed-by: Oliver Upton <oupton@google.com>

>  	vgic_make_rdist_busy(vcpu, true);
> diff --git a/arch/arm64/kvm/vgic/vgic.h b/arch/arm64/kvm/vgic/vgic.h
> index 53581e11f7c8..1d04a900f3e3 100644
> --- a/arch/arm64/kvm/vgic/vgic.h
> +++ b/arch/arm64/kvm/vgic/vgic.h
> @@ -308,6 +308,7 @@ static inline bool vgic_dist_overlap(struct kvm *kvm, gpa_t base, size_t size)
>  		(base < d->vgic_dist_base + KVM_VGIC_V3_DIST_SIZE);
>  }
>  
> +bool vgic_lpis_enabled(struct kvm_vcpu *vcpu);
>  int vgic_copy_lpi_list(struct kvm *kvm, struct kvm_vcpu *vcpu, u32 **intid_ptr);
>  int vgic_its_resolve_lpi(struct kvm *kvm, struct vgic_its *its,
>  			 u32 devid, u32 eventid, struct vgic_irq **irq);
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index d54bb44d6d98..401236f97cf2 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -348,8 +348,8 @@ struct vgic_cpu {
>  
>  	/* Contains the attributes and gpa of the LPI pending tables. */
>  	u64 pendbaser;
> -
> -	bool lpis_enabled;
> +	/* GICR_CTLR.{ENABLE_LPIS,RWP} */
> +	atomic_t ctlr;
>  
>  	/* Cache guest priority bits */
>  	u32 num_pri_bits;
> -- 
> 2.34.1
> 
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm