Re: [Xen-devel] [PATCH] x86/HVM: adjust hvm_interrupt_blocked()

[Jan Beulich <jbeulich@xxxxxxxx>]

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