All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] x86emul: add "unblock NMI" retire flag
@ 2017-04-11 15:36 Jan Beulich
  2017-04-11 16:09 ` Andrew Cooper
  0 siblings, 1 reply; 5+ messages in thread
From: Jan Beulich @ 2017-04-11 15:36 UTC (permalink / raw)
  To: xen-devel; +Cc: Andrew Cooper, Julien Grall, Paul Durrant

[-- Attachment #1: Type: text/plain, Size: 4233 bytes --]

No matter that we emulate IRET for (guest) real more only right now, we
should get its effect on (virtual) NMI delivery right.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Adjust x86_emulate_wrapper() accordingly.

--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -1955,25 +1955,32 @@ static int _hvm_emulate_one(struct hvm_e
         memcpy(vio->mmio_insn, hvmemul_ctxt->insn_buf, vio->mmio_insn_bytes);
     }
 
-    if ( rc != X86EMUL_OKAY )
-        return rc;
+    new_intr_shadow = hvmemul_ctxt->intr_shadow;
 
-    if ( hvmemul_ctxt->ctxt.retire.singlestep )
-        hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
+    /*
+     * IRET, if valid in the given context, clears NMI blocking
+     * (irrespective of rc).
+     */
+    if ( hvmemul_ctxt->ctxt.retire.unblock_nmi )
+        new_intr_shadow &= ~HVM_INTR_SHADOW_NMI;
 
-    new_intr_shadow = hvmemul_ctxt->intr_shadow;
+    if ( rc == X86EMUL_OKAY )
+    {
+        if ( hvmemul_ctxt->ctxt.retire.singlestep )
+            hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
 
-    /* MOV-SS instruction toggles MOV-SS shadow, else we just clear it. */
-    if ( hvmemul_ctxt->ctxt.retire.mov_ss )
-        new_intr_shadow ^= HVM_INTR_SHADOW_MOV_SS;
-    else
-        new_intr_shadow &= ~HVM_INTR_SHADOW_MOV_SS;
-
-    /* STI instruction toggles STI shadow, else we just clear it. */
-    if ( hvmemul_ctxt->ctxt.retire.sti )
-        new_intr_shadow ^= HVM_INTR_SHADOW_STI;
-    else
-        new_intr_shadow &= ~HVM_INTR_SHADOW_STI;
+        /* MOV-SS instruction toggles MOV-SS shadow, else we just clear it. */
+        if ( hvmemul_ctxt->ctxt.retire.mov_ss )
+            new_intr_shadow ^= HVM_INTR_SHADOW_MOV_SS;
+        else
+            new_intr_shadow &= ~HVM_INTR_SHADOW_MOV_SS;
+
+        /* STI instruction toggles STI shadow, else we just clear it. */
+        if ( hvmemul_ctxt->ctxt.retire.sti )
+            new_intr_shadow ^= HVM_INTR_SHADOW_STI;
+        else
+            new_intr_shadow &= ~HVM_INTR_SHADOW_STI;
+    }
 
     if ( hvmemul_ctxt->intr_shadow != new_intr_shadow )
     {
@@ -1981,13 +1988,11 @@ static int _hvm_emulate_one(struct hvm_e
         hvm_funcs.set_interrupt_shadow(curr, new_intr_shadow);
     }
 
-    if ( hvmemul_ctxt->ctxt.retire.hlt &&
+    if ( rc == X86EMUL_OKAY && hvmemul_ctxt->ctxt.retire.hlt &&
          !hvm_local_events_need_delivery(curr) )
-    {
         hvm_hlt(regs->eflags);
-    }
 
-    return X86EMUL_OKAY;
+    return rc;
 }
 
 int hvm_emulate_one(
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -4002,6 +4002,7 @@ x86_emulate(
         uint32_t mask = X86_EFLAGS_VIP | X86_EFLAGS_VIF | X86_EFLAGS_VM;
 
         fail_if(!in_realmode(ctxt, ops));
+        ctxt->retire.unblock_nmi = true;
         if ( (rc = read_ulong(x86_seg_ss, sp_post_inc(op_bytes),
                               &eip, op_bytes, ctxt, ops)) ||
              (rc = read_ulong(x86_seg_ss, sp_post_inc(op_bytes),
@@ -7918,9 +7919,17 @@ int x86_emulate_wrapper(
 
     rc = x86_emulate(ctxt, ops);
 
-    /* Retire flags should only be set for successful instruction emulation. */
+    /*
+     * Most retire flags should only be set for successful instruction
+     * emulation.
+     */
     if ( rc != X86EMUL_OKAY )
-        ASSERT(ctxt->retire.raw == 0);
+    {
+        typeof(ctxt->retire) retire = ctxt->retire;
+
+        retire.unblock_nmi = false;
+        ASSERT(!retire.raw);
+    }
 
     /* All cases returning X86EMUL_EXCEPTION should have fault semantics. */
     if ( rc == X86EMUL_EXCEPTION )
--- a/xen/arch/x86/x86_emulate/x86_emulate.h
+++ b/xen/arch/x86/x86_emulate/x86_emulate.h
@@ -490,6 +490,7 @@ struct x86_emulate_ctxt
             bool hlt:1;          /* Instruction HLTed. */
             bool mov_ss:1;       /* Instruction sets MOV-SS irq shadow. */
             bool sti:1;          /* Instruction sets STI irq shadow. */
+            bool unblock_nmi:1;  /* Instruction clears NMI blocking. */
             bool singlestep:1;   /* Singlestepping was active. */
         };
     } retire;



[-- Attachment #2: x86emul-unblock-NMI.patch --]
[-- Type: text/plain, Size: 4271 bytes --]

x86emul: add "unblock NMI" retire flag

No matter that we emulate IRET for (guest) real more only right now, we
should get its effect on (virtual) NMI delivery right.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Adjust x86_emulate_wrapper() accordingly.

--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -1955,25 +1955,32 @@ static int _hvm_emulate_one(struct hvm_e
         memcpy(vio->mmio_insn, hvmemul_ctxt->insn_buf, vio->mmio_insn_bytes);
     }
 
-    if ( rc != X86EMUL_OKAY )
-        return rc;
+    new_intr_shadow = hvmemul_ctxt->intr_shadow;
 
-    if ( hvmemul_ctxt->ctxt.retire.singlestep )
-        hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
+    /*
+     * IRET, if valid in the given context, clears NMI blocking
+     * (irrespective of rc).
+     */
+    if ( hvmemul_ctxt->ctxt.retire.unblock_nmi )
+        new_intr_shadow &= ~HVM_INTR_SHADOW_NMI;
 
-    new_intr_shadow = hvmemul_ctxt->intr_shadow;
+    if ( rc == X86EMUL_OKAY )
+    {
+        if ( hvmemul_ctxt->ctxt.retire.singlestep )
+            hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
 
-    /* MOV-SS instruction toggles MOV-SS shadow, else we just clear it. */
-    if ( hvmemul_ctxt->ctxt.retire.mov_ss )
-        new_intr_shadow ^= HVM_INTR_SHADOW_MOV_SS;
-    else
-        new_intr_shadow &= ~HVM_INTR_SHADOW_MOV_SS;
-
-    /* STI instruction toggles STI shadow, else we just clear it. */
-    if ( hvmemul_ctxt->ctxt.retire.sti )
-        new_intr_shadow ^= HVM_INTR_SHADOW_STI;
-    else
-        new_intr_shadow &= ~HVM_INTR_SHADOW_STI;
+        /* MOV-SS instruction toggles MOV-SS shadow, else we just clear it. */
+        if ( hvmemul_ctxt->ctxt.retire.mov_ss )
+            new_intr_shadow ^= HVM_INTR_SHADOW_MOV_SS;
+        else
+            new_intr_shadow &= ~HVM_INTR_SHADOW_MOV_SS;
+
+        /* STI instruction toggles STI shadow, else we just clear it. */
+        if ( hvmemul_ctxt->ctxt.retire.sti )
+            new_intr_shadow ^= HVM_INTR_SHADOW_STI;
+        else
+            new_intr_shadow &= ~HVM_INTR_SHADOW_STI;
+    }
 
     if ( hvmemul_ctxt->intr_shadow != new_intr_shadow )
     {
@@ -1981,13 +1988,11 @@ static int _hvm_emulate_one(struct hvm_e
         hvm_funcs.set_interrupt_shadow(curr, new_intr_shadow);
     }
 
-    if ( hvmemul_ctxt->ctxt.retire.hlt &&
+    if ( rc == X86EMUL_OKAY && hvmemul_ctxt->ctxt.retire.hlt &&
          !hvm_local_events_need_delivery(curr) )
-    {
         hvm_hlt(regs->eflags);
-    }
 
-    return X86EMUL_OKAY;
+    return rc;
 }
 
 int hvm_emulate_one(
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -4002,6 +4002,7 @@ x86_emulate(
         uint32_t mask = X86_EFLAGS_VIP | X86_EFLAGS_VIF | X86_EFLAGS_VM;
 
         fail_if(!in_realmode(ctxt, ops));
+        ctxt->retire.unblock_nmi = true;
         if ( (rc = read_ulong(x86_seg_ss, sp_post_inc(op_bytes),
                               &eip, op_bytes, ctxt, ops)) ||
              (rc = read_ulong(x86_seg_ss, sp_post_inc(op_bytes),
@@ -7918,9 +7919,17 @@ int x86_emulate_wrapper(
 
     rc = x86_emulate(ctxt, ops);
 
-    /* Retire flags should only be set for successful instruction emulation. */
+    /*
+     * Most retire flags should only be set for successful instruction
+     * emulation.
+     */
     if ( rc != X86EMUL_OKAY )
-        ASSERT(ctxt->retire.raw == 0);
+    {
+        typeof(ctxt->retire) retire = ctxt->retire;
+
+        retire.unblock_nmi = false;
+        ASSERT(!retire.raw);
+    }
 
     /* All cases returning X86EMUL_EXCEPTION should have fault semantics. */
     if ( rc == X86EMUL_EXCEPTION )
--- a/xen/arch/x86/x86_emulate/x86_emulate.h
+++ b/xen/arch/x86/x86_emulate/x86_emulate.h
@@ -490,6 +490,7 @@ struct x86_emulate_ctxt
             bool hlt:1;          /* Instruction HLTed. */
             bool mov_ss:1;       /* Instruction sets MOV-SS irq shadow. */
             bool sti:1;          /* Instruction sets STI irq shadow. */
+            bool unblock_nmi:1;  /* Instruction clears NMI blocking. */
             bool singlestep:1;   /* Singlestepping was active. */
         };
     } retire;

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH v2] x86emul: add "unblock NMI" retire flag
  2017-04-11 15:36 [PATCH v2] x86emul: add "unblock NMI" retire flag Jan Beulich
@ 2017-04-11 16:09 ` Andrew Cooper
  2017-04-12  7:43   ` Jan Beulich
  0 siblings, 1 reply; 5+ messages in thread
From: Andrew Cooper @ 2017-04-11 16:09 UTC (permalink / raw)
  To: Jan Beulich, xen-devel; +Cc: Julien Grall, Paul Durrant

On 11/04/17 16:36, Jan Beulich wrote:
> No matter that we emulate IRET for (guest) real more only right now, we
> should get its effect on (virtual) NMI delivery right.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: Adjust x86_emulate_wrapper() accordingly.
>
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -1955,25 +1955,32 @@ static int _hvm_emulate_one(struct hvm_e
>          memcpy(vio->mmio_insn, hvmemul_ctxt->insn_buf, vio->mmio_insn_bytes);
>      }
>  
> -    if ( rc != X86EMUL_OKAY )
> -        return rc;
> +    new_intr_shadow = hvmemul_ctxt->intr_shadow;
>  
> -    if ( hvmemul_ctxt->ctxt.retire.singlestep )
> -        hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
> +    /*
> +     * IRET, if valid in the given context, clears NMI blocking
> +     * (irrespective of rc).
> +     */
> +    if ( hvmemul_ctxt->ctxt.retire.unblock_nmi )
> +        new_intr_shadow &= ~HVM_INTR_SHADOW_NMI;
>  
> -    new_intr_shadow = hvmemul_ctxt->intr_shadow;
> +    if ( rc == X86EMUL_OKAY )
> +    {

On further thought, given the assertion, you don't need to introduce
this check, and can avoid the block indentation.  It should make the
patch rather smaller.

> +        if ( hvmemul_ctxt->ctxt.retire.singlestep )
> +            hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
>  
> -    /* MOV-SS instruction toggles MOV-SS shadow, else we just clear it. */
> -    if ( hvmemul_ctxt->ctxt.retire.mov_ss )
> -        new_intr_shadow ^= HVM_INTR_SHADOW_MOV_SS;
> -    else
> -        new_intr_shadow &= ~HVM_INTR_SHADOW_MOV_SS;
> -
> -    /* STI instruction toggles STI shadow, else we just clear it. */
> -    if ( hvmemul_ctxt->ctxt.retire.sti )
> -        new_intr_shadow ^= HVM_INTR_SHADOW_STI;
> -    else
> -        new_intr_shadow &= ~HVM_INTR_SHADOW_STI;
> +        /* MOV-SS instruction toggles MOV-SS shadow, else we just clear it. */
> +        if ( hvmemul_ctxt->ctxt.retire.mov_ss )
> +            new_intr_shadow ^= HVM_INTR_SHADOW_MOV_SS;
> +        else
> +            new_intr_shadow &= ~HVM_INTR_SHADOW_MOV_SS;
> +
> +        /* STI instruction toggles STI shadow, else we just clear it. */
> +        if ( hvmemul_ctxt->ctxt.retire.sti )
> +            new_intr_shadow ^= HVM_INTR_SHADOW_STI;
> +        else
> +            new_intr_shadow &= ~HVM_INTR_SHADOW_STI;
> +    }
>  
>      if ( hvmemul_ctxt->intr_shadow != new_intr_shadow )
>      {
> @@ -1981,13 +1988,11 @@ static int _hvm_emulate_one(struct hvm_e
>          hvm_funcs.set_interrupt_shadow(curr, new_intr_shadow);
>      }
>  
> -    if ( hvmemul_ctxt->ctxt.retire.hlt &&
> +    if ( rc == X86EMUL_OKAY && hvmemul_ctxt->ctxt.retire.hlt &&
>           !hvm_local_events_need_delivery(curr) )

Similarly, you don't need the OKAY check here.

Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

> -    {
>          hvm_hlt(regs->eflags);
> -    }
>  
> -    return X86EMUL_OKAY;
> +    return rc;
>  }
>  
>  int hvm_emulate_one(
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -4002,6 +4002,7 @@ x86_emulate(
>          uint32_t mask = X86_EFLAGS_VIP | X86_EFLAGS_VIF | X86_EFLAGS_VM;
>  
>          fail_if(!in_realmode(ctxt, ops));
> +        ctxt->retire.unblock_nmi = true;
>          if ( (rc = read_ulong(x86_seg_ss, sp_post_inc(op_bytes),
>                                &eip, op_bytes, ctxt, ops)) ||
>               (rc = read_ulong(x86_seg_ss, sp_post_inc(op_bytes),
> @@ -7918,9 +7919,17 @@ int x86_emulate_wrapper(
>  
>      rc = x86_emulate(ctxt, ops);
>  
> -    /* Retire flags should only be set for successful instruction emulation. */
> +    /*
> +     * Most retire flags should only be set for successful instruction
> +     * emulation.
> +     */
>      if ( rc != X86EMUL_OKAY )
> -        ASSERT(ctxt->retire.raw == 0);
> +    {
> +        typeof(ctxt->retire) retire = ctxt->retire;
> +
> +        retire.unblock_nmi = false;
> +        ASSERT(!retire.raw);
> +    }
>  
>      /* All cases returning X86EMUL_EXCEPTION should have fault semantics. */
>      if ( rc == X86EMUL_EXCEPTION )
> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
> @@ -490,6 +490,7 @@ struct x86_emulate_ctxt
>              bool hlt:1;          /* Instruction HLTed. */
>              bool mov_ss:1;       /* Instruction sets MOV-SS irq shadow. */
>              bool sti:1;          /* Instruction sets STI irq shadow. */
> +            bool unblock_nmi:1;  /* Instruction clears NMI blocking. */
>              bool singlestep:1;   /* Singlestepping was active. */
>          };
>      } retire;
>
>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH v2] x86emul: add "unblock NMI" retire flag
  2017-04-11 16:09 ` Andrew Cooper
@ 2017-04-12  7:43   ` Jan Beulich
  2017-04-12  8:06     ` Andrew Cooper
  0 siblings, 1 reply; 5+ messages in thread
From: Jan Beulich @ 2017-04-12  7:43 UTC (permalink / raw)
  To: Andrew Cooper, Paul Durrant; +Cc: xen-devel, Julien Grall

>>> On 11.04.17 at 18:09, <andrew.cooper3@citrix.com> wrote:
> On 11/04/17 16:36, Jan Beulich wrote:
>> --- a/xen/arch/x86/hvm/emulate.c
>> +++ b/xen/arch/x86/hvm/emulate.c
>> @@ -1955,25 +1955,32 @@ static int _hvm_emulate_one(struct hvm_e
>>          memcpy(vio->mmio_insn, hvmemul_ctxt->insn_buf, vio->mmio_insn_bytes);
>>      }
>>  
>> -    if ( rc != X86EMUL_OKAY )
>> -        return rc;
>> +    new_intr_shadow = hvmemul_ctxt->intr_shadow;
>>  
>> -    if ( hvmemul_ctxt->ctxt.retire.singlestep )
>> -        hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
>> +    /*
>> +     * IRET, if valid in the given context, clears NMI blocking
>> +     * (irrespective of rc).
>> +     */
>> +    if ( hvmemul_ctxt->ctxt.retire.unblock_nmi )
>> +        new_intr_shadow &= ~HVM_INTR_SHADOW_NMI;
>>  
>> -    new_intr_shadow = hvmemul_ctxt->intr_shadow;
>> +    if ( rc == X86EMUL_OKAY )
>> +    {
> 
> On further thought, given the assertion, you don't need to introduce
> this check, and can avoid the block indentation.  It should make the
> patch rather smaller.

Hmm, I did consider this, but I feel uneasy doing so, as it leaves
production builds in potentially bad shape (in case we have a path
we don't ever execute in any of the routine testing done, but
which someone nevertheless ends up coming down in a released
product). Paul, you're the maintainer of the code, do you have an
opinion either way?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH v2] x86emul: add "unblock NMI" retire flag
  2017-04-12  7:43   ` Jan Beulich
@ 2017-04-12  8:06     ` Andrew Cooper
  2017-04-12  8:42       ` Jan Beulich
  0 siblings, 1 reply; 5+ messages in thread
From: Andrew Cooper @ 2017-04-12  8:06 UTC (permalink / raw)
  To: Jan Beulich, Paul Durrant; +Cc: xen-devel, Julien Grall

On 12/04/2017 08:43, Jan Beulich wrote:
>>>> On 11.04.17 at 18:09, <andrew.cooper3@citrix.com> wrote:
>> On 11/04/17 16:36, Jan Beulich wrote:
>>> --- a/xen/arch/x86/hvm/emulate.c
>>> +++ b/xen/arch/x86/hvm/emulate.c
>>> @@ -1955,25 +1955,32 @@ static int _hvm_emulate_one(struct hvm_e
>>>          memcpy(vio->mmio_insn, hvmemul_ctxt->insn_buf, vio->mmio_insn_bytes);
>>>      }
>>>  
>>> -    if ( rc != X86EMUL_OKAY )
>>> -        return rc;
>>> +    new_intr_shadow = hvmemul_ctxt->intr_shadow;
>>>  
>>> -    if ( hvmemul_ctxt->ctxt.retire.singlestep )
>>> -        hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
>>> +    /*
>>> +     * IRET, if valid in the given context, clears NMI blocking
>>> +     * (irrespective of rc).
>>> +     */
>>> +    if ( hvmemul_ctxt->ctxt.retire.unblock_nmi )
>>> +        new_intr_shadow &= ~HVM_INTR_SHADOW_NMI;
>>>  
>>> -    new_intr_shadow = hvmemul_ctxt->intr_shadow;
>>> +    if ( rc == X86EMUL_OKAY )
>>> +    {
>> On further thought, given the assertion, you don't need to introduce
>> this check, and can avoid the block indentation.  It should make the
>> patch rather smaller.
> Hmm, I did consider this, but I feel uneasy doing so, as it leaves
> production builds in potentially bad shape (in case we have a path
> we don't ever execute in any of the routine testing done, but
> which someone nevertheless ends up coming down in a released
> product). Paul, you're the maintainer of the code, do you have an
> opinion either way?

Actually, on yet further thought, the reset nature of these conditions
are actually necessary on the != OKAY path.

Consider "mov %eax, %ss; ud2a".  Even on raising an exception for the
ud2a, the SS shadow needs dropping.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH v2] x86emul: add "unblock NMI" retire flag
  2017-04-12  8:06     ` Andrew Cooper
@ 2017-04-12  8:42       ` Jan Beulich
  0 siblings, 0 replies; 5+ messages in thread
From: Jan Beulich @ 2017-04-12  8:42 UTC (permalink / raw)
  To: Andrew Cooper, Paul Durrant; +Cc: xen-devel, Julien Grall

>>> On 12.04.17 at 10:06, <andrew.cooper3@citrix.com> wrote:
> On 12/04/2017 08:43, Jan Beulich wrote:
>>>>> On 11.04.17 at 18:09, <andrew.cooper3@citrix.com> wrote:
>>> On 11/04/17 16:36, Jan Beulich wrote:
>>>> --- a/xen/arch/x86/hvm/emulate.c
>>>> +++ b/xen/arch/x86/hvm/emulate.c
>>>> @@ -1955,25 +1955,32 @@ static int _hvm_emulate_one(struct hvm_e
>>>>          memcpy(vio->mmio_insn, hvmemul_ctxt->insn_buf, vio->mmio_insn_bytes);
>>>>      }
>>>>  
>>>> -    if ( rc != X86EMUL_OKAY )
>>>> -        return rc;
>>>> +    new_intr_shadow = hvmemul_ctxt->intr_shadow;
>>>>  
>>>> -    if ( hvmemul_ctxt->ctxt.retire.singlestep )
>>>> -        hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
>>>> +    /*
>>>> +     * IRET, if valid in the given context, clears NMI blocking
>>>> +     * (irrespective of rc).
>>>> +     */
>>>> +    if ( hvmemul_ctxt->ctxt.retire.unblock_nmi )
>>>> +        new_intr_shadow &= ~HVM_INTR_SHADOW_NMI;
>>>>  
>>>> -    new_intr_shadow = hvmemul_ctxt->intr_shadow;
>>>> +    if ( rc == X86EMUL_OKAY )
>>>> +    {
>>> On further thought, given the assertion, you don't need to introduce
>>> this check, and can avoid the block indentation.  It should make the
>>> patch rather smaller.
>> Hmm, I did consider this, but I feel uneasy doing so, as it leaves
>> production builds in potentially bad shape (in case we have a path
>> we don't ever execute in any of the routine testing done, but
>> which someone nevertheless ends up coming down in a released
>> product). Paul, you're the maintainer of the code, do you have an
>> opinion either way?
> 
> Actually, on yet further thought, the reset nature of these conditions
> are actually necessary on the != OKAY path.
> 
> Consider "mov %eax, %ss; ud2a".  Even on raising an exception for the
> ud2a, the SS shadow needs dropping.

Hmm, indeed (for EXCEPTION at least, not so sure about the others;
quite certainly not for RETRY). Along the lines of the above this would
then call for an "else" to the newly added "if ( rc == X86EMUL_OKAY )"
though.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-04-12  8:42 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-11 15:36 [PATCH v2] x86emul: add "unblock NMI" retire flag Jan Beulich
2017-04-11 16:09 ` Andrew Cooper
2017-04-12  7:43   ` Jan Beulich
2017-04-12  8:06     ` Andrew Cooper
2017-04-12  8:42       ` Jan Beulich

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.