Message-ID: <YVH/LjCqk/9PfDHn@google.com>
Date:   Mon, 27 Sep 2021 17:28:14 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Marc Zyngier <maz@...nel.org>
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        Huacai Chen <chenhuacai@...nel.org>,
        Aleksandar Markovic <aleksandar.qemu.devel@...il.com>,
        Paul Mackerras <paulus@...abs.org>,
        Christian Borntraeger <borntraeger@...ibm.com>,
        Janosch Frank <frankja@...ux.ibm.com>,
        James Morse <james.morse@....com>,
        Alexandru Elisei <alexandru.elisei@....com>,
        Suzuki K Poulose <suzuki.poulose@....com>,
        David Hildenbrand <david@...hat.com>,
        Cornelia Huck <cohuck@...hat.com>,
        Claudio Imbrenda <imbrenda@...ux.ibm.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>,
        linux-arm-kernel@...ts.infradead.org, kvmarm@...ts.cs.columbia.edu,
        linux-mips@...r.kernel.org, kvm@...r.kernel.org,
        kvm-ppc@...r.kernel.org, linux-kernel@...r.kernel.org,
        David Matlack <dmatlack@...gle.com>,
        Jing Zhang <jingzhangos@...gle.com>
Subject: Re: [PATCH 07/14] KVM: Don't block+unblock when halt-polling is
 successful

On Sun, Sep 26, 2021, Marc Zyngier wrote:
> On Sun, 26 Sep 2021 07:27:28 +0100,
> Paolo Bonzini <pbonzini@...hat.com> wrote:
> > 
> > On 25/09/21 11:50, Marc Zyngier wrote:
> > >> there is no need for arm64 to put/load
> > >> the vGIC as KVM hasn't relinquished control of the vCPU in any way.
> > > 
> > > This doesn't mean that there is no requirement for any state
> > > change. The put/load on GICv4 is crucial for performance, and the VMCR
> > > resync is a correctness requirement.

Ah crud, I didn't blame that code beforehand, I simply assumed
kvm_arch_vcpu_blocking() was purely for the blocking/schedule() sequence.  The
comment in arm64's kvm_arch_vcpu_blocking() about kvm_arch_vcpu_runnable() makes
more sense now too.

> > I wouldn't even say it's crucial for performance: halt polling cannot
> > work and is a waste of time without (the current implementation of)
> > put/load.
> 
> Not quite. A non-V{LPI,SGI} could still be used as the a wake-up from
> WFI (which is the only reason we end-up on this path). Only LPIs (and
> SGIs on GICv4.1) can be directly injected, meaning that SPIs and PPIs
> still follow the standard SW injection model.
> 
> However, there is still the ICH_VMCR_EL2 requirement (to get the
> up-to-date priority mask and group enable bits) for SW-injected
> interrupt wake-up to work correctly, and I really don't want to save
> that one eagerly on each shallow exit.

IIUC, VMCR is resident in hardware while the guest is running, and KVM needs to
retrieve the VMCR when processing interrupts to determine if a interrupt is above
the priority threshold.  If that's the case, then IMO handling the VMCR via an
arch hook is unnecessarily fragile, e.g. any generic call that leads to
kvm_arch_vcpu_runnable() needs to know that arm64 lazily retrieves a guest
register.  A better approach for VMCR would be to retrieve the value from
hardware on-demand, e.g. via a hook in vgic_get_vmcr(), so that it's all but
impossible to have bugs where KVM is working with a stale VMCR, e.g.

diff --git a/arch/arm64/kvm/vgic/vgic-mmio.c b/arch/arm64/kvm/vgic/vgic-mmio.c
index 48c6067fc5ec..0784de0c4080 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio.c
+++ b/arch/arm64/kvm/vgic/vgic-mmio.c
@@ -828,6 +828,13 @@ void vgic_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr)
 
 void vgic_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr)
 {
+       if (!vcpu->...->vmcr_available) {
+               preempt_disable();
+               kvm_vgic_vmcr_sync(vcpu);
+               preempt_enable();
+               vcpu->...->vmcr_available = true;
+       }
+
        if (kvm_vgic_global_state.type == VGIC_V2)
                vgic_v2_get_vmcr(vcpu, vmcr);
        else


Regarding vGIC v4, does KVM require it to be resident in hardware while the vCPU
is loaded?  If not, then we could do something like this, which would eliminate
the arch hooks entirely if the VMCR is handled as above.

diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index fe102cd2e518..efc862c4d802 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -365,31 +365,6 @@ int kvm_cpu_has_pending_timer(struct kvm_vcpu *vcpu)
        return kvm_timer_is_pending(vcpu);
 }

-void kvm_arch_vcpu_blocking(struct kvm_vcpu *vcpu)
-{
-       /*
-        * If we're about to block (most likely because we've just hit a
-        * WFI), we need to sync back the state of the GIC CPU interface
-        * so that we have the latest PMR and group enables. This ensures
-        * that kvm_arch_vcpu_runnable has up-to-date data to decide
-        * whether we have pending interrupts.
-        *
-        * For the same reason, we want to tell GICv4 that we need
-        * doorbells to be signalled, should an interrupt become pending.
-        */
-       preempt_disable();
-       kvm_vgic_vmcr_sync(vcpu);
-       vgic_v4_put(vcpu, true);
-       preempt_enable();
-}
-
-void kvm_arch_vcpu_unblocking(struct kvm_vcpu *vcpu)
-{
-       preempt_disable();
-       vgic_v4_load(vcpu);
-       preempt_enable();
-}
-
 void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 {
        struct kvm_s2_mmu *mmu;
@@ -697,7 +672,6 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu)
                        /* The distributor enable bits were changed */
                        preempt_disable();
                        vgic_v4_put(vcpu, false);
-                       vgic_v4_load(vcpu);
                        preempt_enable();
                }

@@ -813,6 +787,13 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
                 */
                preempt_disable();

+               /*
+                * Reload vGIC v4 if necessary, as it may be put on-demand so
+                * that KVM can detect directly injected interrupts, e.g. when
+                * determining if the vCPU is runnable due to a pending event.
+                */
+               vgic_v4_load(vcpu);
+
                kvm_pmu_flush_hwstate(vcpu);

                local_irq_disable();
diff --git a/arch/arm64/kvm/vgic/vgic.c b/arch/arm64/kvm/vgic/vgic.c
index 5dad4996cfb2..3ef360a20a22 100644
--- a/arch/arm64/kvm/vgic/vgic.c
+++ b/arch/arm64/kvm/vgic/vgic.c
@@ -969,6 +969,16 @@ int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu)

        vgic_get_vmcr(vcpu, &vmcr);

+       /*
+        * Tell GICv4 that we need doorbells to be signalled, should an
+        * interrupt become pending.
+        */
+       if (vcpu->arch.vgic_cpu.vgic_v3.its_vpe.vpe_resident) {
+               preempt_disable();
+               vgic_v4_put(vcpu, true);
+               preempt_enable();
+       }
+
        raw_spin_lock_irqsave(&vgic_cpu->ap_list_lock, flags);

        list_for_each_entry(irq, &vgic_cpu->ap_list_head, ap_list) {

Powered by blists - more mailing lists