Re: [Xen-devel] [PATCH] x86/HVM: adjust hvm_interrupt_blocked()
On Mon, Sep 04, 2023 at 03:40:46PM +0200, Jan Beulich wrote: > On 31.08.2023 12:57, Roger Pau Monné wrote: > > On Thu, Aug 31, 2023 at 12:42:58PM +0200, Roger Pau Monné wrote: > >> On Fri, Oct 12, 2018 at 09:58:46AM -0600, Jan Beulich wrote: > >>> First of all, hvm_intsrc_mce was not considered here at all, yet nothing > >>> blocks #MC (other than an already in-progress #MC, but dealing with this > >>> is not the purpose of this patch). > >>> > >>> Additionally STI-shadow only blocks maskable interrupts, but not NMI. > >> > >> I've found the Table 25-3 on Intel SDM vol3 quite helpful: > >> > >> "Execution of STI with RFLAGS.IF = 0 blocks maskable interrupts on the > >> instruction boundary following its execution.1 Setting this bit > >> indicates that this blocking is in effect." > >> > >> And: > >> > >> "Execution of a MOV to SS or a POP to SS blocks or suppresses certain > >> debug exceptions as well as interrupts (maskable and nonmaskable) on > >> the instruction boundary following its execution." > >> > >> Might be worth adding to the commit message IMO. > > > > So I've found a further footnote that contains: > > > > "Nonmaskable interrupts and system-management interrupts may also be > > inhibited on the instruction boundary following such an execution of > > STI." > > > > So we want to take the more restrictive implementation of STI-shadow, > > and block #NMI there also. > > Hmm, that text says "may", not will, and imo STI affecting NMI can at best > be viewed as a quirk (quite possibly intentional, for simplifying some > internal logic on the processor). Possibly, but a guest expecting such behavior and Xen not emulating it would most likely lead to a crash, while forcing the other way around (Xen blocking NMIs on STI shadow unconditionally) is not likely to cause issues for OSes not relying on it. > Plus I'm not convinced AMD allows similar > leeway in SVM; at least I can't spot any similar statement in their PM. Hard to tell, in any case I would apply the same reasoning as above, as IMO implementing STI shadow blocking NMIs is the safer option, and is what Xen has been doing so far without reported issues that I know of. Thanks, Roger.
Re: [Xen-devel] [PATCH] x86/HVM: adjust hvm_interrupt_blocked()
On 31.08.2023 12:42, Roger Pau Monné wrote: > On Fri, Oct 12, 2018 at 09:58:46AM -0600, Jan Beulich wrote: >> First of all, hvm_intsrc_mce was not considered here at all, yet nothing >> blocks #MC (other than an already in-progress #MC, but dealing with this >> is not the purpose of this patch). >> >> Additionally STI-shadow only blocks maskable interrupts, but not NMI. > > I've found the Table 25-3 on Intel SDM vol3 quite helpful: > > "Execution of STI with RFLAGS.IF = 0 blocks maskable interrupts on the > instruction boundary following its execution.1 Setting this bit > indicates that this blocking is in effect." > > And: > > "Execution of a MOV to SS or a POP to SS blocks or suppresses certain > debug exceptions as well as interrupts (maskable and nonmaskable) on > the instruction boundary following its execution." > > Might be worth adding to the commit message IMO. Hmm, to be honest I'm not sure why reproducing some of one vendor's doc would be useful here. >> --- a/xen/arch/x86/hvm/hvm.c >> +++ b/xen/arch/x86/hvm/hvm.c >> @@ -3771,19 +3771,24 @@ enum hvm_intblk hvm_interrupt_blocked(st >> return intr; >> } >> >> -if ( (intack.source != hvm_intsrc_nmi) && >> - !(guest_cpu_user_regs()->eflags & X86_EFLAGS_IF) ) >> -return hvm_intblk_rflags_ie; >> +if ( intack.source == hvm_intsrc_mce ) >> +return hvm_intblk_none; > > I've been wondering, why do we handle #MC here, instead of doing the > same as for other Traps/Exceptions and use hvm_inject_hw_exception() > directly? I'm afraid I can only guess here, but I suspect a connection to how vMCE "works" (and the point in time when v->arch.mce_pending would be set). >> intr_shadow = hvm_funcs.get_interrupt_shadow(v); >> >> -if ( intr_shadow & (HVM_INTR_SHADOW_STI|HVM_INTR_SHADOW_MOV_SS) ) >> +if ( intr_shadow & HVM_INTR_SHADOW_MOV_SS ) >> return hvm_intblk_shadow; >> >> if ( intack.source == hvm_intsrc_nmi ) >> return ((intr_shadow & HVM_INTR_SHADOW_NMI) ? >> hvm_intblk_nmi_iret : hvm_intblk_none); >> >> +if ( intr_shadow & HVM_INTR_SHADOW_STI ) >> +return hvm_intblk_shadow; >> + >> +if ( !(guest_cpu_user_regs()->eflags & X86_EFLAGS_IF) ) >> +return hvm_intblk_rflags_ie; > > I do wonder whether this code would be clearer using a `switch ( > intack.source )` construct, but that's out of the scope. If it would help, I could switch to using switch(), but the order of checks isn't really a sequence of comparisons against intack.source. Jan
Re: [Xen-devel] [PATCH] x86/HVM: adjust hvm_interrupt_blocked()
On 31.08.2023 12:57, Roger Pau Monné wrote: > On Thu, Aug 31, 2023 at 12:42:58PM +0200, Roger Pau Monné wrote: >> On Fri, Oct 12, 2018 at 09:58:46AM -0600, Jan Beulich wrote: >>> First of all, hvm_intsrc_mce was not considered here at all, yet nothing >>> blocks #MC (other than an already in-progress #MC, but dealing with this >>> is not the purpose of this patch). >>> >>> Additionally STI-shadow only blocks maskable interrupts, but not NMI. >> >> I've found the Table 25-3 on Intel SDM vol3 quite helpful: >> >> "Execution of STI with RFLAGS.IF = 0 blocks maskable interrupts on the >> instruction boundary following its execution.1 Setting this bit >> indicates that this blocking is in effect." >> >> And: >> >> "Execution of a MOV to SS or a POP to SS blocks or suppresses certain >> debug exceptions as well as interrupts (maskable and nonmaskable) on >> the instruction boundary following its execution." >> >> Might be worth adding to the commit message IMO. > > So I've found a further footnote that contains: > > "Nonmaskable interrupts and system-management interrupts may also be > inhibited on the instruction boundary following such an execution of > STI." > > So we want to take the more restrictive implementation of STI-shadow, > and block #NMI there also. Hmm, that text says "may", not will, and imo STI affecting NMI can at best be viewed as a quirk (quite possibly intentional, for simplifying some internal logic on the processor). Plus I'm not convinced AMD allows similar leeway in SVM; at least I can't spot any similar statement in their PM. Jan
Re: [Xen-devel] [PATCH] x86/HVM: adjust hvm_interrupt_blocked()
On Thu, Aug 31, 2023 at 12:42:58PM +0200, Roger Pau Monné wrote: > On Fri, Oct 12, 2018 at 09:58:46AM -0600, Jan Beulich wrote: > > First of all, hvm_intsrc_mce was not considered here at all, yet nothing > > blocks #MC (other than an already in-progress #MC, but dealing with this > > is not the purpose of this patch). > > > > Additionally STI-shadow only blocks maskable interrupts, but not NMI. > > I've found the Table 25-3 on Intel SDM vol3 quite helpful: > > "Execution of STI with RFLAGS.IF = 0 blocks maskable interrupts on the > instruction boundary following its execution.1 Setting this bit > indicates that this blocking is in effect." > > And: > > "Execution of a MOV to SS or a POP to SS blocks or suppresses certain > debug exceptions as well as interrupts (maskable and nonmaskable) on > the instruction boundary following its execution." > > Might be worth adding to the commit message IMO. So I've found a further footnote that contains: "Nonmaskable interrupts and system-management interrupts may also be inhibited on the instruction boundary following such an execution of STI." So we want to take the more restrictive implementation of STI-shadow, and block #NMI there also. Thanks, Roger.
Re: [Xen-devel] [PATCH] x86/HVM: adjust hvm_interrupt_blocked()
On Fri, Oct 12, 2018 at 09:58:46AM -0600, Jan Beulich wrote: > First of all, hvm_intsrc_mce was not considered here at all, yet nothing > blocks #MC (other than an already in-progress #MC, but dealing with this > is not the purpose of this patch). > > Additionally STI-shadow only blocks maskable interrupts, but not NMI. I've found the Table 25-3 on Intel SDM vol3 quite helpful: "Execution of STI with RFLAGS.IF = 0 blocks maskable interrupts on the instruction boundary following its execution.1 Setting this bit indicates that this blocking is in effect." And: "Execution of a MOV to SS or a POP to SS blocks or suppresses certain debug exceptions as well as interrupts (maskable and nonmaskable) on the instruction boundary following its execution." Might be worth adding to the commit message IMO. > Signed-off-by: Jan Beulich > > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -3771,19 +3771,24 @@ enum hvm_intblk hvm_interrupt_blocked(st > return intr; > } > > -if ( (intack.source != hvm_intsrc_nmi) && > - !(guest_cpu_user_regs()->eflags & X86_EFLAGS_IF) ) > -return hvm_intblk_rflags_ie; > +if ( intack.source == hvm_intsrc_mce ) > +return hvm_intblk_none; I've been wondering, why do we handle #MC here, instead of doing the same as for other Traps/Exceptions and use hvm_inject_hw_exception() directly? > > intr_shadow = hvm_funcs.get_interrupt_shadow(v); > > -if ( intr_shadow & (HVM_INTR_SHADOW_STI|HVM_INTR_SHADOW_MOV_SS) ) > +if ( intr_shadow & HVM_INTR_SHADOW_MOV_SS ) > return hvm_intblk_shadow; > > if ( intack.source == hvm_intsrc_nmi ) > return ((intr_shadow & HVM_INTR_SHADOW_NMI) ? > hvm_intblk_nmi_iret : hvm_intblk_none); > > +if ( intr_shadow & HVM_INTR_SHADOW_STI ) > +return hvm_intblk_shadow; > + > +if ( !(guest_cpu_user_regs()->eflags & X86_EFLAGS_IF) ) > +return hvm_intblk_rflags_ie; I do wonder whether this code would be clearer using a `switch ( intack.source )` construct, but that's out of the scope. Thanks, Roger.
Re: [Xen-devel] [PATCH] x86/HVM: adjust hvm_interrupt_blocked()
>>> On 12.10.18 at 18:37, wrote: > Furthermore, I believe even #MC is blocked by the MOVSS shadow, because > the purpose of the shadow is to indicate "my stack is not safe to take > an exception". I've just looked at the precise SDM text again, which I see has changed compared to the prior revision. It now reads "Execution of a MOV to SS or a POP to SS blocks or suppresses certain debug exceptions as well as interrupts (maskable and nonmaskable) on the instruction boundary following its execution. Setting this bit indicates that this blocking is in effect.2 This document uses the term “blocking by MOV SS,” but it applies equally to POP SS." No mention of exceptions at all. >> Additionally STI-shadow only blocks maskable interrupts, but not NMI. > > This has been discussed on LKML in the past, but `STI; HLT` will > deadlock if NMIs don't respect the STI shadow. > > An NMI which hits that instruction boundary will IRET with IF clear, at > which point the core will halt and never wake up. > > I believe the input from the vendor architects was that some very old > cores suffer from this problem, but anything you can get yours hand on > today will respect the STI shadow. The SDM has a footnote actually saying "Nonmaskable interrupts and system-management interrupts may also be inhibited on the instruction boundary following such an execution of STI." Note the word "may". Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] x86/HVM: adjust hvm_interrupt_blocked()
>>> On 12.10.18 at 18:37, wrote: > Furthermore, I believe even #MC is blocked by the MOVSS shadow, because > the purpose of the shadow is to indicate "my stack is not safe to take > an exception". Having thought about this some more over lunch, I'm afraid I now think that both variants are equally likely due to being equally risky (as to resulting in shutdown): If the #MC was in any way fetch/execution related, honoring mov-ss-shadow would mean a second #MC (on the following instruction) would be deadly. >> Additionally STI-shadow only blocks maskable interrupts, but not NMI. > > This has been discussed on LKML in the past, but `STI; HLT` will > deadlock if NMIs don't respect the STI shadow. > > An NMI which hits that instruction boundary will IRET with IF clear, at > which point the core will halt and never wake up. No. STI-shadow, aiui, does not delay the setting of EFLAGS.IF, but only the recognition of interrupts. Hence in your scenario the NMI handler would see IF set in the saved image on the stack. As a result I don't currently think the change is in contradiction to documented or expectable behavior, and hence for now I don't see reasons to adjust it. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] x86/HVM: adjust hvm_interrupt_blocked()
>>> On 12.10.18 at 18:37, wrote: > On 12/10/18 16:58, Jan Beulich wrote: >> First of all, hvm_intsrc_mce was not considered here at all, yet nothing >> blocks #MC (other than an already in-progress #MC, but dealing with this >> is not the purpose of this patch). > > I don't believe we've got sufficient infrastructure to fix this > reasonably yet, but for the record, the real behaviour for MCEs is: > > If intel > broadcast to every thread covered by the MCE bank > else if AMD > sent to the thread with the lowest id covered by the MCE bank > > When trying to inject: > > if !CR4.MCE or MCG_STATUS.MCIP > shutdown How is this related to the function the patch changes? > Furthermore, I believe even #MC is blocked by the MOVSS shadow, because > the purpose of the shadow is to indicate "my stack is not safe to take > an exception". "I believe" is what I would have said too, but the documentation has not even a hint to that effect, and (to me) instead suggests that this is not the case. I'm sort of assuming that they imply a full stack switch (32-bit: task switch; 64-bit: IST) to be arranged for by the OS. But of course the documentation being explicit in this regard would certainly help decide what the ordering of actions/tests in the function here really ought to be. >> Additionally STI-shadow only blocks maskable interrupts, but not NMI. > > This has been discussed on LKML in the past, but `STI; HLT` will > deadlock if NMIs don't respect the STI shadow. > > An NMI which hits that instruction boundary will IRET with IF clear, at > which point the core will halt and never wake up. True, yet again ... > I believe the input from the vendor architects was that some very old > cores suffer from this problem, but anything you can get yours hand on > today will respect the STI shadow. ... I haven't been able to find anything like this in the doc; instead iirc I did, just like for #MC, again find hints towards the behavior that the patch implements. I don't really know what to do here, when the doc is as unclear as it looks to be (or a possibly more clear statement is in an unexpected place, and hence not reasonable to be found). Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] x86/HVM: adjust hvm_interrupt_blocked()
On 12/10/18 16:58, Jan Beulich wrote: > First of all, hvm_intsrc_mce was not considered here at all, yet nothing > blocks #MC (other than an already in-progress #MC, but dealing with this > is not the purpose of this patch). I don't believe we've got sufficient infrastructure to fix this reasonably yet, but for the record, the real behaviour for MCEs is: If intel broadcast to every thread covered by the MCE bank else if AMD sent to the thread with the lowest id covered by the MCE bank When trying to inject: if !CR4.MCE or MCG_STATUS.MCIP shutdown Furthermore, I believe even #MC is blocked by the MOVSS shadow, because the purpose of the shadow is to indicate "my stack is not safe to take an exception". > Additionally STI-shadow only blocks maskable interrupts, but not NMI. This has been discussed on LKML in the past, but `STI; HLT` will deadlock if NMIs don't respect the STI shadow. An NMI which hits that instruction boundary will IRET with IF clear, at which point the core will halt and never wake up. I believe the input from the vendor architects was that some very old cores suffer from this problem, but anything you can get yours hand on today will respect the STI shadow. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
