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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

> +{
> +    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.

> +        return;
> +
> +    if ( !mode_ring0() )
> +        bndcfg = read_bndcfgu();
> +    else if ( !ops->read_msr ||
> +              ops->read_msr(MSR_BNDCFGS, &bndcfg, ctxt) != X86EMUL_OKAY )
> +        return;
> +    if ( (bndcfg & BNDCFG_ENABLE) && !(bndcfg & BNDCFG_PRESERVE) )
> +    {
> +        /*
> +         * Using BNDMK or any other MPX instruction here is pointless, as
> +         * we run with MPX disabled ourselves, and hence they're all no-ops.
> +         * Therefore we have two ways to clear BNDn: Enable MPX temporarily
> +         * (in which case executing any suitable non-prefixed branch
> +         * instruction would do), or use XRSTOR.
> +         */
> +        xstate_set_init(XSTATE_BNDREGS);
> +    }
> + done:;
> +}
> +
>  int x86emul_unhandleable_rw(
>      enum x86_segment seg,
>      unsigned long offset,
> --- 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.

> +
> +        bndcsr = (void *)xstate + xstate_offsets[_XSTATE_BNDCSR];
> +    }
> +
> +    if ( cr0 & X86_CR0_TS )
> +        write_cr0(cr0);
> +
> +    return xstate->xsave_hdr.xstate_bv & XSTATE_BNDCSR ? bndcsr->bndcfgu : 0;
> +}
> +
> +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.

> +
> +    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) ?

~Andrew

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