[PATCH] x86/HVM: adjust hvm_interrupt_blocked()

[PATCH] x86/HVM: adjust hvm_interrupt_blocked()

[Jan Beulich]

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.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- 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;
 
     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;
+
     if ( intack.source == hvm_intsrc_lapic )
     {
         uint32_t tpr = vlapic_get_reg(vcpu_vlapic(v), APIC_TASKPRI) & 0xF0;





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

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

[Andrew Cooper]

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

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

[Jan Beulich]

>>> On 12.10.18 at 18:37, <andrew.cooper3@citrix.com> 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: [PATCH] x86/HVM: adjust hvm_interrupt_blocked()

[Jan Beulich]

>>> On 12.10.18 at 18:37, <andrew.cooper3@citrix.com> 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: [PATCH] x86/HVM: adjust hvm_interrupt_blocked()

[Jan Beulich]

>>> On 12.10.18 at 18:37, <andrew.cooper3@citrix.com> 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()

[Roger Pau Monné]

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 <jbeulich@suse.com>
> 
> --- 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()

[Roger Pau Monné]

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()

[Jan Beulich]

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()

[Jan Beulich]

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()

[Roger Pau Monné]

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.