Re: [Xen-devel] [PATCH 4/6] x86/AMD: Introduce and use X86_BUG_NULL_SEG

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.12.18 at 13:39, <andrew.cooper3@xxxxxxxxxx> wrote:
> AMD processors don't clear the base or limit fields when loading a NULL
> segment, and Hygon processors inherit this behaviour.
> 
> Express the logic in terms of cpu_bug_null_seg,

If this behavior was considered a bug, AMD surely would have fixed
it by now. It's a quirk at best, but personally I would call it just an
implementation detail. Hence cpu_bug_ as a prefix is simply
inappropriate.

> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1307,16 +1307,16 @@ arch_do_vcpu_op(
>  }
>  
>  /*
> - * Loading a nul selector does not clear bases and limits on AMD CPUs. Be on
> - * the safe side and re-initialize both to flat segment values before loading
> - * a nul selector.
> + * Loading a NULL selector doesn't always clear bases and limits.  Be on the
> + * safe side and re-initialize both to flat segment values before loading a
> + * NULL selector.

To be honest I dislike the abuse of NULL in cases like this one: In
C code NULL would better stand for just the one thing the language
assigns to it as a meaning. Hence "nul" was better as a term (or if
you dislike its spelling, "null" would still be better).

>   */
> -#define preload_segment(seg, value) do {              \
> -    if ( !((value) & ~3) &&                           \
> -         boot_cpu_data.x86_vendor == X86_VENDOR_AMD ) \
> -        asm volatile ( "movl %k0, %%" #seg            \
> -                       :: "r" (FLAT_USER_DS32) );     \
> -} while ( false )
> +#define preload_segment(seg, value)                     \
> +    do {                                                \
> +        if ( cpu_bug_null_seg && !((value) & ~3) )      \
> +            asm volatile ( "mov %k0, %%" #seg           \
> +                           :: "rm" (FLAT_USER_DS32) );  \

As you say in the description, "mov %sreg" allows for a memory
operand, so from an abstract perspective the change is fine. It
won't have any practical effect though (and hence be potentially
misleading), as "m" can't be combined with a literal number as
value (you may want to try out removing the r from the
constraint) - for that to work the compiler would have to allocate
an unnamed stack variable, which it doesn't do (instead you get
"memory input <N> is not directly addressable").

Jan



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