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
