Prev: Another BUG_ON() in blkio controller
Next: KVM: SVM: Sync cr0 and cr3 to kvm state before nested handling
From: Alexander Graf on 23 Apr 2010 09:50

On 22.04.2010, at 12:33, Joerg Roedel wrote:

> The patch introducing nested nmi handling had a bug. The
> check does not belong to enable_nmi_window but must be in
> nmi_allowed. This patch fixes this.
>
> Signed-off-by: Joerg Roedel <joerg.roedel(a)amd.com>
> ---
> arch/x86/kvm/svm.c | 16 +++++++++-------
> 1 files changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index ab78eb8..ec20584 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -2771,8 +2771,12 @@ static int svm_nmi_allowed(struct kvm_vcpu *vcpu)
> {
> struct vcpu_svm *svm = to_svm(vcpu);
> struct vmcb *vmcb = svm->vmcb;
> - return !(vmcb->control.int_state & SVM_INTERRUPT_SHADOW_MASK) &&
> - !(svm->vcpu.arch.hflags & HF_NMI_MASK);
> + int ret;
> + ret = !(vmcb->control.int_state & SVM_INTERRUPT_SHADOW_MASK) &&
> + !(svm->vcpu.arch.hflags & HF_NMI_MASK);
> + ret = ret && gif_set(svm) && nested_svm_nmi(svm);
> +
> + return ret;
> }
>
> static bool svm_get_nmi_mask(struct kvm_vcpu *vcpu)
> @@ -2841,11 +2845,9 @@ static void enable_nmi_window(struct kvm_vcpu *vcpu)
> * Something prevents NMI from been injected. Single step over possible
> * problem (IRET or exception injection or interrupt shadow)
> */
> - if (gif_set(svm) && nested_svm_nmi(svm)) {
> - svm->nmi_singlestep = true;
> - svm->vmcb->save.rflags |= (X86_EFLAGS_TF | X86_EFLAGS_RF);
> - update_db_intercept(vcpu);
> - }
> + svm->nmi_singlestep = true;
> + svm->vmcb->save.rflags |= (X86_EFLAGS_TF | X86_EFLAGS_RF);
> + update_db_intercept(vcpu);

So we're always messing with the nested guest state when the host wants to inject an nmi into the l1 guest? Is that safe?


Alex

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Joerg Roedel on 23 Apr 2010 10:20
On Fri, Apr 23, 2010 at 03:46:07PM +0200, Alexander Graf wrote:
>
> On 22.04.2010, at 12:33, Joerg Roedel wrote:
>
> > The patch introducing nested nmi handling had a bug. The
> > check does not belong to enable_nmi_window but must be in
> > nmi_allowed. This patch fixes this.
> >
> > Signed-off-by: Joerg Roedel <joerg.roedel(a)amd.com>
> > ---
> > arch/x86/kvm/svm.c | 16 +++++++++-------
> > 1 files changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> > index ab78eb8..ec20584 100644
> > --- a/arch/x86/kvm/svm.c
> > +++ b/arch/x86/kvm/svm.c
> > @@ -2771,8 +2771,12 @@ static int svm_nmi_allowed(struct kvm_vcpu *vcpu)
> > {
> > struct vcpu_svm *svm = to_svm(vcpu);
> > struct vmcb *vmcb = svm->vmcb;
> > - return !(vmcb->control.int_state & SVM_INTERRUPT_SHADOW_MASK) &&
> > - !(svm->vcpu.arch.hflags & HF_NMI_MASK);
> > + int ret;
> > + ret = !(vmcb->control.int_state & SVM_INTERRUPT_SHADOW_MASK) &&
> > + !(svm->vcpu.arch.hflags & HF_NMI_MASK);
> > + ret = ret && gif_set(svm) && nested_svm_nmi(svm);
> > +
> > + return ret;
> > }
> >
> > static bool svm_get_nmi_mask(struct kvm_vcpu *vcpu)
> > @@ -2841,11 +2845,9 @@ static void enable_nmi_window(struct kvm_vcpu *vcpu)
> > * Something prevents NMI from been injected. Single step over possible
> > * problem (IRET or exception injection or interrupt shadow)
> > */
> > - if (gif_set(svm) && nested_svm_nmi(svm)) {
> > - svm->nmi_singlestep = true;
> > - svm->vmcb->save.rflags |= (X86_EFLAGS_TF | X86_EFLAGS_RF);
> > - update_db_intercept(vcpu);
> > - }
> > + svm->nmi_singlestep = true;
> > + svm->vmcb->save.rflags |= (X86_EFLAGS_TF | X86_EFLAGS_RF);
> > + update_db_intercept(vcpu);
>
> So we're always messing with the nested guest state when the host
> wants to inject an nmi into the l1 guest? Is that safe?

Why not? We can't inject an NMI directly into L2 if the nested
hypervisor intercepts it.

Joerg


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Alexander Graf on 23 Apr 2010 10:30

On 23.04.2010, at 16:13, Joerg Roedel wrote:

> On Fri, Apr 23, 2010 at 03:46:07PM +0200, Alexander Graf wrote:
>>
>> On 22.04.2010, at 12:33, Joerg Roedel wrote:
>>
>>> The patch introducing nested nmi handling had a bug. The
>>> check does not belong to enable_nmi_window but must be in
>>> nmi_allowed. This patch fixes this.
>>>
>>> Signed-off-by: Joerg Roedel <joerg.roedel(a)amd.com>
>>> ---
>>> arch/x86/kvm/svm.c | 16 +++++++++-------
>>> 1 files changed, 9 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>>> index ab78eb8..ec20584 100644
>>> --- a/arch/x86/kvm/svm.c
>>> +++ b/arch/x86/kvm/svm.c
>>> @@ -2771,8 +2771,12 @@ static int svm_nmi_allowed(struct kvm_vcpu *vcpu)
>>> {
>>> struct vcpu_svm *svm = to_svm(vcpu);
>>> struct vmcb *vmcb = svm->vmcb;
>>> - return !(vmcb->control.int_state & SVM_INTERRUPT_SHADOW_MASK) &&
>>> - !(svm->vcpu.arch.hflags & HF_NMI_MASK);
>>> + int ret;
>>> + ret = !(vmcb->control.int_state & SVM_INTERRUPT_SHADOW_MASK) &&
>>> + !(svm->vcpu.arch.hflags & HF_NMI_MASK);
>>> + ret = ret && gif_set(svm) && nested_svm_nmi(svm);
>>> +
>>> + return ret;
>>> }
>>>
>>> static bool svm_get_nmi_mask(struct kvm_vcpu *vcpu)
>>> @@ -2841,11 +2845,9 @@ static void enable_nmi_window(struct kvm_vcpu *vcpu)
>>> * Something prevents NMI from been injected. Single step over possible
>>> * problem (IRET or exception injection or interrupt shadow)
>>> */
>>> - if (gif_set(svm) && nested_svm_nmi(svm)) {
>>> - svm->nmi_singlestep = true;
>>> - svm->vmcb->save.rflags |= (X86_EFLAGS_TF | X86_EFLAGS_RF);
>>> - update_db_intercept(vcpu);
>>> - }
>>> + svm->nmi_singlestep = true;
>>> + svm->vmcb->save.rflags |= (X86_EFLAGS_TF | X86_EFLAGS_RF);
>>> + update_db_intercept(vcpu);
>>
>> So we're always messing with the nested guest state when the host
>> wants to inject an nmi into the l1 guest? Is that safe?
>
> Why not? We can't inject an NMI directly into L2 if the nested
> hypervisor intercepts it.

So where did the code go that does the #vmexit in case the nested hypervisor does intercept it? It used to be nested_svm_nmi(), right?


Alex

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Joerg Roedel on 23 Apr 2010 10:30
On Fri, Apr 23, 2010 at 04:19:40PM +0200, Alexander Graf wrote:
>
> On 23.04.2010, at 16:13, Joerg Roedel wrote:
>
> > On Fri, Apr 23, 2010 at 03:46:07PM +0200, Alexander Graf wrote:
> >>
> >> On 22.04.2010, at 12:33, Joerg Roedel wrote:
> >>
> >>> The patch introducing nested nmi handling had a bug. The
> >>> check does not belong to enable_nmi_window but must be in
> >>> nmi_allowed. This patch fixes this.
> >>>
> >>> Signed-off-by: Joerg Roedel <joerg.roedel(a)amd.com>
> >>> ---
> >>> arch/x86/kvm/svm.c | 16 +++++++++-------
> >>> 1 files changed, 9 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> >>> index ab78eb8..ec20584 100644
> >>> --- a/arch/x86/kvm/svm.c
> >>> +++ b/arch/x86/kvm/svm.c
> >>> @@ -2771,8 +2771,12 @@ static int svm_nmi_allowed(struct kvm_vcpu *vcpu)
> >>> {
> >>> struct vcpu_svm *svm = to_svm(vcpu);
> >>> struct vmcb *vmcb = svm->vmcb;
> >>> - return !(vmcb->control.int_state & SVM_INTERRUPT_SHADOW_MASK) &&
> >>> - !(svm->vcpu.arch.hflags & HF_NMI_MASK);
> >>> + int ret;
> >>> + ret = !(vmcb->control.int_state & SVM_INTERRUPT_SHADOW_MASK) &&
> >>> + !(svm->vcpu.arch.hflags & HF_NMI_MASK);
> >>> + ret = ret && gif_set(svm) && nested_svm_nmi(svm);
> >>> +
> >>> + return ret;
> >>> }
> >>>
> >>> static bool svm_get_nmi_mask(struct kvm_vcpu *vcpu)
> >>> @@ -2841,11 +2845,9 @@ static void enable_nmi_window(struct kvm_vcpu *vcpu)
> >>> * Something prevents NMI from been injected. Single step over possible
> >>> * problem (IRET or exception injection or interrupt shadow)
> >>> */
> >>> - if (gif_set(svm) && nested_svm_nmi(svm)) {
> >>> - svm->nmi_singlestep = true;
> >>> - svm->vmcb->save.rflags |= (X86_EFLAGS_TF | X86_EFLAGS_RF);
> >>> - update_db_intercept(vcpu);
> >>> - }
> >>> + svm->nmi_singlestep = true;
> >>> + svm->vmcb->save.rflags |= (X86_EFLAGS_TF | X86_EFLAGS_RF);
> >>> + update_db_intercept(vcpu);
> >>
> >> So we're always messing with the nested guest state when the host
> >> wants to inject an nmi into the l1 guest? Is that safe?
> >
> > Why not? We can't inject an NMI directly into L2 if the nested
> > hypervisor intercepts it.
>
> So where did the code go that does the #vmexit in case the nested
> hypervisor does intercept it? It used to be nested_svm_nmi(), right?

No, nested_svm_nmi runs in atomic context where we can't emulate a
vmexit. We set exit_required and emulate the vmexit later.

Joerg


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Joerg Roedel on 23 Apr 2010 10:40
On Fri, Apr 23, 2010 at 04:24:54PM +0200, Alexander Graf wrote:
>
> On 23.04.2010, at 16:22, Joerg Roedel wrote:

> > No, nested_svm_nmi runs in atomic context where we can't emulate a
> > vmexit. We set exit_required and emulate the vmexit later.
>
> So we modify the L2 rflags and then trigger a #vmexit, leaving the l2
> state broken?

No, the rflags are changed in enable_nmi_window which isn't called when
we run nested and the nested hypervisor intercepts nmi. So it only runs
in the !nested case where it can't corrupt L2 state.

Joerg


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 |  Next  |  Last
Pages: 1 2
Prev: Another BUG_ON() in blkio controller
Next: KVM: SVM: Sync cr0 and cr3 to kvm state before nested handling