Ping: [PATCH v2] x86: refine guest_mode()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.05.2020 18:23, Jan Beulich wrote:
> The 2nd of the assertions as well as the macro's return value have been
> assuming we're on the primary stack. While for most IST exceptions we
> switch back to the main one when user mode was interrupted, for #DF we
> intentionally never do, and hence a #DF actually triggering on a user
> mode insn (which then is still a Xen bug) would in turn trigger this
> assertion, rather than cleanly logging state.
> 
> Reported-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

In a prior discussion, as so often, my most recent reply remained
unanswered. Quoting it partially (fully reply at [1]):

"> I'd certainly prefer to go for something which is more robust, even if
 > it is a larger change.
 
 ... what's your suggestion? Basing on _just_ CS.RPL obviously won't
 work. Not even if we put in place the guest's CS (albeit that
 somewhat depends on the meaning we assign to the macro's returned
 value). Using current inside the macro to determine whether the
 guest is HVM would also seem fragile to me - there are quite a few
 uses of guest_mode(). Which would leave passing in a const struct
 vcpu * (or domain *), requiring to touch all call sites, including
 Arm's.

 Compared to this it would seem to me that the change as presented
 is a clear improvement without becoming overly large of a change."

Lacking a clear route to take to address your request for further
change, based on the last sentence I intend to finally commit this
one (in its re-based form) as well once the tree is fully open
again. Of course, as always, unless I hear otherwise by then
(including a clear path forward). I definitely don't want to
celebrate the 1st anniversary of this patch's submission without it
having got committed.

Jan

[1] https://lists.xen.org/archives/html/xen-devel/2020-04/msg01550.html

> ---
> v2: Correct description.
> ---
> While we could go further and also assert we're on the correct IST
> stack in an "else" ti the "if()" added, I'm not fully convinced this
> would be generally helpful. I'll be happy to adjust accordingly if
> others think differently; at such a point though I think this should
> then no longer be a macro.
> 
> --- unstable.orig/xen/include/asm-x86/regs.h  2020-01-22 20:03:18.000000000 
> +0100
> +++ unstable/xen/include/asm-x86/regs.h       2020-04-27 10:02:40.009916762 
> +0200
> @@ -10,9 +10,10 @@
>      /* Frame pointer must point into current CPU stack. */                   
>  \
>      ASSERT(diff < STACK_SIZE);                                               
>  \
>      /* If not a guest frame, it must be a hypervisor frame. */               
>  \
> -    ASSERT((diff == 0) || (r->cs == __HYPERVISOR_CS));                       
>  \
> +    if ( diff < PRIMARY_STACK_SIZE )                                         
>  \
> +        ASSERT(!diff || ((r)->cs == __HYPERVISOR_CS));                       
>  \
>      /* Return TRUE if it's a guest frame. */                                 
>  \
> -    (diff == 0);                                                             
>  \
> +    !diff || ((r)->cs != __HYPERVISOR_CS);                                   
>  \
>  })
>  
>  #endif /* __X86_REGS_H__ */
>