Re: [Xen-devel] [V6 1/4] x86/xsaves: add basic definitions/helpers to support xsaves

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.10.15 at 08:07, <shuai.ruan@xxxxxxxxxxxxxxx> wrote:
> --- a/xen/arch/x86/xstate.c
> +++ b/xen/arch/x86/xstate.c
> @@ -23,6 +23,11 @@ static u32 __read_mostly xsave_cntxt_size;
>  
>  /* A 64-bit bitmask of the XSAVE/XRSTOR features supported by processor. */
>  u64 __read_mostly xfeature_mask;
> +unsigned int * __read_mostly xstate_offsets;
> +unsigned int * __read_mostly xstate_sizes;

You appear to need the latter outside of this file in patch 3, but the
former could (and hence should) be static afaict.

> --- a/xen/include/asm-x86/xstate.h
> +++ b/xen/include/asm-x86/xstate.h
> @@ -19,7 +19,11 @@
>  
>  #define XCR_XFEATURE_ENABLED_MASK 0x00000000  /* index of XCR0 */
>  
> +#define XSAVE_HDR_SIZE            64
> +#define XSAVE_SSE_OFFSET          160
>  #define XSTATE_YMM_SIZE           256
> +#define FXSAVE_SIZE               512
> +#define XSAVE_HDR_OFFSET          FXSAVE_SIZE
>  #define XSTATE_AREA_MIN_SIZE      (512 + 64)  /* FP/SSE + XSAVE.HEADER */

This 512 should now get replaced too.

> @@ -38,8 +42,24 @@
>  #define XSTATE_ALL     (~(1ULL << 63))
>  #define XSTATE_NONLAZY (XSTATE_LWP | XSTATE_BNDREGS | XSTATE_BNDCSR)
>  #define XSTATE_LAZY    (XSTATE_ALL & ~XSTATE_NONLAZY)
> +#define XSTATE_COMPACTION_ENABLED  (1ULL << 63)
> +
> +#define XSTATE_FIXUP ".section .fixup,\"ax\"      \n"        \
> +                     "2: mov %5,%%ecx             \n"        \
> +                     "   xor %1,%1                \n"        \
> +                     "   rep stosb                \n"        \
> +                     "   lea %2,%0                \n"        \
> +                     "   mov %3,%1                \n"        \
> +                     "   jmp 1b                   \n"        \
> +                     ".previous                   \n"        \
> +                     _ASM_EXTABLE(1b, 2b)                    \
> +                     : "+&D" (ptr), "+&a" (lmask)            \
> +                     : "m" (*ptr), "g" (lmask), "d" (hmask), \
> +                       "m" (xsave_cntxt_size)                \
> +                     : "ecx"

First of all I don't think this belongs here - neither in this patch, nor
in this header. It's not being used by this patch, and the connection
between variable names used here and in patch 2 is completely lost.
If you put this into xrstor() as a local #define, then this may be
okay. In any other case the user of the macro has to have a way
to control the variable names used.

Jan


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