| Message ID | 20210925005528.1145584-8-seanjc@google.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | KVM: Halt-polling fixes, cleanups and a new stat | expand |
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.
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
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.
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) {
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.
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);
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 --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); /*
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(-)