[PATCH] x86/xsave: Expose xsave_cntxt_size to avoid reloading xcr0

[PATCH] x86/xsave: Expose xsave_cntxt_size to avoid reloading xcr0

[Andrew Cooper]

xstate_ctxt_size(xfeature_mask) is runtime constant after boot.  There is no
need to reload xcr0 twice for this basic bounds check.  Also annotate
xfeature_mask as __read_mostly as it is only ever written once.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/domctl.c        |    3 +--
 xen/arch/x86/xstate.c        |    4 ++--
 xen/include/asm-x86/xstate.h |    1 +
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 8f5b287..75b0522 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1145,8 +1145,7 @@ long arch_do_domctl(
 
             ret = -EINVAL;
             if ( evc->size < 2 * sizeof(uint64_t) ||
-                 evc->size > 2 * sizeof(uint64_t) +
-                             xstate_ctxt_size(xfeature_mask) )
+                 evc->size > 2 * sizeof(uint64_t) + xsave_cntxt_size )
                 goto vcpuextstate_out;
 
             receive_buf = xmalloc_bytes(evc->size);
diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index e202344..86ea244 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -21,10 +21,10 @@ bool_t __read_mostly cpu_has_xsaveopt;
  * the supported and enabled features on the processor, including the
  * XSAVE.HEADER. We only enable XCNTXT_MASK that we have known.
  */
-static u32 __read_mostly xsave_cntxt_size;
+u32 __read_mostly xsave_cntxt_size;
 
 /* A 64-bit bitmask of the XSAVE/XRSTOR features supported by processor. */
-u64 xfeature_mask;
+u64 __read_mostly xfeature_mask;
 
 /* Cached xcr0 for fast read */
 static DEFINE_PER_CPU(uint64_t, xcr0);
diff --git a/xen/include/asm-x86/xstate.h b/xen/include/asm-x86/xstate.h
index 8d21349..e0abeef 100644
--- a/xen/include/asm-x86/xstate.h
+++ b/xen/include/asm-x86/xstate.h
@@ -39,6 +39,7 @@
 #define XSTATE_NONLAZY (XSTATE_LWP | XSTATE_BNDREGS | XSTATE_BNDCSR)
 #define XSTATE_LAZY    (XSTATE_ALL & ~XSTATE_NONLAZY)
 
+extern u32 xsave_cntxt_size;
 extern u64 xfeature_mask;
 
 /* extended state save area */
-- 
1.7.10.4

Re: [PATCH] x86/xsave: Expose xsave_cntxt_size to avoid reloading xcr0

[Jan Beulich]

>>> On 04.06.14 at 13:56, <andrew.cooper3@citrix.com> wrote:
> xstate_ctxt_size(xfeature_mask) is runtime constant after boot.  There is no
> need to reload xcr0 twice for this basic bounds check.

Honestly I'm not in favor of exposing this variable for a not
performance critical code path. If we really care about saving a few
cycles here, how about having xstate_ctxt_size() check its argument
against xfeature_mask and return xsave_cntxt_size if equal?
Admittedly that'll need some extra care for the uses of the function
in xstate_init(), but it's not horribly difficult to make the current
function an internal one, and the global one be a wrapper with that
check done first.

Jan

Re: [PATCH] x86/xsave: Expose xsave_cntxt_size to avoid reloading xcr0

[Andrew Cooper]

On 04/06/14 13:32, Jan Beulich wrote:
>>>> On 04.06.14 at 13:56, <andrew.cooper3@citrix.com> wrote:
>> xstate_ctxt_size(xfeature_mask) is runtime constant after boot.  There is no
>> need to reload xcr0 twice for this basic bounds check.
> Honestly I'm not in favor of exposing this variable for a not
> performance critical code path. If we really care about saving a few
> cycles here, how about having xstate_ctxt_size() check its argument
> against xfeature_mask and return xsave_cntxt_size if equal?
> Admittedly that'll need some extra care for the uses of the function
> in xstate_init(), but it's not horribly difficult to make the current
> function an internal one, and the global one be a wrapper with that
> check done first.
>
> Jan
>

Personally, I think that is less pleasant than exposing
xsave_cntxt_size, but I will see about doing it.

It turns out I also need this value for other correctness fixes
elsewhere in xsave domctl code.

~Andrew

Re: [PATCH] x86/xsave: Expose xsave_cntxt_size to avoid reloading xcr0

[Jan Beulich]

>>> On 04.06.14 at 14:39, <andrew.cooper3@citrix.com> wrote:
> On 04/06/14 13:32, Jan Beulich wrote:
>>>>> On 04.06.14 at 13:56, <andrew.cooper3@citrix.com> wrote:
>>> xstate_ctxt_size(xfeature_mask) is runtime constant after boot.  There is no
>>> need to reload xcr0 twice for this basic bounds check.
>> Honestly I'm not in favor of exposing this variable for a not
>> performance critical code path. If we really care about saving a few
>> cycles here, how about having xstate_ctxt_size() check its argument
>> against xfeature_mask and return xsave_cntxt_size if equal?
>> Admittedly that'll need some extra care for the uses of the function
>> in xstate_init(), but it's not horribly difficult to make the current
>> function an internal one, and the global one be a wrapper with that
>> check done first.
> 
> Personally, I think that is less pleasant than exposing
> xsave_cntxt_size, but I will see about doing it.

My main concern is that the variable in no way says that this is
the size when all supported features are enabled. The function
call, otoh, nicely documents that.

Jan