Re: [Xen-devel] [PATCH 2/3 v2] x86emul: conditionally clear BNDn for branches

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.01.17 at 22:11, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 12/12/16 10:00, Jan Beulich wrote:
>> @@ -1791,6 +1795,34 @@ static int inject_swint(enum x86_swint_t
>>      generate_exception(fault_type, error_code);
>>  }
>>  
>> +static void clear_bnd(struct x86_emulate_ctxt *ctxt,
>> +                      const struct x86_emulate_ops *ops, enum vex_pfx pfx)
> 
> As with register_address_adjust(), this would be better as adjust_bnd()
> as we don't necessarily clear them.

Okay.

>> +{
>> +    uint64_t bndcfg;
>> +    int rc;
>> +
>> +    if ( pfx == vex_f2 || !vcpu_has_mpx() )
> 
> This is an awkward edgecase of the rules concerning the host_ variants,
> but we will take a fault from xsave/xrstor for using XSTATE_BND* if the
> host doesn't support MPX.

Right. For now the implication is that guest available MPX implies
host support.

>> --- a/xen/arch/x86/xstate.c
>> +++ b/xen/arch/x86/xstate.c
>> @@ -723,6 +741,66 @@ int handle_xsetbv(u32 index, u64 new_bv)
>>      return 0;
>>  }
>>  
>> +uint64_t read_bndcfgu(void)
>> +{
>> +    unsigned long cr0 = read_cr0();
>> +    struct xsave_struct *xstate
>> +        = idle_vcpu[smp_processor_id()]->arch.xsave_area;
>> +    const struct xstate_bndcsr *bndcsr;
>> +
>> +    ASSERT(cpu_has_mpx);
>> +    clts();
>> +
>> +    if ( cpu_has_xsavec )
>> +    {
>> +        asm ( ".byte 0x0f,0xc7,0x27\n" /* xsavec */
>> +              : "=m" (*xstate)
>> +              : "a" (XSTATE_BNDCSR), "d" (0), "D" (xstate) );
>> +
>> +        bndcsr = (void *)(xstate + 1);
>> +    }
>> +    else
>> +    {
>> +        alternative_io(".byte 0x0f,0xae,0x27\n", /* xsave */
>> +                       ".byte 0x0f,0xae,0x37\n", /* xsaveopt */
>> +                       X86_FEATURE_XSAVEOPT,
>> +                       "=m" (*xstate),
>> +                       "a" (XSTATE_BNDCSR), "d" (0), "D" (xstate));
> 
> I am not certain this is safe.  xsaveopt has an extra optimisation to do
> with whether the state has been internally modified.  Because we use an
> xsave area from the idle vcpu, I can't see anything which prevents the
> LAXA (linear address of XSAVE area) optimisation kicking in, causing us
> to observe a zero in xstate_bv despite BNDCSR having a non-zero value
> loaded in hardware.

True - I've dropped the alternative now as well as the use of XSAVEC.

>> +void xstate_set_init(uint64_t mask)
>> +{
>> +    unsigned long cr0 = read_cr0();
>> +    unsigned long xcr0 = this_cpu(xcr0);
>> +    struct vcpu *v = idle_vcpu[smp_processor_id()];
>> +    struct xsave_struct *xstate = v->arch.xsave_area;
>> +
>> +    if ( ~xfeature_mask & mask )
>> +        return;
> 
> As the function is void, this should be an ASSERT or BUG.  IMO, it is a
> caller error to ask for a feature to be reset for hardware which doesn't
> support the requested state.

ASSERT() then - no reason to bring down a production system
here.

>> +
>> +    if ( (~xcr0 & mask) && !set_xcr0(xcr0 | mask) )
>> +        return;
>> +
>> +    clts();
>> +
>> +    memset(&xstate->xsave_hdr, 0, sizeof(xstate->xsave_hdr));
>> +    xrstor(v, mask);
>> +
>> +    if ( cr0 & X86_CR0_TS )
>> +        write_cr0(cr0);
>> +
>> +    if ( ~xcr0 & mask )
>> +        xsetbv(XCR_XFEATURE_ENABLED_MASK, xcr0);
> 
> Shouldn't this be set_xcr0() again, to undo the possible clobbering of
> this_cpu(xcr0) ?

Of course - I think early on it was xsetbv() in both places, and I've
later (wrongly) changed only one.

Jan


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