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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
> 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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
