Re: [PATCH 07/14] KVM: Don't block+unblock when halt-polling is successful
From: Marc Zyngier <maz@kernel.org>
Date: 2021-09-28 09:24:41
Also in:
kvm, kvmarm, linux-mips, lkml
On Mon, 27 Sep 2021 18:28:14 +0100, Sean Christopherson [off-list ref] wrote:
On Sun, Sep 26, 2021, Marc Zyngier wrote:quoted
On Sun, 26 Sep 2021 07:27:28 +0100, Paolo Bonzini [off-list ref] wrote:quoted
On 25/09/21 11:50, Marc Zyngier wrote:quoted
quoted
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.quoted
quoted
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.
quoted hunk ↗ jump to hunk
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).
quoted hunk ↗ jump to hunk
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. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel