[PATCH] SVM: limit GIF=0 region

[PATCH] SVM: limit GIF=0 region

[Jan Beulich]

Use EFLAGS.IF for most ordinary purposes; there's in particular no need
to unduly defer NMI/#MC. Clear GIF only immediately before VMRUN itself.
This has the additional advantage that svm_stgi_label now indeed marks
the only place where GIF gets set.

Note regarding the main STI placement: Quite counterintuitively the
host's EFLAGS.IF continues to have a meaning while the guest runs; see
PM Vol 2 section "Physical (INTR) Interrupt Masking in EFLAGS". Hence we
need to set the flag for the duration of time being in guest context.
However, SPEC_CTRL_ENTRY_FROM_HVM wants to be carried out with EFLAGS.IF
clear.

Note regarding the main STGI placement: It could be moved further up,
but at present SPEC_CTRL_EXIT_TO_HVM is not NMI/#MC-safe.

Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v3: Keep main STGI at its current place, and explain why that is (I'm
    sorry Boris, had to drop your R-b yet another time).
v2: Add CLI after VMRUN. Adjust description.

--- a/xen/arch/x86/hvm/svm/entry.S
+++ b/xen/arch/x86/hvm/svm/entry.S
@@ -43,7 +43,7 @@ ENTRY(svm_asm_do_resume)
         lea  irq_stat+IRQSTAT_softirq_pending(%rip),%rdx
         xor  %ecx,%ecx
         shl  $IRQSTAT_shift,%eax
-        CLGI
+        cli
         cmp  %ecx,(%rdx,%rax,1)
         jne  .Lsvm_process_softirqs
 
@@ -57,7 +57,7 @@ UNLIKELY_START(ne, nsvm_hap)
          * Someone shot down our nested p2m table; go round again
          * and nsvm_vcpu_switch() will fix it for us.
          */
-        STGI
+        sti
         jmp  .Lsvm_do_resume
 __UNLIKELY_END(nsvm_hap)
 
@@ -87,6 +87,8 @@ __UNLIKELY_END(nsvm_hap)
         pop  %rsi
         pop  %rdi
 
+        CLGI
+        sti
         VMRUN
 
         SAVE_ALL
@@ -103,6 +105,6 @@ GLOBAL(svm_stgi_label)
         jmp  .Lsvm_do_resume
 
 .Lsvm_process_softirqs:
-        STGI
+        sti
         call do_softirq
         jmp  .Lsvm_do_resume





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

Re: [PATCH] SVM: limit GIF=0 region

[Boris Ostrovsky]

On 09/10/2018 11:02 AM, Jan Beulich wrote:
> Use EFLAGS.IF for most ordinary purposes; there's in particular no need
> to unduly defer NMI/#MC. Clear GIF only immediately before VMRUN itself.
> This has the additional advantage that svm_stgi_label now indeed marks
> the only place where GIF gets set.
>
> Note regarding the main STI placement: Quite counterintuitively the
> host's EFLAGS.IF continues to have a meaning while the guest runs; see
> PM Vol 2 section "Physical (INTR) Interrupt Masking in EFLAGS". Hence we
> need to set the flag for the duration of time being in guest context.
> However, SPEC_CTRL_ENTRY_FROM_HVM wants to be carried out with EFLAGS.IF
> clear.
>
> Note regarding the main STGI placement: It could be moved further up,
> but at present SPEC_CTRL_EXIT_TO_HVM is not NMI/#MC-safe.
>
> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>



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

[PATCH] SVM: limit GIF=0 region

[Jan Beulich]

Use EFLAGS.IF for all ordinary purposes; there's in particular no need
to unduly defer NMI/#MC. Clear/set GIF solely around VMRUN itself. This
has the additional advantage that svm_stgi_label now indeed marks the
only place where GIF is being set.

A note regarding the main STI placement: Orignally I had it at the place
the main STGI was sitting at so far. However, my Fam15 box reliably
locks up hard with this, unless I have the NMI watchdog enabled. I can
only deduce that the CPU doesn't like STGI with EFLAGS.IF clear plus
some other condition (the lockup occurs only after exiting the boot
loader in the guest). As there's nothing wrong with interrupts being on
right after VMRUN, I've decided to put the STI right after the CLGI
(matching what KVM does, i.e. having a fair chance of working
everywhere).

Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/svm/entry.S
+++ b/xen/arch/x86/hvm/svm/entry.S
@@ -43,7 +43,7 @@ ENTRY(svm_asm_do_resume)
         lea  irq_stat+IRQSTAT_softirq_pending(%rip),%rdx
         xor  %ecx,%ecx
         shl  $IRQSTAT_shift,%eax
-        CLGI
+        cli
         cmp  %ecx,(%rdx,%rax,1)
         jne  .Lsvm_process_softirqs
 
@@ -57,7 +57,7 @@ UNLIKELY_START(ne, nsvm_hap)
          * Someone shot down our nested p2m table; go round again
          * and nsvm_vcpu_switch() will fix it for us.
          */
-        STGI
+        sti
         jmp  .Lsvm_do_resume
 __UNLIKELY_END(nsvm_hap)
 
@@ -87,7 +87,11 @@ __UNLIKELY_END(nsvm_hap)
         pop  %rsi
         pop  %rdi
 
+        CLGI
+        sti
         VMRUN
+        STGI
+GLOBAL(svm_stgi_label)
 
         SAVE_ALL
 
@@ -96,13 +100,11 @@ __UNLIKELY_END(nsvm_hap)
         SPEC_CTRL_ENTRY_FROM_HVM    /* Req: b=curr %rsp=regs/cpuinfo, Clob: acd */
         /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
 
-        STGI
-GLOBAL(svm_stgi_label)
         mov  %rsp,%rdi
         call svm_vmexit_handler
         jmp  .Lsvm_do_resume
 
 .Lsvm_process_softirqs:
-        STGI
+        sti
         call do_softirq
         jmp  .Lsvm_do_resume





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

Re: [PATCH] SVM: limit GIF=0 region

[Andrew Cooper]

On 26/06/2018 08:32, Jan Beulich wrote:
> Use EFLAGS.IF for all ordinary purposes; there's in particular no need
> to unduly defer NMI/#MC. Clear/set GIF solely around VMRUN itself. This
> has the additional advantage that svm_stgi_label now indeed marks the
> only place where GIF is being set.
>
> A note regarding the main STI placement: Orignally I had it at the place
> the main STGI was sitting at so far. However, my Fam15 box reliably
> locks up hard with this, unless I have the NMI watchdog enabled. I can
> only deduce that the CPU doesn't like STGI with EFLAGS.IF clear plus
> some other condition (the lockup occurs only after exiting the boot
> loader in the guest). As there's nothing wrong with interrupts being on
> right after VMRUN, I've decided to put the STI right after the CLGI
> (matching what KVM does, i.e. having a fair chance of working
> everywhere).

I'd like some input from AMD on this observation, because it is
completely bizarre.

>
> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/arch/x86/hvm/svm/entry.S
> +++ b/xen/arch/x86/hvm/svm/entry.S
> @@ -43,7 +43,7 @@ ENTRY(svm_asm_do_resume)
>          lea  irq_stat+IRQSTAT_softirq_pending(%rip),%rdx
>          xor  %ecx,%ecx
>          shl  $IRQSTAT_shift,%eax
> -        CLGI
> +        cli
>          cmp  %ecx,(%rdx,%rax,1)
>          jne  .Lsvm_process_softirqs
>  
> @@ -57,7 +57,7 @@ UNLIKELY_START(ne, nsvm_hap)
>           * Someone shot down our nested p2m table; go round again
>           * and nsvm_vcpu_switch() will fix it for us.
>           */
> -        STGI
> +        sti
>          jmp  .Lsvm_do_resume
>  __UNLIKELY_END(nsvm_hap)
>  
> @@ -87,7 +87,11 @@ __UNLIKELY_END(nsvm_hap)
>          pop  %rsi
>          pop  %rdi
>  
> +        CLGI

As an observation, I'm going to need to insert a call to C here, to fix
LBR handling on pre-LBR-virt capable hardware.  Yet another of the many
many broken things with debug handling.

> +        sti
>          VMRUN
> +        STGI
> +GLOBAL(svm_stgi_label)
>  
>          SAVE_ALL
>  
> @@ -96,13 +100,11 @@ __UNLIKELY_END(nsvm_hap)
>          SPEC_CTRL_ENTRY_FROM_HVM    /* Req: b=curr %rsp=regs/cpuinfo, Clob: acd */
>          /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
>  
> -        STGI
> -GLOBAL(svm_stgi_label)

Unfortunately, the stgi label must remain here. 
SPEC_CTRL_ENTRY_FROM_HVM depends on NMIs being blocked to avoid
corrupting guest state.

Otherwise, LGTM.

~Andrew

>          mov  %rsp,%rdi
>          call svm_vmexit_handler
>          jmp  .Lsvm_do_resume
>  
>  .Lsvm_process_softirqs:
> -        STGI
> +        sti
>          call do_softirq
>          jmp  .Lsvm_do_resume
>
>
>
>


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

Re: [PATCH] SVM: limit GIF=0 region

[Jan Beulich]

>>> On 26.06.18 at 10:45, <andrew.cooper3@citrix.com> wrote:
> On 26/06/2018 08:32, Jan Beulich wrote:
>> Use EFLAGS.IF for all ordinary purposes; there's in particular no need
>> to unduly defer NMI/#MC. Clear/set GIF solely around VMRUN itself. This
>> has the additional advantage that svm_stgi_label now indeed marks the
>> only place where GIF is being set.
>>
>> A note regarding the main STI placement: Orignally I had it at the place
>> the main STGI was sitting at so far. However, my Fam15 box reliably
>> locks up hard with this, unless I have the NMI watchdog enabled. I can
>> only deduce that the CPU doesn't like STGI with EFLAGS.IF clear plus
>> some other condition (the lockup occurs only after exiting the boot
>> loader in the guest). As there's nothing wrong with interrupts being on
>> right after VMRUN, I've decided to put the STI right after the CLGI
>> (matching what KVM does, i.e. having a fair chance of working
>> everywhere).
> 
> I'd like some input from AMD on this observation, because it is
> completely bizarre.

Would be nice indeed.

>> @@ -96,13 +100,11 @@ __UNLIKELY_END(nsvm_hap)
>>          SPEC_CTRL_ENTRY_FROM_HVM    /* Req: b=curr %rsp=regs/cpuinfo, Clob: acd */
>>          /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
>>  
>> -        STGI
>> -GLOBAL(svm_stgi_label)
> 
> Unfortunately, the stgi label must remain here. 
> SPEC_CTRL_ENTRY_FROM_HVM depends on NMIs being blocked to avoid
> corrupting guest state.

I'm afraid I don't follow: How are things safe then without NMIs blocked
for VMX?

Jan



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

Re: [PATCH] SVM: limit GIF=0 region

[Andrew Cooper]

On 26/06/18 10:52, Jan Beulich wrote:
>>>> On 26.06.18 at 10:45, <andrew.cooper3@citrix.com> wrote:
>> On 26/06/2018 08:32, Jan Beulich wrote:
>>> Use EFLAGS.IF for all ordinary purposes; there's in particular no need
>>> to unduly defer NMI/#MC. Clear/set GIF solely around VMRUN itself. This
>>> has the additional advantage that svm_stgi_label now indeed marks the
>>> only place where GIF is being set.
>>>
>>> A note regarding the main STI placement: Orignally I had it at the place
>>> the main STGI was sitting at so far. However, my Fam15 box reliably
>>> locks up hard with this, unless I have the NMI watchdog enabled. I can
>>> only deduce that the CPU doesn't like STGI with EFLAGS.IF clear plus
>>> some other condition (the lockup occurs only after exiting the boot
>>> loader in the guest). As there's nothing wrong with interrupts being on
>>> right after VMRUN, I've decided to put the STI right after the CLGI
>>> (matching what KVM does, i.e. having a fair chance of working
>>> everywhere).
>> I'd like some input from AMD on this observation, because it is
>> completely bizarre.
> Would be nice indeed.
>
>>> @@ -96,13 +100,11 @@ __UNLIKELY_END(nsvm_hap)
>>>          SPEC_CTRL_ENTRY_FROM_HVM    /* Req: b=curr %rsp=regs/cpuinfo, Clob: acd */
>>>          /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
>>>  
>>> -        STGI
>>> -GLOBAL(svm_stgi_label)
>> Unfortunately, the stgi label must remain here. 
>> SPEC_CTRL_ENTRY_FROM_HVM depends on NMIs being blocked to avoid
>> corrupting guest state.
> I'm afraid I don't follow: How are things safe then without NMIs blocked
> for VMX?

27.5.5 VMExits > Updating Non-Register State

VM exits caused directly by non-maskable interrupts (NMIs) cause
blocking by NMI (see Table 24-3). Other VM exits do not affect blocking
by NMI.

~Andrew

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

Re: [PATCH] SVM: limit GIF=0 region

[Jan Beulich]

>>> On 26.06.18 at 12:52, <andrew.cooper3@citrix.com> wrote:
> On 26/06/18 10:52, Jan Beulich wrote:
>>>>> On 26.06.18 at 10:45, <andrew.cooper3@citrix.com> wrote:
>>> On 26/06/2018 08:32, Jan Beulich wrote:
>>>> Use EFLAGS.IF for all ordinary purposes; there's in particular no need
>>>> to unduly defer NMI/#MC. Clear/set GIF solely around VMRUN itself. This
>>>> has the additional advantage that svm_stgi_label now indeed marks the
>>>> only place where GIF is being set.
>>>>
>>>> A note regarding the main STI placement: Orignally I had it at the place
>>>> the main STGI was sitting at so far. However, my Fam15 box reliably
>>>> locks up hard with this, unless I have the NMI watchdog enabled. I can
>>>> only deduce that the CPU doesn't like STGI with EFLAGS.IF clear plus
>>>> some other condition (the lockup occurs only after exiting the boot
>>>> loader in the guest). As there's nothing wrong with interrupts being on
>>>> right after VMRUN, I've decided to put the STI right after the CLGI
>>>> (matching what KVM does, i.e. having a fair chance of working
>>>> everywhere).
>>> I'd like some input from AMD on this observation, because it is
>>> completely bizarre.
>> Would be nice indeed.
>>
>>>> @@ -96,13 +100,11 @@ __UNLIKELY_END(nsvm_hap)
>>>>          SPEC_CTRL_ENTRY_FROM_HVM    /* Req: b=curr %rsp=regs/cpuinfo, Clob: acd */
>>>>          /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
>>>>  
>>>> -        STGI
>>>> -GLOBAL(svm_stgi_label)
>>> Unfortunately, the stgi label must remain here. 
>>> SPEC_CTRL_ENTRY_FROM_HVM depends on NMIs being blocked to avoid
>>> corrupting guest state.
>> I'm afraid I don't follow: How are things safe then without NMIs blocked
>> for VMX?
> 
> 27.5.5 VMExits > Updating Non-Register State
> 
> VM exits caused directly by non-maskable interrupts (NMIs) cause
> blocking by NMI (see Table 24-3). Other VM exits do not affect blocking
> by NMI.

Telling us that an NMI can occur at any point between
vmx_asm_vmexit_handler and the point where guest state was
saved in SPEC_CTRL_ENTRY_FROM_HVM. So afaict if the macro is
indeed unsafe right now, it's the macro that needs to change,
not the patch we're discussing here.

Jan



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

Re: [PATCH] SVM: limit GIF=0 region

[Andrew Cooper]

On 26/06/18 12:50, Jan Beulich wrote:
>>>> On 26.06.18 at 12:52, <andrew.cooper3@citrix.com> wrote:
>> On 26/06/18 10:52, Jan Beulich wrote:
>>>>>> On 26.06.18 at 10:45, <andrew.cooper3@citrix.com> wrote:
>>>> On 26/06/2018 08:32, Jan Beulich wrote:
>>>>> Use EFLAGS.IF for all ordinary purposes; there's in particular no need
>>>>> to unduly defer NMI/#MC. Clear/set GIF solely around VMRUN itself. This
>>>>> has the additional advantage that svm_stgi_label now indeed marks the
>>>>> only place where GIF is being set.
>>>>>
>>>>> A note regarding the main STI placement: Orignally I had it at the place
>>>>> the main STGI was sitting at so far. However, my Fam15 box reliably
>>>>> locks up hard with this, unless I have the NMI watchdog enabled. I can
>>>>> only deduce that the CPU doesn't like STGI with EFLAGS.IF clear plus
>>>>> some other condition (the lockup occurs only after exiting the boot
>>>>> loader in the guest). As there's nothing wrong with interrupts being on
>>>>> right after VMRUN, I've decided to put the STI right after the CLGI
>>>>> (matching what KVM does, i.e. having a fair chance of working
>>>>> everywhere).
>>>> I'd like some input from AMD on this observation, because it is
>>>> completely bizarre.
>>> Would be nice indeed.
>>>
>>>>> @@ -96,13 +100,11 @@ __UNLIKELY_END(nsvm_hap)
>>>>>          SPEC_CTRL_ENTRY_FROM_HVM    /* Req: b=curr %rsp=regs/cpuinfo, Clob: acd */
>>>>>          /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
>>>>>  
>>>>> -        STGI
>>>>> -GLOBAL(svm_stgi_label)
>>>> Unfortunately, the stgi label must remain here. 
>>>> SPEC_CTRL_ENTRY_FROM_HVM depends on NMIs being blocked to avoid
>>>> corrupting guest state.
>>> I'm afraid I don't follow: How are things safe then without NMIs blocked
>>> for VMX?
>> 27.5.5 VMExits > Updating Non-Register State
>>
>> VM exits caused directly by non-maskable interrupts (NMIs) cause
>> blocking by NMI (see Table 24-3). Other VM exits do not affect blocking
>> by NMI.
> Telling us that an NMI can occur at any point between
> vmx_asm_vmexit_handler and the point where guest state was
> saved in SPEC_CTRL_ENTRY_FROM_HVM. So afaict if the macro is
> indeed unsafe right now, it's the macro that needs to change,
> not the patch we're discussing here.

No... It tells you the exact opposite.

NMIs are not delivered while the NMI shadow is in effect, and is why we
deliberately execute an IRET in the NMI path in vmx_vmexit_handler()

~Andrew

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

Re: [PATCH] SVM: limit GIF=0 region

[Andrew Cooper]

On 26/06/18 12:53, Andrew Cooper wrote:
> On 26/06/18 12:50, Jan Beulich wrote:
>>>>> On 26.06.18 at 12:52, <andrew.cooper3@citrix.com> wrote:
>>> On 26/06/18 10:52, Jan Beulich wrote:
>>>>>>> On 26.06.18 at 10:45, <andrew.cooper3@citrix.com> wrote:
>>>>> On 26/06/2018 08:32, Jan Beulich wrote:
>>>>>> Use EFLAGS.IF for all ordinary purposes; there's in particular no need
>>>>>> to unduly defer NMI/#MC. Clear/set GIF solely around VMRUN itself. This
>>>>>> has the additional advantage that svm_stgi_label now indeed marks the
>>>>>> only place where GIF is being set.
>>>>>>
>>>>>> A note regarding the main STI placement: Orignally I had it at the place
>>>>>> the main STGI was sitting at so far. However, my Fam15 box reliably
>>>>>> locks up hard with this, unless I have the NMI watchdog enabled. I can
>>>>>> only deduce that the CPU doesn't like STGI with EFLAGS.IF clear plus
>>>>>> some other condition (the lockup occurs only after exiting the boot
>>>>>> loader in the guest). As there's nothing wrong with interrupts being on
>>>>>> right after VMRUN, I've decided to put the STI right after the CLGI
>>>>>> (matching what KVM does, i.e. having a fair chance of working
>>>>>> everywhere).
>>>>> I'd like some input from AMD on this observation, because it is
>>>>> completely bizarre.
>>>> Would be nice indeed.
>>>>
>>>>>> @@ -96,13 +100,11 @@ __UNLIKELY_END(nsvm_hap)
>>>>>>          SPEC_CTRL_ENTRY_FROM_HVM    /* Req: b=curr %rsp=regs/cpuinfo, Clob: acd */
>>>>>>          /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
>>>>>>  
>>>>>> -        STGI
>>>>>> -GLOBAL(svm_stgi_label)
>>>>> Unfortunately, the stgi label must remain here. 
>>>>> SPEC_CTRL_ENTRY_FROM_HVM depends on NMIs being blocked to avoid
>>>>> corrupting guest state.
>>>> I'm afraid I don't follow: How are things safe then without NMIs blocked
>>>> for VMX?
>>> 27.5.5 VMExits > Updating Non-Register State
>>>
>>> VM exits caused directly by non-maskable interrupts (NMIs) cause
>>> blocking by NMI (see Table 24-3). Other VM exits do not affect blocking
>>> by NMI.
>> Telling us that an NMI can occur at any point between
>> vmx_asm_vmexit_handler and the point where guest state was
>> saved in SPEC_CTRL_ENTRY_FROM_HVM. So afaict if the macro is
>> indeed unsafe right now, it's the macro that needs to change,
>> not the patch we're discussing here.
> No... It tells you the exact opposite.
>
> NMIs are not delivered while the NMI shadow is in effect, and is why we
> deliberately execute an IRET in the NMI path in vmx_vmexit_handler()

Actually never mind.  I'm being very dumb...  Let me experiment

~Andrew

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