Re: [PATCH v2] KVM: Fix simultaneous NMIs

Marcelo Tosatti
2011-09-20T13:26:03Z

On Tue, Sep 20, 2011 at 01:43:14PM +0300, Avi Kivity wrote:
> If simultaneous NMIs happen, we're supposed to queue the second
> and next (collapsing them), but currently we sometimes collapse
> the second into the first.
>
> Fix by using a counter for pending NMIs instead of a bool; since
> the counter limit depends on whether the processor is currently
> in an NMI handler, which can only be checked in vcpu context
> (via the NMI mask), we add a new KVM_REQ_NMI to request recalculation
> of the counter.
>
> Signed-off-by: Avi Kivity <avi@redhat.com>
> ---
> arch/x86/include/asm/kvm_host.h | 5 ++-
> arch/x86/kvm/x86.c | 48 +++++++++++++++++++++++++-------------
> include/linux/kvm_host.h | 1 +
> 3 files changed, 35 insertions(+), 19 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 6ab4241..ab62711 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -413,8 +413,9 @@ struct kvm_vcpu_arch {
> u32 tsc_catchup_mult;
> s8 tsc_catchup_shift;
>
> - bool nmi_pending;
> - bool nmi_injected;
> + atomic_t nmi_queued; /* unprocessed asynchronous NMIs */
> + unsigned nmi_pending; /* NMI queued after currently running handler */
> + bool nmi_injected; /* Trying to inject an NMI this entry */
>
> struct mtrr_state_type mtrr_state;
> u32 pat;
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 6b37f18..d51e407 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -83,6 +83,7 @@
> static void update_cr8_intercept(struct kvm_vcpu *vcpu);
> static int kvm_dev_ioctl_get_supported_cpuid(struct kvm_cpuid2 *cpuid,
> struct kvm_cpuid_entry2 __user *entries);
> +static void process_nmi(struct kvm_vcpu *vcpu);
>
> struct kvm_x86_ops *kvm_x86_ops;
> EXPORT_SYMBOL_GPL(kvm_x86_ops);
> @@ -359,8 +360,8 @@ void kvm_propagate_fault(struct kvm_vcpu *vcpu, struct x86_exception *fault)
>
> void kvm_inject_nmi(struct kvm_vcpu *vcpu)
> {
> - kvm_make_request(KVM_REQ_EVENT, vcpu);
> - vcpu->arch.nmi_pending = 1;
> + atomic_inc(&vcpu->arch.nmi_queued);
> + kvm_make_request(KVM_REQ_NMI, vcpu);
> }
> EXPORT_SYMBOL_GPL(kvm_inject_nmi);
>
> @@ -2827,6 +2828,7 @@ static int kvm_vcpu_ioctl_x86_set_mce(struct kvm_vcpu *vcpu,
> static void kvm_vcpu_ioctl_x86_get_vcpu_events(struct kvm_vcpu *vcpu,
> struct kvm_vcpu_events *events)
> {
> + process_nmi(vcpu);
> events->exception.injected =
> vcpu->arch.exception.pending &&
> !kvm_exception_is_soft(vcpu->arch.exception.nr);
> @@ -2844,7 +2846,7 @@ static void kvm_vcpu_ioctl_x86_get_vcpu_events(struct kvm_vcpu *vcpu,
> KVM_X86_SHADOW_INT_MOV_SS | KVM_X86_SHADOW_INT_STI);
>
> events->nmi.injected = vcpu->arch.nmi_injected;
> - events->nmi.pending = vcpu->arch.nmi_pending;
> + events->nmi.pending = vcpu->arch.nmi_pending != 0;
> events->nmi.masked = kvm_x86_ops->get_nmi_mask(vcpu);
> events->nmi.pad = 0;

nmi_queued should also be saved and restored. Not sure if its necessary
though.

Should at least reset nmi_queued somewhere (set_vcpu_events?).

> @@ -2864,6 +2866,7 @@ static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu,
> | KVM_VCPUEVENT_VALID_SHADOW))
> return -EINVAL;
>
> + process_nmi(vcpu);

This should be after nmi fields are set, not before?