Re: [PATCH v2] KVM: Fix simultaneous NMIs
On Wed, Sep 21, 2011 at 01:44:49PM -0300, Marcelo Tosatti wrote: On Wed, Sep 21, 2011 at 11:46:03AM +0300, Avi Kivity wrote: On 09/20/2011 08:28 PM, Avi Kivity wrote: On 09/20/2011 07:30 PM, Marcelo Tosatti wrote: We do have a small issue. If we exit during NMI-blocked-by-STI and nmi_pending == 2, then we lose the second interrupt. Should rarely happen, since external interrupts never exit in that condition, but it's a wart. Actually exits in the window between - increase of nmi_queued and - transfer to nmi_pending/nmi_injected Lose all nmi_queued values, no? Applied the patch, please fix save/restore later, thanks. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] KVM: Fix simultaneous NMIs
On 09/20/2011 08:28 PM, Avi Kivity wrote: On 09/20/2011 07:30 PM, Marcelo Tosatti wrote: We do have a small issue. If we exit during NMI-blocked-by-STI and nmi_pending == 2, then we lose the second interrupt. Should rarely happen, since external interrupts never exit in that condition, but it's a wart. And the above system reset case, you should be able to handle it by saving/restoring nmi_queued (so that QEMU can zero it in vcpu_reset). We could just add a KVM_CAP (and flag) that extends nmi_pending from a bool to a counter. Or just add a new field to the pad. Okay; I'll address this in a follow-on patch (my preference is making nmi_pending a counter). Yet another way to do this is to redefine .injected (just in the API) to mean: inject immediately, unless blocked by interrupt shadow; in this case inject in the next instruction. No KVM_CAP or anything. The drawback is that if we hit the corner case of two NMIs queued and held back by interrupt shadow, an older kvm live migration target will not run the guest (exit with invalid state). The advantage is that no user space or API changes are necessary. Given that to get into this corner you need an NMI intensive load AND a sti; blah pair that spans two pages AND have the second page unavailable when those NMIs hit, I think it's better to avoid the API change. Opinions? -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] KVM: Fix simultaneous NMIs
On Wed, Sep 21, 2011 at 11:46:03AM +0300, Avi Kivity wrote: On 09/20/2011 08:28 PM, Avi Kivity wrote: On 09/20/2011 07:30 PM, Marcelo Tosatti wrote: We do have a small issue. If we exit during NMI-blocked-by-STI and nmi_pending == 2, then we lose the second interrupt. Should rarely happen, since external interrupts never exit in that condition, but it's a wart. Actually exits in the window between - increase of nmi_queued and - transfer to nmi_pending/nmi_injected Lose all nmi_queued values, no? And the above system reset case, you should be able to handle it by saving/restoring nmi_queued (so that QEMU can zero it in vcpu_reset). We could just add a KVM_CAP (and flag) that extends nmi_pending from a bool to a counter. Or just add a new field to the pad. Okay; I'll address this in a follow-on patch (my preference is making nmi_pending a counter). Yet another way to do this is to redefine .injected (just in the API) to mean: inject immediately, unless blocked by interrupt shadow; in this case inject in the next instruction. No KVM_CAP or anything. The drawback is that if we hit the corner case of two NMIs queued and held back by interrupt shadow, an older kvm live migration target will not run the guest (exit with invalid state). The advantage is that no user space or API changes are necessary. Given that to get into this corner you need an NMI intensive load AND a sti; blah pair that spans two pages AND have the second page unavailable when those NMIs hit, I think it's better to avoid the API change. Opinions? -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] KVM: Fix simultaneous NMIs
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 a...@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?
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] KVM: Fix simultaneous NMIs
On 09/20/2011 04:25 PM, Marcelo Tosatti wrote:
@@ -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?).
Did you miss the call to process_nmi()?
We do have a small issue. If we exit during NMI-blocked-by-STI and
nmi_pending == 2, then we lose the second interrupt. Should rarely
happen, since external interrupts never exit in that condition, but it's
a wart.
@@ -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?
It's actually to clear queued NMIs in case we set nmi_pending (which
should never happen unless the machine is completely quiet, since it's
asynchronous to the vcpu; same as the IRR).
--
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] KVM: Fix simultaneous NMIs
On Tue, Sep 20, 2011 at 04:56:02PM +0300, Avi Kivity wrote:
On 09/20/2011 04:25 PM, Marcelo Tosatti wrote:
@@ -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?).
Did you miss the call to process_nmi()?
It transfers nmi_queued to nmi_pending with capping. What i mean is that
upon system reset, nmi_queued (which refers to pre-reset system state)
should be zeroed.
We do have a small issue. If we exit during NMI-blocked-by-STI and
nmi_pending == 2, then we lose the second interrupt. Should rarely
happen, since external interrupts never exit in that condition, but
it's a wart.
And the above system reset case, you should be able to handle it by
saving/restoring nmi_queued (so that QEMU can zero it in vcpu_reset).
@@ -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?
It's actually to clear queued NMIs in case we set nmi_pending (which
should never happen unless the machine is completely quiet, since
it's asynchronous to the vcpu; same as the IRR).
OK.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] KVM: Fix simultaneous NMIs
On 09/20/2011 05:59 PM, Marcelo Tosatti wrote:
On Tue, Sep 20, 2011 at 04:56:02PM +0300, Avi Kivity wrote:
On 09/20/2011 04:25 PM, Marcelo Tosatti wrote:
@@ -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?).
Did you miss the call to process_nmi()?
It transfers nmi_queued to nmi_pending with capping. What i mean is that
upon system reset, nmi_queued (which refers to pre-reset system state)
should be zeroed.
The is get_events(); for set_events(), process_nmi() does zero
nmi_queued() (and then we overwrite nmi_pending).
We do have a small issue. If we exit during NMI-blocked-by-STI and
nmi_pending == 2, then we lose the second interrupt. Should rarely
happen, since external interrupts never exit in that condition, but
it's a wart.
And the above system reset case, you should be able to handle it by
saving/restoring nmi_queued (so that QEMU can zero it in vcpu_reset).
We could just add a KVM_CAP (and flag) that extends nmi_pending from a
bool to a counter.
--
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] KVM: Fix simultaneous NMIs
On Tue, Sep 20, 2011 at 07:24:01PM +0300, Avi Kivity wrote:
On 09/20/2011 05:59 PM, Marcelo Tosatti wrote:
On Tue, Sep 20, 2011 at 04:56:02PM +0300, Avi Kivity wrote:
On 09/20/2011 04:25 PM, Marcelo Tosatti wrote:
@@ -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?).
Did you miss the call to process_nmi()?
It transfers nmi_queued to nmi_pending with capping. What i mean is that
upon system reset, nmi_queued (which refers to pre-reset system state)
should be zeroed.
The is get_events(); for set_events(), process_nmi() does zero
nmi_queued() (and then we overwrite nmi_pending).
Right.
We do have a small issue. If we exit during NMI-blocked-by-STI and
nmi_pending == 2, then we lose the second interrupt. Should rarely
happen, since external interrupts never exit in that condition, but
it's a wart.
And the above system reset case, you should be able to handle it by
saving/restoring nmi_queued (so that QEMU can zero it in vcpu_reset).
We could just add a KVM_CAP (and flag) that extends nmi_pending from
a bool to a counter.
Or just add a new field to the pad.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] KVM: Fix simultaneous NMIs
On 09/20/2011 07:30 PM, Marcelo Tosatti wrote: We do have a small issue. If we exit during NMI-blocked-by-STI and nmi_pending == 2, then we lose the second interrupt. Should rarely happen, since external interrupts never exit in that condition, but it's a wart. And the above system reset case, you should be able to handle it by saving/restoring nmi_queued (so that QEMU can zero it in vcpu_reset). We could just add a KVM_CAP (and flag) that extends nmi_pending from a bool to a counter. Or just add a new field to the pad. Okay; I'll address this in a follow-on patch (my preference is making nmi_pending a counter). -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
