| Message ID | 8d319389dd68794c629b10ce3a071f16119d248b.1364150685.git.jan.kiszka@web.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Mar 24, 2013 at 07:44:47PM +0100, Jan Kiszka wrote: > From: Jan Kiszka <jan.kiszka@siemens.com> > > If we are in guest mode, L0 can only inject events into L2 if L1 has > nothing pending. Otherwise, L0 would overwrite L1's events and they > would get lost. But even if no injection of L1 is pending, we do not > want L0 to interrupt unnecessarily an on going vmentry with all its side > effects on the vmcs. Therefore, injection shall be disallowed during > L1->L2 transitions. This check is conceptually independent of > nested_exit_on_intr. > > If L1 traps external interrupts, then we also need to look at L1's > idt_vectoring_info_field. If it is empty, we can kick the guest from L2 > to L1, just like the previous code worked. > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > --- > arch/x86/kvm/vmx.c | 28 ++++++++++++++++++++-------- > 1 files changed, 20 insertions(+), 8 deletions(-) > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index d1bc834..30aa198 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -4325,16 +4325,28 @@ static int vmx_nmi_allowed(struct kvm_vcpu *vcpu) > > static int vmx_interrupt_allowed(struct kvm_vcpu *vcpu) > { > - if (is_guest_mode(vcpu) && nested_exit_on_intr(vcpu)) { > + if (is_guest_mode(vcpu)) { > struct vmcs12 *vmcs12 = get_vmcs12(vcpu); > - if (to_vmx(vcpu)->nested.nested_run_pending || > - (vmcs12->idt_vectoring_info_field & > - VECTORING_INFO_VALID_MASK)) > + > + if (to_vmx(vcpu)->nested.nested_run_pending) > return 0; > - nested_vmx_vmexit(vcpu); > - vmcs12->vm_exit_reason = EXIT_REASON_EXTERNAL_INTERRUPT; > - vmcs12->vm_exit_intr_info = 0; > - /* fall through to normal code, but now in L1, not L2 */ > + if (nested_exit_on_intr(vcpu)) { > + /* > + * Check if the idt_vectoring_info_field is free. We > + * cannot raise EXIT_REASON_EXTERNAL_INTERRUPT if it > + * isn't. > + */ > + if (vmcs12->idt_vectoring_info_field & > + VECTORING_INFO_VALID_MASK) > + return 0; After patch 2 I do not see how this can be true. Now this case is handled by the common code: since event queue is not empty the code will not get here. > + nested_vmx_vmexit(vcpu); > + vmcs12->vm_exit_reason = > + EXIT_REASON_EXTERNAL_INTERRUPT; > + vmcs12->vm_exit_intr_info = 0; > + /* > + * fall through to normal code, but now in L1, not L2 > + */ > + } > } > > return (vmcs_readl(GUEST_RFLAGS) & X86_EFLAGS_IF) && > -- > 1.7.3.4 -- Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2013-04-11 13:20, Gleb Natapov wrote: > On Sun, Mar 24, 2013 at 07:44:47PM +0100, Jan Kiszka wrote: >> From: Jan Kiszka <jan.kiszka@siemens.com> >> >> If we are in guest mode, L0 can only inject events into L2 if L1 has >> nothing pending. Otherwise, L0 would overwrite L1's events and they >> would get lost. But even if no injection of L1 is pending, we do not >> want L0 to interrupt unnecessarily an on going vmentry with all its side >> effects on the vmcs. Therefore, injection shall be disallowed during >> L1->L2 transitions. This check is conceptually independent of >> nested_exit_on_intr. >> >> If L1 traps external interrupts, then we also need to look at L1's >> idt_vectoring_info_field. If it is empty, we can kick the guest from L2 >> to L1, just like the previous code worked. >> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> >> --- >> arch/x86/kvm/vmx.c | 28 ++++++++++++++++++++-------- >> 1 files changed, 20 insertions(+), 8 deletions(-) >> >> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c >> index d1bc834..30aa198 100644 >> --- a/arch/x86/kvm/vmx.c >> +++ b/arch/x86/kvm/vmx.c >> @@ -4325,16 +4325,28 @@ static int vmx_nmi_allowed(struct kvm_vcpu *vcpu) >> >> static int vmx_interrupt_allowed(struct kvm_vcpu *vcpu) >> { >> - if (is_guest_mode(vcpu) && nested_exit_on_intr(vcpu)) { >> + if (is_guest_mode(vcpu)) { >> struct vmcs12 *vmcs12 = get_vmcs12(vcpu); >> - if (to_vmx(vcpu)->nested.nested_run_pending || >> - (vmcs12->idt_vectoring_info_field & >> - VECTORING_INFO_VALID_MASK)) >> + >> + if (to_vmx(vcpu)->nested.nested_run_pending) >> return 0; >> - nested_vmx_vmexit(vcpu); >> - vmcs12->vm_exit_reason = EXIT_REASON_EXTERNAL_INTERRUPT; >> - vmcs12->vm_exit_intr_info = 0; >> - /* fall through to normal code, but now in L1, not L2 */ >> + if (nested_exit_on_intr(vcpu)) { >> + /* >> + * Check if the idt_vectoring_info_field is free. We >> + * cannot raise EXIT_REASON_EXTERNAL_INTERRUPT if it >> + * isn't. >> + */ >> + if (vmcs12->idt_vectoring_info_field & >> + VECTORING_INFO_VALID_MASK) >> + return 0; > After patch 2 I do not see how this can be true. Now this case is > handled by the common code: since event queue is not empty the code will not > get here. The event queue is unconditionally cleared (after being migrated to vmcs12) in patch 2. Jan
On Thu, Apr 11, 2013 at 04:27:23PM +0200, Jan Kiszka wrote: > On 2013-04-11 13:20, Gleb Natapov wrote: > > On Sun, Mar 24, 2013 at 07:44:47PM +0100, Jan Kiszka wrote: > >> From: Jan Kiszka <jan.kiszka@siemens.com> > >> > >> If we are in guest mode, L0 can only inject events into L2 if L1 has > >> nothing pending. Otherwise, L0 would overwrite L1's events and they > >> would get lost. But even if no injection of L1 is pending, we do not > >> want L0 to interrupt unnecessarily an on going vmentry with all its side > >> effects on the vmcs. Therefore, injection shall be disallowed during > >> L1->L2 transitions. This check is conceptually independent of > >> nested_exit_on_intr. > >> > >> If L1 traps external interrupts, then we also need to look at L1's > >> idt_vectoring_info_field. If it is empty, we can kick the guest from L2 > >> to L1, just like the previous code worked. > >> > >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > >> --- > >> arch/x86/kvm/vmx.c | 28 ++++++++++++++++++++-------- > >> 1 files changed, 20 insertions(+), 8 deletions(-) > >> > >> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > >> index d1bc834..30aa198 100644 > >> --- a/arch/x86/kvm/vmx.c > >> +++ b/arch/x86/kvm/vmx.c > >> @@ -4325,16 +4325,28 @@ static int vmx_nmi_allowed(struct kvm_vcpu *vcpu) > >> > >> static int vmx_interrupt_allowed(struct kvm_vcpu *vcpu) > >> { > >> - if (is_guest_mode(vcpu) && nested_exit_on_intr(vcpu)) { > >> + if (is_guest_mode(vcpu)) { > >> struct vmcs12 *vmcs12 = get_vmcs12(vcpu); > >> - if (to_vmx(vcpu)->nested.nested_run_pending || > >> - (vmcs12->idt_vectoring_info_field & > >> - VECTORING_INFO_VALID_MASK)) > >> + > >> + if (to_vmx(vcpu)->nested.nested_run_pending) > >> return 0; > >> - nested_vmx_vmexit(vcpu); > >> - vmcs12->vm_exit_reason = EXIT_REASON_EXTERNAL_INTERRUPT; > >> - vmcs12->vm_exit_intr_info = 0; > >> - /* fall through to normal code, but now in L1, not L2 */ > >> + if (nested_exit_on_intr(vcpu)) { > >> + /* > >> + * Check if the idt_vectoring_info_field is free. We > >> + * cannot raise EXIT_REASON_EXTERNAL_INTERRUPT if it > >> + * isn't. > >> + */ > >> + if (vmcs12->idt_vectoring_info_field & > >> + VECTORING_INFO_VALID_MASK) > >> + return 0; > > After patch 2 I do not see how this can be true. Now this case is > > handled by the common code: since event queue is not empty the code will not > > get here. > > The event queue is unconditionally cleared (after being migrated to > vmcs12) in patch 2. > During vmexit, yes. But here we are in if(is_guest_mode(vcpu)). -- Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2013-04-11 16:29, Gleb Natapov wrote: > On Thu, Apr 11, 2013 at 04:27:23PM +0200, Jan Kiszka wrote: >> On 2013-04-11 13:20, Gleb Natapov wrote: >>> On Sun, Mar 24, 2013 at 07:44:47PM +0100, Jan Kiszka wrote: >>>> From: Jan Kiszka <jan.kiszka@siemens.com> >>>> >>>> If we are in guest mode, L0 can only inject events into L2 if L1 has >>>> nothing pending. Otherwise, L0 would overwrite L1's events and they >>>> would get lost. But even if no injection of L1 is pending, we do not >>>> want L0 to interrupt unnecessarily an on going vmentry with all its side >>>> effects on the vmcs. Therefore, injection shall be disallowed during >>>> L1->L2 transitions. This check is conceptually independent of >>>> nested_exit_on_intr. >>>> >>>> If L1 traps external interrupts, then we also need to look at L1's >>>> idt_vectoring_info_field. If it is empty, we can kick the guest from L2 >>>> to L1, just like the previous code worked. >>>> >>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> >>>> --- >>>> arch/x86/kvm/vmx.c | 28 ++++++++++++++++++++-------- >>>> 1 files changed, 20 insertions(+), 8 deletions(-) >>>> >>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c >>>> index d1bc834..30aa198 100644 >>>> --- a/arch/x86/kvm/vmx.c >>>> +++ b/arch/x86/kvm/vmx.c >>>> @@ -4325,16 +4325,28 @@ static int vmx_nmi_allowed(struct kvm_vcpu *vcpu) >>>> >>>> static int vmx_interrupt_allowed(struct kvm_vcpu *vcpu) >>>> { >>>> - if (is_guest_mode(vcpu) && nested_exit_on_intr(vcpu)) { >>>> + if (is_guest_mode(vcpu)) { >>>> struct vmcs12 *vmcs12 = get_vmcs12(vcpu); >>>> - if (to_vmx(vcpu)->nested.nested_run_pending || >>>> - (vmcs12->idt_vectoring_info_field & >>>> - VECTORING_INFO_VALID_MASK)) >>>> + >>>> + if (to_vmx(vcpu)->nested.nested_run_pending) >>>> return 0; >>>> - nested_vmx_vmexit(vcpu); >>>> - vmcs12->vm_exit_reason = EXIT_REASON_EXTERNAL_INTERRUPT; >>>> - vmcs12->vm_exit_intr_info = 0; >>>> - /* fall through to normal code, but now in L1, not L2 */ >>>> + if (nested_exit_on_intr(vcpu)) { >>>> + /* >>>> + * Check if the idt_vectoring_info_field is free. We >>>> + * cannot raise EXIT_REASON_EXTERNAL_INTERRUPT if it >>>> + * isn't. >>>> + */ >>>> + if (vmcs12->idt_vectoring_info_field & >>>> + VECTORING_INFO_VALID_MASK) >>>> + return 0; >>> After patch 2 I do not see how this can be true. Now this case is >>> handled by the common code: since event queue is not empty the code will not >>> get here. >> >> The event queue is unconditionally cleared (after being migrated to >> vmcs12) in patch 2. >> > During vmexit, yes. But here we are in if(is_guest_mode(vcpu)). Hmm, looks like: We leave L2, transfer the real vectoring info into the queue and then consider injecting something in addition. That should actually be avoided at higher level. Ok, will drop this test. Jan
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index d1bc834..30aa198 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -4325,16 +4325,28 @@ static int vmx_nmi_allowed(struct kvm_vcpu *vcpu) static int vmx_interrupt_allowed(struct kvm_vcpu *vcpu) { - if (is_guest_mode(vcpu) && nested_exit_on_intr(vcpu)) { + if (is_guest_mode(vcpu)) { struct vmcs12 *vmcs12 = get_vmcs12(vcpu); - if (to_vmx(vcpu)->nested.nested_run_pending || - (vmcs12->idt_vectoring_info_field & - VECTORING_INFO_VALID_MASK)) + + if (to_vmx(vcpu)->nested.nested_run_pending) return 0; - nested_vmx_vmexit(vcpu); - vmcs12->vm_exit_reason = EXIT_REASON_EXTERNAL_INTERRUPT; - vmcs12->vm_exit_intr_info = 0; - /* fall through to normal code, but now in L1, not L2 */ + if (nested_exit_on_intr(vcpu)) { + /* + * Check if the idt_vectoring_info_field is free. We + * cannot raise EXIT_REASON_EXTERNAL_INTERRUPT if it + * isn't. + */ + if (vmcs12->idt_vectoring_info_field & + VECTORING_INFO_VALID_MASK) + return 0; + nested_vmx_vmexit(vcpu); + vmcs12->vm_exit_reason = + EXIT_REASON_EXTERNAL_INTERRUPT; + vmcs12->vm_exit_intr_info = 0; + /* + * fall through to normal code, but now in L1, not L2 + */ + } } return (vmcs_readl(GUEST_RFLAGS) & X86_EFLAGS_IF) &&