Re: [PATCH 07/14] KVM: Don't block+unblock when halt-polling is successful

[Marc Zyngier <maz@kernel.org>]

On Mon, 27 Sep 2021 18:28:14 +0100,
Sean Christopherson <seanjc@google.com> wrote:
> 
> On Sun, Sep 26, 2021, Marc Zyngier wrote:
> > On Sun, 26 Sep 2021 07:27:28 +0100,
> > Paolo Bonzini <pbonzini@redhat.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.

Not quite. We only need to retrieve the VMCR if we are in a situation
where we need to trigger a wake-up from WFI at the point where we have
not done a vcpu_put() yet. All the other cases where the interrupt is
injected are managed by the HW. And the only case where
kvm_arch_vcpu_runnable() gets called is when blocking.

I also don't get why a hook would be fragile, as long as it has well
defined semantics.

> 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;
> +       }
> +

But most of the uses of vgic_get_vmcr() are in contexts where the vcpu
isn't running at all (such as save/restore). It really only operates
on the shadow state, and what you have above will only lead to state
corruption.

>         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?

It is a requirement. Otherwise, we end-up with an inconsistent state
between the delivery of doorbells and the state of the vgic. Also,
reloading the GICv4 state can be pretty expensive (multiple MMIO
accesses), which is why we really don't want to do that on the hot
path (kvm_arch_vcpu_ioctl_run() *is* a hot path).

> 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);

You'd need to detect that a previous put has been done. But overall,
it puts the complexity at the wrong place. WFI (aka kvm_vcpu_block) is
the place where we want to handle this synchronisation, and not the
run loop.

Instead of having a well defined interface with the blocking code
where we implement the required synchronisation, you spray the vgic
crap all over, and it becomes much harder to reason about it. Guess
what, I'm not keen on it.

	M.

-- 
Without deviation from the norm, progress is not possible.