Re: [Xen-devel] [PATCH v2] x86/PV: fix/generalize guest nul selector handling

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 29/09/17 16:17, Roger Pau Monné wrote:
> On Thu, Sep 28, 2017 at 08:41:28AM +0000, Jan Beulich wrote:
>> Segment bases (and limits) aren't being cleared by the loading of a nul
>> selector into a segment register on AMD CPUs. Therefore, if an
>> outgoing vCPU has a non-zero base in FS or GS and the subsequent
>> incoming vCPU has a non-zero but nul selector in the respective
>> register(s), the selector value(s) would be loaded without clearing the
>> segment base(s) in the hidden register portion.
>>
>> Since the ABI states "zero" in its description of the fs and gs fields,
>> it is worth noting that the chosen approach to fix this alters the
>> written down ABI. I consider this preferrable over enforcing the
>> previously written down behavior, as nul selectors are far more likely
>> to be what was meant from the beginning.
>>
>> The adjustments also eliminate an inconsistency between FS and GS
>> handling: Old code had an extra pointless (gs_base_user was always zero
>> when DIRTY_GS was set) conditional for GS. The old bitkeeper changeset
>> has no explanation for this asymmetry.
>>
>> Inspired by Linux commit e137a4d8f4dd2e277e355495b6b2cb241a8693c3.
>>
>> Additionally for DS and ES a flat selector is being loaded prior to the
>> loading of a nul one on AMD CPUs, just as a precautionary measure
>> (we're not currently aware of ways for a guest to deduce the base of a
>> segment register which has a nul selector loaded).
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> ---
>> v2: Add DS/ES handling.
>>
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -1237,6 +1237,18 @@ arch_do_vcpu_op(
>>      return rc;
>>  }
>>  
>> +/*
>> + * 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.
>> + */
>> +#define preload_segment(seg, value) do {              \
>> +    if ( !((value) & ~3) &&                           \
>> +         boot_cpu_data.x86_vendor == X86_VENDOR_AMD ) \
>> +        asm volatile ( "movl %k0, %%" #seg            \
> Shouldn't this be a movw? Segment selectors are 16b, not 32b, but I
> might be missing something here.
>
> I see loadsegment is also using movl, so yes, I guess I'm missing
> something.

It is perfectly legal to encode a mov of a large GPR and a 16 bit
segment selector, and the upper bits are ignored/zeroed (depending on
the direction of the mov).

Furthermore, the 32bit form is shorter to encoded as it doesn't require
an operand size override prefix (which, being length-changing, also
causes a small pipeline decode stall on all modern processors).

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel