Re: [PATCH V3] vmx/nmi: Do not use self_nmi() in VMEXIT handler

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 28/02/13 13:00, Tim Deegan wrote:
> At 09:58 +0000 on 28 Feb (1362045494), Jan Beulich wrote:
>>>>> On 22.11.12 at 17:12, Jan Beulich wrote:
>>>>>> On 22.11.12 at 17:05, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>>> On 22/11/12 15:55, Jan Beulich wrote:
>>>>>>>> On 22.11.12 at 16:37, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>>>>> A quick solution would be to execute a noop function with
>>>>>> run_in_exception_handler().  Alternatively, I can code enable_nmi() or
>>>>>> so which does an inline iret to itself.  Which of these would you prefer?
>>>>> I would actually consider avoiding to run softirqs altogether in that
>>>>> case, just like native Linux doesn't do any event or scheduler
>>>>> processing in that case.
>>>> That would probably be the easiest solution.
>>>>
>>>> I was already going to do the same for the rentrant NMI and MCE handling
>>>> case (and also the process pending upcalls checking), due to the
>>>> complexities of fixing the race condition at the end of the handler.
>>>>
>>>> Unfortunately, I don't think I have time to look at this issue
>>>> immediately, but if it is ok to wait till the beginning of next week, I
>>> That's fine of course.
>> So that was 3 months ago, and we're likely going to need to do
>> further unfixed releases if this won't move forward. Can you
>> estimate whether you'll be able to get back to this?
> Let's make a step in the right direction before 4.3, at least:
>
> commit d278beed1df2d226911dce92295411018c9bba2f
> Author: Tim Deegan <tim@xen.org>
> Date:   Thu Feb 28 12:42:15 2013 +0000
>
>     vmx: handle NMIs before re-enabling interrupts.
>     
>     Also, switch to calling do_nmi() and explicitly re-enabling NMIs
>     rather than raising a fake NMI and relying on the NMI processing to
>     IRET, since that handling code is likely to change a fair amount in
>     future.
>     
>     Signed-off-by: Tim Deegan <tim@xen.org>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

As this is functionally equivalent to the patch I was just testing.

>
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 5378928..462bb0f 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2313,6 +2313,13 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>          vector = intr_info & INTR_INFO_VECTOR_MASK;
>          if ( vector == TRAP_machine_check )
>              do_machine_check(regs);
> +        if ( vector == TRAP_machine_check
> +             && ((intr_info & INTR_INFO_INTR_TYPE_MASK) ==
> +                 (X86_EVENTTYPE_NMI << 8)) )
> +        {
> +            do_nmi(regs);
> +            enable_nmis();
> +        }
>          break;
>      case EXIT_REASON_MCE_DURING_VMENTRY:
>          do_machine_check(regs);
> @@ -2486,7 +2493,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>                   (X86_EVENTTYPE_NMI << 8) )
>                  goto exit_and_crash;
>              HVMTRACE_0D(NMI);
> -            self_nmi(); /* Real NMI, vector 2: normal processing. */
> +            /* Already handled above. */
>              break;
>          case TRAP_machine_check:
>              HVMTRACE_0D(MCE);