diff mbox series

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

Message ID 20210925005528.1145584-8-seanjc@google.com
State New
Headers show
Series KVM: Halt-polling fixes, cleanups and a new stat | expand

Commit Message

Sean Christopherson Sept. 25, 2021, 12:55 a.m. UTC
Invoke the arch hooks for block+unblock if and only if KVM actually
attempts to block the vCPU.  The only non-nop implementation is on arm64,
and if halt-polling is successful, there is no need for arm64 to put/load
the vGIC as KVM hasn't relinquished control of the vCPU in any way.

The primary motivation is to allow future cleanup to split out "block"
from "halt", but this is also likely a small performance boost on arm64
when halt-polling is successful.

Adjust the post-block path to update "cur" after unblocking, i.e. include
vGIC load time in halt_wait_ns and halt_wait_hist, so that the behavior
is consistent.  Moving just the pre-block arch hook would result in only
the vGIC put latency being included in the halt_wait stats.  There is no
obvious evidence that one way or the other is correct, so just ensure KVM
is consistent.

Cc: Marc Zyngier <maz@kernel.org>
Cc: James Morse <james.morse@arm.com>
Cc: Alexandru Elisei <alexandru.elisei@arm.com>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 virt/kvm/kvm_main.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Comments

Marc Zyngier Sept. 25, 2021, 9:50 a.m. UTC | #1
On Sat, 25 Sep 2021 01:55:21 +0100,
Sean Christopherson <seanjc@google.com> wrote:
> 
> Invoke the arch hooks for block+unblock if and only if KVM actually
> attempts to block the vCPU.  The only non-nop implementation is on arm64,
> and if halt-polling is successful, 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.

> 
> The primary motivation is to allow future cleanup to split out "block"
> from "halt", but this is also likely a small performance boost on arm64
> when halt-polling is successful.
> 
> Adjust the post-block path to update "cur" after unblocking, i.e. include
> vGIC load time in halt_wait_ns and halt_wait_hist, so that the behavior
> is consistent.  Moving just the pre-block arch hook would result in only
> the vGIC put latency being included in the halt_wait stats.  There is no
> obvious evidence that one way or the other is correct, so just ensure KVM
> is consistent.

This effectively reverts 07ab0f8d9a12 ("KVM: Call
kvm_arch_vcpu_blocking early into the blocking sequence"), which was a
huge gain on arm64, not to mention a correctness fix.

Without this, a GICv4 machine will always pay for the full poll
penalty, going into schedule(), and only then get a doorbell interrupt
signalling telling the kernel that there was an interrupt.

On a non-GICv4 machine, it means that interrupts injected by another
thread during the pooling will be evaluated with an outdated priority
mask, which can result in either a spurious wake-up or a missed
wake-up.

If it means introducing a new set of {pre,post}-poll arch-specific
hooks, so be it. But I don't think this change is acceptable as is.

Thanks,

	M.
Paolo Bonzini Sept. 26, 2021, 6:27 a.m. UTC | #2
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.

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.

However, is activating the doorbell necessary?  If possible, polling the 
VGIC directly for pending VLPIs without touching the ITS (for example by 
emulating IAR reads) may make sense.  IIUC that must be done at EL2 
though, so maybe it would even make sense to move all of halt polling to 
EL2 for the nVHE case.  It all depends on benchmark results, of course.

Sorry for the many stupid questions I'm asking lately, but I'm trying to 
pay more attention to ARM and understand the VGIC and EL1/EL2 split better.

Paolo
Marc Zyngier Sept. 26, 2021, 9:02 a.m. UTC | #3
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.
> 
> 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.

> However, is activating the doorbell necessary?  If possible, polling
> the VGIC directly for pending VLPIs without touching the ITS (for
> example by emulating IAR reads) may make sense.  IIUC that must be
> done at EL2 though, so maybe it would even make sense to move all of
> halt polling to EL2 for the nVHE case.  It all depends on benchmark
> results, of course.

No, there is no architectural way to observe the VLPI state. EL2
cannot impersonate the guest an read ICV_IAR1_EL1 (because it
conveniently has the same encoding as ICC_IAR1_EL1), and if it could,
it would be *destructive* (not what you want). The equivalent of the
LR that is used to hold the highest priority VLPI presented to the
virtual CPU interface is not visible to SW at all.

There are exactly two ways for the hypervisor to get a hint about the
VLPI state (and that's only a hint, as everything can be spurious):

- Make the vPE non resident and use GICR_VPENDBASER.PendingLast bit to
  find out whether there are pending VLPIs

- Make the vPE non resident and get a doorbell interrupt

See the common pattern?

There is no polling mechanism, and the only way to flush the VLPI
state to memory is to destroy the GIC view of the vPE, which is a bit
counter-productive. It also only work on GICv4.1, and not GICv4 (which
is why we don't support live migration on GICv4).

> Sorry for the many stupid questions I'm asking lately, but I'm trying
> to pay more attention to ARM and understand the VGIC and EL1/EL2 split
> better.

Feel free to ask any question. The more people understand how the
architecture works, the better.

	M.
Sean Christopherson Sept. 27, 2021, 5:28 p.m. UTC | #4
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.  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) {
Marc Zyngier Sept. 28, 2021, 9:24 a.m. UTC | #5
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.
Sean Christopherson Sept. 28, 2021, 4:21 p.m. UTC | #6
On Tue, Sep 28, 2021, Marc Zyngier wrote:
> 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.

Generic KVM should not have to know that a seemingly benign arch hook,
kvm_arch_vcpu_runnable(), cannot be safely called without first calling another
arch hook.  E.g. I suspect there's a (benign?) race in kvm_vcpu_on_spin().  If
the loop is delayed between checking rcuwait_active() and vcpu_dy_runnable(),
and the target vCPU is awakened during that period, KVM can call
kvm_arch_vcpu_runnable() while the vCPU is running.

It's kind of a counter-example to my below suggestion as putting the vGIC would
indeed lead to state corruption if the vCPU is running, but I would argue that
arm64 should override kvm_arch_dy_runnable() so that its correctness is guaranteed,
e.g. by not calling kvm_arch_vcpu_runnable() if the vCPU is already running.

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

Ignoring the kvm_arch_dy_runnable() case for the moment, how would it lead to
corruption?  The idea is that the 'vmcr_available' flag would be cleared when the
vCPU is run, i.e. it tracks whether or not the shadow state may be stale.

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

For my own understanding, does KVM require it to be resident in hardware while
the vCPU is loaded but _not_ running?  What I don't fully understand is how KVM
can safely load/put the vCPU if that true, i.e. wouldn't there always be a window
for badness?

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

I wasn't suggesting to reload GICv4 on every entry, it would only be reloaded
if it was put between vcpu_load() and entry to the guest.

> > If not, then we could do something like
> > this, which would eliminate the arch hooks entirely if the VMCR is
> > handled as above.

...

> > @@ -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.

Not that it will likely matter, but doesn't the its_vpe.resident check automatically
handle this?

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

My objection to the arch hooks is that, from generic KVM's perspective, the
direct dependency is not on blocking, it's on calling kvm_arch_vcpu_runnable().
That's why I suggested handling this by tracking whether or not the VMCR is
up-to-date/stale, as it allows generic KVM to safely call kvm_arch_vcpu_runnable()
whenever the vCPU is loaded.

I don't have a strong opinion on arm64 preferring the sync to be specific to
WFI, but if that's the case then IMO this should be handled fully in arm64, e.g.
a patch like so (or with a wrapper around the call to kvm_vcpu_block() if we
want to guard against future calls into generic KVM)

diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index fe102cd2e518..312f3acd3ca3 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -367,27 +367,12 @@ int kvm_cpu_has_pending_timer(struct kvm_vcpu *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)
diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
index 275a27368a04..9870e824a27e 100644
--- a/arch/arm64/kvm/handle_exit.c
+++ b/arch/arm64/kvm/handle_exit.c
@@ -95,8 +95,28 @@ static int kvm_handle_wfx(struct kvm_vcpu *vcpu)
        } else {
                trace_kvm_wfx_arm64(*vcpu_pc(vcpu), false);
                vcpu->stat.wfi_exit_stat++;
+
+               /*
+                * 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, e.g. when determining if the
+                * vCPU should block.
+                *
+                * 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();
+
                kvm_vcpu_block(vcpu);
                kvm_clear_request(KVM_REQ_UNHALT, vcpu);
+
+               preempt_disable();
+               vgic_v4_load(vcpu);
+               preempt_enable();
        }

        kvm_incr_pc(vcpu);
Marc Zyngier Sept. 30, 2021, 9:36 a.m. UTC | #7
On Tue, 28 Sep 2021 17:21:12 +0100,
Sean Christopherson <seanjc@google.com> wrote:
> 
> On Tue, Sep 28, 2021, Marc Zyngier wrote:
> > 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.
> 
> Generic KVM should not have to know that a seemingly benign arch hook,
> kvm_arch_vcpu_runnable(), cannot be safely called without first calling another
> arch hook.  E.g. I suspect there's a (benign?) race in kvm_vcpu_on_spin().  If
> the loop is delayed between checking rcuwait_active() and vcpu_dy_runnable(),
> and the target vCPU is awakened during that period, KVM can call
> kvm_arch_vcpu_runnable() while the vCPU is running.

Humph. Indeed, there is a potential gold-plated turd there.

> It's kind of a counter-example to my below suggestion as putting the vGIC would
> indeed lead to state corruption if the vCPU is running, but I would argue that
> arm64 should override kvm_arch_dy_runnable() so that its correctness is guaranteed,
> e.g. by not calling kvm_arch_vcpu_runnable() if the vCPU is already running.

I'll work something out for that case.

> > > 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.
> 
> Ignoring the kvm_arch_dy_runnable() case for the moment, how would
> it lead to corruption?  The idea is that the 'vmcr_available' flag
> would be cleared when the vCPU is run, i.e. it tracks whether or not
> the shadow state may be stale.

I guess that 'vmcr_available' would have to be initialised to 'true'
at vcpu reset time so that the userspace side cannot trigger a read
from the HW.

> > >         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.
> 
> For my own understanding, does KVM require it to be resident in
> hardware while the vCPU is loaded but _not_ running?  What I don't
> fully understand is how KVM can safely load/put the vCPU if that
> true, i.e. wouldn't there always be a window for badness?

No, that part is fine. It is when you start running the vcpu without
the GICv4 context loaded that ugly stuff happens (get a doorbell that
tells you to schedule the currently running vcpu, for example).

> 
> > 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).
> 
> I wasn't suggesting to reload GICv4 on every entry, it would only be reloaded
> if it was put between vcpu_load() and entry to the guest.
> 
> > > If not, then we could do something like
> > > this, which would eliminate the arch hooks entirely if the VMCR is
> > > handled as above.
> 
> ...
> 
> > > @@ -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.
> 
> Not that it will likely matter, but doesn't the its_vpe.resident
> check automatically handle this?

Sort of. I eventually want to get rid of this as it papers over all
sort of sins. I introduced it exactly because of the nesting that
vcpu_block triggers, but this is a bit of a layering violation between
KVM and the underlying GICv4 driver.

> 
> > 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.
> 
> My objection to the arch hooks is that, from generic KVM's
> perspective, the direct dependency is not on blocking, it's on
> calling kvm_arch_vcpu_runnable().  That's why I suggested handling
> this by tracking whether or not the VMCR is up-to-date/stale, as it
> allows generic KVM to safely call kvm_arch_vcpu_runnable() whenever
> the vCPU is loaded.
> 
> I don't have a strong opinion on arm64 preferring the sync to be
> specific to WFI, but if that's the case then IMO this should be
> handled fully in arm64, e.g.  a patch like so (or with a wrapper
> around the call to kvm_vcpu_block() if we want to guard against
> future calls into generic KVM)
> 
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index fe102cd2e518..312f3acd3ca3 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -367,27 +367,12 @@ int kvm_cpu_has_pending_timer(struct kvm_vcpu *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)
> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index 275a27368a04..9870e824a27e 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -95,8 +95,28 @@ static int kvm_handle_wfx(struct kvm_vcpu *vcpu)
>         } else {
>                 trace_kvm_wfx_arm64(*vcpu_pc(vcpu), false);
>                 vcpu->stat.wfi_exit_stat++;
> +
> +               /*
> +                * 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, e.g. when determining if the
> +                * vCPU should block.
> +                *
> +                * 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();
> +
>                 kvm_vcpu_block(vcpu);
>                 kvm_clear_request(KVM_REQ_UNHALT, vcpu);
> +
> +               preempt_disable();
> +               vgic_v4_load(vcpu);
> +               preempt_enable();
>         }
> 
>         kvm_incr_pc(vcpu);

I actually largely prefer this approach, which is massively more
readable than the current setup. Feel free to wrap that in your
series.

I'll also have a look at the vcpu_dy_runnable() asap.

Thanks,

	M.
diff mbox series

Patch

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 2015a1f532ce..f96cda8312f3 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -3232,8 +3232,6 @@  void kvm_vcpu_block(struct kvm_vcpu *vcpu)
 	bool waited = false;
 	u64 block_ns;
 
-	kvm_arch_vcpu_blocking(vcpu);
-
 	start = cur = poll_end = ktime_get();
 	if (do_halt_poll) {
 		ktime_t stop = ktime_add_ns(ktime_get(), vcpu->halt_poll_ns);
@@ -3250,6 +3248,7 @@  void kvm_vcpu_block(struct kvm_vcpu *vcpu)
 		} while (kvm_vcpu_can_poll(cur, stop));
 	}
 
+	kvm_arch_vcpu_blocking(vcpu);
 
 	prepare_to_rcuwait(&vcpu->wait);
 	for (;;) {
@@ -3262,6 +3261,9 @@  void kvm_vcpu_block(struct kvm_vcpu *vcpu)
 		schedule();
 	}
 	finish_rcuwait(&vcpu->wait);
+
+	kvm_arch_vcpu_unblocking(vcpu);
+
 	cur = ktime_get();
 	if (waited) {
 		vcpu->stat.generic.halt_wait_ns +=
@@ -3269,8 +3271,8 @@  void kvm_vcpu_block(struct kvm_vcpu *vcpu)
 		KVM_STATS_LOG_HIST_UPDATE(vcpu->stat.generic.halt_wait_hist,
 				ktime_to_ns(cur) - ktime_to_ns(poll_end));
 	}
+
 out:
-	kvm_arch_vcpu_unblocking(vcpu);
 	block_ns = ktime_to_ns(cur) - ktime_to_ns(start);
 
 	/*