Re: [Xen-devel] [PATCH 1/3] X86: MPX support for HVM guest

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.11.13 at 09:38, "Liu, Jinsong" <jinsong.liu@xxxxxxxxx> wrote:
> From be65afbcf37ca7349ab657f552859ec95a872268 Mon Sep 17 00:00:00 2001
> From: Liu Jinsong <jinsong.liu@xxxxxxxxx>
> Date: Fri, 8 Nov 2013 00:49:41 +0800
> Subject: [PATCH 1/3] X86: MPX support for HVM guest
> 
> Signed-off-by: Xudong Hao <xudong.hao@xxxxxxxxx>
> Acked-by: Liu Jinsong <jinsong.liu@xxxxxxxxx>

First of all - please consider using the (to be slightly extended)
patch attached instead of the hypervisor parts of this one. (You
likely also recall that it's generally easier for the maintainers if
you submit hypervisor and tools changes in separate patches.)

> --- a/xen/arch/x86/xstate.c
> +++ b/xen/arch/x86/xstate.c
> @@ -357,6 +357,9 @@ int handle_xsetbv(u32 index, u64 new_bv)
>      if ( (new_bv & XSTATE_YMM) && !(new_bv & XSTATE_SSE) )
>          return -EINVAL;
>  
> +    if ( (!!(XSTATE_BNDREGS & new_bv)) != (!!(XSTATE_BNDCSR & new_bv)) )
> +        return -EINVAL;

A single ! each would fully suffice. And a similar check - see
attached patch - is needed in validate_xstate() too.

> @@ -377,6 +380,9 @@ int handle_xsetbv(u32 index, u64 new_bv)
>              write_cr0(cr0);
>      }
>  
> +    if ( xsave_enabled(curr) && cpu_has_mpx )
> +        curr->arch.nonlazy_xstate_used = !!(cpu_has_mpx);

So you set the flag no matter whether the guest enables the
feature? That's surely too eager. (And using !!cpu_has_mpx in
the body of a conditional having checked that cpu_has_mpx is
non-zero is dubious too.)

But yes, something along those lines is what my patch is
currently lacking (apart from the MSR related aspects mentioned
at its top). The main question is whether it is really necessary for
the flag to be sticky.

Jan

Attachment: x86-xsave-more.patch
Description: Text document

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