[Xen-devel] [PATCH] SVM: limit GIF=0 region
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 Signed-off-by: Jan Beulich --- 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
[Xen-devel] [PATCH] SVM: limit GIF=0 region
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 Signed-off-by: Jan Beulich --- 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: [Xen-devel] [PATCH] SVM: limit GIF=0 region
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 > Signed-off-by: Jan Beulich Reviewed-by: Boris Ostrovsky ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] SVM: limit GIF=0 region
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 > Signed-off-by: Jan Beulich > > --- 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: [Xen-devel] [PATCH] SVM: limit GIF=0 region
>>> On 26.06.18 at 10:45, 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: [Xen-devel] [PATCH] SVM: limit GIF=0 region
On 26/06/18 10:52, Jan Beulich wrote: On 26.06.18 at 10:45, 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: [Xen-devel] [PATCH] SVM: limit GIF=0 region
>>> On 26.06.18 at 12:52, wrote: > On 26/06/18 10:52, Jan Beulich wrote: > On 26.06.18 at 10:45, 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: [Xen-devel] [PATCH] SVM: limit GIF=0 region
On 26/06/18 12:50, Jan Beulich wrote: On 26.06.18 at 12:52, wrote: >> On 26/06/18 10:52, Jan Beulich wrote: >> On 26.06.18 at 10:45, 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: [Xen-devel] [PATCH] SVM: limit GIF=0 region
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, wrote: >>> On 26/06/18 10:52, Jan Beulich wrote: >>> On 26.06.18 at 10:45, 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
