[PATCH v3 0/4] Introduce a fpu_initilised field to HVM CPU context

[PATCH v3 0/4] Introduce a fpu_initilised field to HVM CPU context

[Roger Pau Monne]

Hello,

This patch series tries to properly solve the problem seen with the HVMlite 
series, that Xen always assumes the FPU is initialised on CPU context 
restore.

Roger.

[PATCH v3 1/4] xen/save: pass a size parameter to the HVM compat functions

[Roger Pau Monne]

In order to cope with types having multiple compat versions pass a size
parameter to the fixup function so we can identify which compat version
Xen is dealing with.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Tim Deegan <tim@xen.org>
---
Changes since v2:
 - Size is uint32_t not int.
 - Pass the actual size of the record and not the size of the whole stream.
---
 xen/include/public/arch-x86/hvm/save.h |  2 +-
 xen/include/public/hvm/save.h          | 10 ++++++----
 xen/include/xen/hvm/save.h             |  4 +++-
 3 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/xen/include/public/arch-x86/hvm/save.h b/xen/include/public/arch-x86/hvm/save.h
index efb0b62..29d513c 100644
--- a/xen/include/public/arch-x86/hvm/save.h
+++ b/xen/include/public/arch-x86/hvm/save.h
@@ -268,7 +268,7 @@ struct hvm_hw_cpu_compat {
     uint32_t error_code;
 };
 
-static inline int _hvm_hw_fix_cpu(void *h) {
+static inline int _hvm_hw_fix_cpu(void *h, uint32_t size) {
 
     union hvm_hw_cpu_union {
         struct hvm_hw_cpu nat;
diff --git a/xen/include/public/hvm/save.h b/xen/include/public/hvm/save.h
index cc8b5fd..0bd240d 100644
--- a/xen/include/public/hvm/save.h
+++ b/xen/include/public/hvm/save.h
@@ -63,13 +63,15 @@ struct hvm_save_descriptor {
 
 #ifdef __XEN__
 # define DECLARE_HVM_SAVE_TYPE_COMPAT(_x, _code, _type, _ctype, _fix)     \
-    static inline int __HVM_SAVE_FIX_COMPAT_##_x(void *h) { return _fix(h); } \
-    struct __HVM_SAVE_TYPE_##_x { _type t; char c[_code]; char cpt[2];}; \
+    static inline int __HVM_SAVE_FIX_COMPAT_##_x(void *h, uint32_t size)  \
+        { return _fix(h, size); }                                         \
+    struct __HVM_SAVE_TYPE_##_x { _type t; char c[_code]; char cpt[2];};  \
     struct __HVM_SAVE_TYPE_COMPAT_##_x { _ctype t; }                   
 
 # include <xen/lib.h> /* BUG() */
 # define DECLARE_HVM_SAVE_TYPE(_x, _code, _type)                         \
-    static inline int __HVM_SAVE_FIX_COMPAT_##_x(void *h) { BUG(); return -1; } \
+    static inline int __HVM_SAVE_FIX_COMPAT_##_x(void *h, uint32_t size) \
+        { BUG(); return -1; }                                            \
     struct __HVM_SAVE_TYPE_##_x { _type t; char c[_code]; char cpt[1];}; \
     struct __HVM_SAVE_TYPE_COMPAT_##_x { _type t; }                   
 #else
@@ -89,7 +91,7 @@ struct hvm_save_descriptor {
 # define HVM_SAVE_LENGTH_COMPAT(_x) (sizeof (HVM_SAVE_TYPE_COMPAT(_x)))
 
 # define HVM_SAVE_HAS_COMPAT(_x) (sizeof (((struct __HVM_SAVE_TYPE_##_x *)(0))->cpt)-1)
-# define HVM_SAVE_FIX_COMPAT(_x, _dst) __HVM_SAVE_FIX_COMPAT_##_x(_dst)
+# define HVM_SAVE_FIX_COMPAT(_x, _dst, _size) __HVM_SAVE_FIX_COMPAT_##_x(_dst, _size)
 #endif
 
 /* 
diff --git a/xen/include/xen/hvm/save.h b/xen/include/xen/hvm/save.h
index aa27a50..a97694e 100644
--- a/xen/include/xen/hvm/save.h
+++ b/xen/include/xen/hvm/save.h
@@ -60,6 +60,8 @@ void _hvm_read_entry(struct hvm_domain_context *h,
  */
 #define _hvm_load_entry(_x, _h, _dst, _strict) ({                       \
     int r;                                                              \
+    struct hvm_save_descriptor *d                                       \
+        = (struct hvm_save_descriptor *)&(_h)->data[(_h)->cur];         \
     if ( (r = _hvm_check_entry((_h), HVM_SAVE_CODE(_x),                 \
                HVM_SAVE_LENGTH(_x), (_strict))) == 0 )                  \
         _hvm_read_entry((_h), (_dst), HVM_SAVE_LENGTH(_x));             \
@@ -67,7 +69,7 @@ void _hvm_read_entry(struct hvm_domain_context *h,
              && (r = _hvm_check_entry((_h), HVM_SAVE_CODE(_x),          \
                        HVM_SAVE_LENGTH_COMPAT(_x), (_strict))) == 0 ) { \
         _hvm_read_entry((_h), (_dst), HVM_SAVE_LENGTH_COMPAT(_x));      \
-        r=HVM_SAVE_FIX_COMPAT(_x, (_dst));                              \
+        r = HVM_SAVE_FIX_COMPAT(_x, (_dst), d->length);                 \
     }                                                                   \
     r; })
 
-- 
1.9.5 (Apple Git-50.3)


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

[PATCH v3 2/4] xen/save: allow the usage of zeroextend and a fixup function

[Roger Pau Monne]

With the current compat implementation in the save/restore context handling,
only one compat structure is allowed, and using _zeroextend prevents the
fixup function from being called.

In order to allow for the compat handling layer to be able to handle
different compat versions allow calling the fixup function with
hvm_load_entry_zeroextend.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Tim Deegan <tim@xen.org>
---
 xen/include/xen/hvm/save.h | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/xen/include/xen/hvm/save.h b/xen/include/xen/hvm/save.h
index a97694e..22bcdb9 100644
--- a/xen/include/xen/hvm/save.h
+++ b/xen/include/xen/hvm/save.h
@@ -58,19 +58,22 @@ void _hvm_read_entry(struct hvm_domain_context *h,
  * Unmarshalling: check, then copy. Evaluates to zero on success. This load
  * function requires the save entry to be the same size as the dest structure.
  */
-#define _hvm_load_entry(_x, _h, _dst, _strict) ({                       \
-    int r;                                                              \
-    struct hvm_save_descriptor *d                                       \
-        = (struct hvm_save_descriptor *)&(_h)->data[(_h)->cur];         \
-    if ( (r = _hvm_check_entry((_h), HVM_SAVE_CODE(_x),                 \
-               HVM_SAVE_LENGTH(_x), (_strict))) == 0 )                  \
-        _hvm_read_entry((_h), (_dst), HVM_SAVE_LENGTH(_x));             \
-    else if (HVM_SAVE_HAS_COMPAT(_x)                                    \
-             && (r = _hvm_check_entry((_h), HVM_SAVE_CODE(_x),          \
-                       HVM_SAVE_LENGTH_COMPAT(_x), (_strict))) == 0 ) { \
-        _hvm_read_entry((_h), (_dst), HVM_SAVE_LENGTH_COMPAT(_x));      \
-        r = HVM_SAVE_FIX_COMPAT(_x, (_dst), d->length);                 \
-    }                                                                   \
+#define _hvm_load_entry(_x, _h, _dst, _strict) ({                           \
+    int r;                                                                  \
+    struct hvm_save_descriptor *d                                           \
+        = (struct hvm_save_descriptor *)&(_h)->data[(_h)->cur];             \
+    if ( (r = _hvm_check_entry((_h), HVM_SAVE_CODE(_x),                     \
+               HVM_SAVE_LENGTH(_x), (_strict))) == 0 ) {                    \
+        _hvm_read_entry((_h), (_dst), HVM_SAVE_LENGTH(_x));                 \
+        if ( HVM_SAVE_HAS_COMPAT(_x) && d->length != HVM_SAVE_LENGTH(_x) )  \
+            r = HVM_SAVE_FIX_COMPAT(_x, (_dst), d->length);                 \
+    }                                                                       \
+    else if (HVM_SAVE_HAS_COMPAT(_x)                                        \
+             && (r = _hvm_check_entry((_h), HVM_SAVE_CODE(_x),              \
+                       HVM_SAVE_LENGTH_COMPAT(_x), (_strict))) == 0 ) {     \
+        _hvm_read_entry((_h), (_dst), HVM_SAVE_LENGTH_COMPAT(_x));          \
+        r = HVM_SAVE_FIX_COMPAT(_x, (_dst), d->length);                     \
+    }                                                                       \
     r; })
 
 #define hvm_load_entry(_x, _h, _dst)            \
-- 
1.9.5 (Apple Git-50.3)


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

[PATCH v3 3/4] xen/hvm: introduce a fpu_uninitialised field to the CPU save record

[Roger Pau Monne]

Introduce a new field to signal if the FPU has been initialised or not. Xen
needs this new field in order to know whether to set the FPU as initialised
or not during restore of CPU context. Previously Xen always wrongly assumed
the FPU was initialised on restore.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
Changes since v1:
 - Don't add yet another compat structure, new fields should always be added
   to the end of the existing structure and offsetof should be used to
   compare sizes.
 - Leave the previous compat structure as-is, since the field was not added
   to the end we cannot remove it and use offsetof in this case.
 - Set xstate_bv based on fpu_initialised value instead of unconditionally
   setting it to XSTATE_FP_SSE.
---
 xen/arch/x86/hvm/hvm.c                 |  8 +++++---
 xen/include/public/arch-x86/hvm/save.h | 26 +++++++++++++++++++-------
 2 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index ea982e2..72a4e4f 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1800,6 +1800,7 @@ static int hvm_save_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
             memcpy(ctxt.fpu_regs, v->arch.fpu_ctxt, sizeof(ctxt.fpu_regs));
         else 
             memset(ctxt.fpu_regs, 0, sizeof(ctxt.fpu_regs));
+        ctxt.fpu_initialised = v->fpu_initialised;
 
         ctxt.rax = v->arch.user_regs.eax;
         ctxt.rbx = v->arch.user_regs.ebx;
@@ -1979,7 +1980,7 @@ static int hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
         return -EINVAL;
     }
 
-    if ( hvm_load_entry(CPU, h, &ctxt) != 0 ) 
+    if ( hvm_load_entry_zeroextend(CPU, h, &ctxt) != 0 )
         return -EINVAL;
 
     /* Sanity check some control registers. */
@@ -2091,7 +2092,8 @@ static int hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
         struct xsave_struct *xsave_area = v->arch.xsave_area;
 
         memcpy(v->arch.xsave_area, ctxt.fpu_regs, sizeof(ctxt.fpu_regs));
-        xsave_area->xsave_hdr.xstate_bv = XSTATE_FP_SSE;
+        xsave_area->xsave_hdr.xstate_bv = ctxt.fpu_initialised ?
+                                                    XSTATE_FP_SSE : 0;
     }
     else
         memcpy(v->arch.fpu_ctxt, ctxt.fpu_regs, sizeof(ctxt.fpu_regs));
@@ -2122,7 +2124,7 @@ static int hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
     v->arch.debugreg[7] = ctxt.dr7;
 
     v->arch.vgc_flags = VGCF_online;
-    v->fpu_initialised = 1;
+    v->fpu_initialised = !!ctxt.fpu_initialised;
 
     /* Auxiliary processors should be woken immediately. */
     v->is_initialised = 1;
diff --git a/xen/include/public/arch-x86/hvm/save.h b/xen/include/public/arch-x86/hvm/save.h
index 29d513c..0f7ef5a 100644
--- a/xen/include/public/arch-x86/hvm/save.h
+++ b/xen/include/public/arch-x86/hvm/save.h
@@ -47,7 +47,9 @@ DECLARE_HVM_SAVE_TYPE(HEADER, 1, struct hvm_save_header);
 /*
  * Processor
  *
- * Compat: Pre-3.4 didn't have msr_tsc_aux
+ * Compat:
+ *     - Pre-3.4 didn't have msr_tsc_aux
+ *     - Pre-4.7 didn't have fpu_initialised
  */
 
 struct hvm_hw_cpu {
@@ -157,6 +159,8 @@ struct hvm_hw_cpu {
     };
     /* error code for pending event */
     uint32_t error_code;
+    /* is fpu initialised? */
+    uint32_t fpu_initialised;
 };
 
 struct hvm_hw_cpu_compat {
@@ -266,6 +270,7 @@ struct hvm_hw_cpu_compat {
     };
     /* error code for pending event */
     uint32_t error_code;
+    /*uint32_t fpu_initialised; COMPAT */
 };
 
 static inline int _hvm_hw_fix_cpu(void *h, uint32_t size) {
@@ -275,12 +280,19 @@ static inline int _hvm_hw_fix_cpu(void *h, uint32_t size) {
         struct hvm_hw_cpu_compat cmp;
     } *ucpu = (union hvm_hw_cpu_union *)h;
 
-    /* If we copy from the end backwards, we should
-     * be able to do the modification in-place */
-    ucpu->nat.error_code = ucpu->cmp.error_code;
-    ucpu->nat.pending_event = ucpu->cmp.pending_event;
-    ucpu->nat.tsc = ucpu->cmp.tsc;
-    ucpu->nat.msr_tsc_aux = 0;
+    if ( size == sizeof(struct hvm_hw_cpu_compat) )
+    {
+        /*
+         * If we copy from the end backwards, we should
+         * be able to do the modification in-place.
+         */
+        ucpu->nat.error_code = ucpu->cmp.error_code;
+        ucpu->nat.pending_event = ucpu->cmp.pending_event;
+        ucpu->nat.tsc = ucpu->cmp.tsc;
+        ucpu->nat.msr_tsc_aux = 0;
+    }
+    /* Mimic the old behaviour by unconditionally setting fpu_initialised. */
+    ucpu->nat.fpu_initialised = 1;
 
     return 0;
 }
-- 
1.9.5 (Apple Git-50.3)


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

[PATCH v3 4/4] Revert "libxc: create an initial FPU state for HVM guests"

[Roger Pau Monne]

This reverts commit d64dbbcc7c9934a46126c59d78536235908377ad:

Xen always set the FPU as initialized when loading a HVM context, so libxc
has to provide a valid FPU context when setting the CPU registers.

This was a stop-gap measure in order to unblock OSSTest Windows 7 failures
while a proper fix for the HVM CPU save/restore is being worked on.

This can now be reverted because a proper fix is in place and we can signal
in the save record whether the FPU is initialized or not.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxc/xc_dom_x86.c | 38 --------------------------------------
 1 file changed, 38 deletions(-)

diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
index 7279fa2..b5e155d 100644
--- a/tools/libxc/xc_dom_x86.c
+++ b/tools/libxc/xc_dom_x86.c
@@ -910,27 +910,6 @@ static int vcpu_hvm(struct xc_dom_image *dom)
         struct hvm_save_descriptor end_d;
         HVM_SAVE_TYPE(END) end;
     } bsp_ctx;
-    /*
-     * The layout of the fpu context structure is the same for
-     * both 32 and 64 bits.
-     */
-    struct {
-        uint16_t fcw;
-        uint16_t fsw;
-        uint8_t ftw;
-        uint8_t rsvd1;
-        uint16_t fop;
-        union {
-            uint64_t addr;
-            struct {
-                uint32_t offs;
-                uint16_t sel;
-                uint16_t rsvd;
-            };
-        } fip, fdp;
-        uint32_t mxcsr;
-        uint32_t mxcsr_mask;
-    } *fpu_ctxt;
     uint8_t *full_ctx = NULL;
     int rc;
 
@@ -998,23 +977,6 @@ static int vcpu_hvm(struct xc_dom_image *dom)
     /* Set the control registers. */
     bsp_ctx.cpu.cr0 = X86_CR0_PE | X86_CR0_ET;
 
-    /*
-     * XXX: Set initial FPU state.
-     *
-     * This should be removed once Xen is able to know if the
-     * FPU state saved is valid or not, now Xen always sets
-     * fpu_initialised to true regardless of the FPU state.
-     *
-     * The code below mimics the FPU sate after executing
-     * fninit
-     * ldmxcsr 0x1f80
-     */
-    fpu_ctxt = (typeof(fpu_ctxt))bsp_ctx.cpu.fpu_regs;
-
-    fpu_ctxt->fcw = 0x37f;
-    fpu_ctxt->ftw = 0xff;
-    fpu_ctxt->mxcsr = 0x1f80;
-
     /* Set the IP. */
     bsp_ctx.cpu.rip = dom->parms.phys_entry;
 
-- 
1.9.5 (Apple Git-50.3)


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

Re: [PATCH v3 1/4] xen/save: pass a size parameter to the HVM compat functions

[Andrew Cooper]

On 18/11/15 16:37, Roger Pau Monne wrote:
> In order to cope with types having multiple compat versions pass a size
> parameter to the fixup function so we can identify which compat version
> Xen is dealing with.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Tim Deegan <tim@xen.org>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH v3 2/4] xen/save: allow the usage of zeroextend and a fixup function

[Andrew Cooper]

On 18/11/15 16:37, Roger Pau Monne wrote:
> With the current compat implementation in the save/restore context handling,
> only one compat structure is allowed, and using _zeroextend prevents the
> fixup function from being called.
>
> In order to allow for the compat handling layer to be able to handle
> different compat versions allow calling the fixup function with
> hvm_load_entry_zeroextend.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Tim Deegan <tim@xen.org>

Eww - how nasty.

I believe the only actual change here is:

+        if ( HVM_SAVE_HAS_COMPAT(_x) && d->length !=
HVM_SAVE_LENGTH(_x) )  \
+            r = HVM_SAVE_FIX_COMPAT(_x, (_dst),
d->length);                 \

which looks to be appropriate.

If so, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
>  xen/include/xen/hvm/save.h | 29 ++++++++++++++++-------------
>  1 file changed, 16 insertions(+), 13 deletions(-)
>
> diff --git a/xen/include/xen/hvm/save.h b/xen/include/xen/hvm/save.h
> index a97694e..22bcdb9 100644
> --- a/xen/include/xen/hvm/save.h
> +++ b/xen/include/xen/hvm/save.h
> @@ -58,19 +58,22 @@ void _hvm_read_entry(struct hvm_domain_context *h,
>   * Unmarshalling: check, then copy. Evaluates to zero on success. This load
>   * function requires the save entry to be the same size as the dest structure.
>   */
> -#define _hvm_load_entry(_x, _h, _dst, _strict) ({                       \
> -    int r;                                                              \
> -    struct hvm_save_descriptor *d                                       \
> -        = (struct hvm_save_descriptor *)&(_h)->data[(_h)->cur];         \
> -    if ( (r = _hvm_check_entry((_h), HVM_SAVE_CODE(_x),                 \
> -               HVM_SAVE_LENGTH(_x), (_strict))) == 0 )                  \
> -        _hvm_read_entry((_h), (_dst), HVM_SAVE_LENGTH(_x));             \
> -    else if (HVM_SAVE_HAS_COMPAT(_x)                                    \
> -             && (r = _hvm_check_entry((_h), HVM_SAVE_CODE(_x),          \
> -                       HVM_SAVE_LENGTH_COMPAT(_x), (_strict))) == 0 ) { \
> -        _hvm_read_entry((_h), (_dst), HVM_SAVE_LENGTH_COMPAT(_x));      \
> -        r = HVM_SAVE_FIX_COMPAT(_x, (_dst), d->length);                 \
> -    }                                                                   \
> +#define _hvm_load_entry(_x, _h, _dst, _strict) ({                           \
> +    int r;                                                                  \
> +    struct hvm_save_descriptor *d                                           \
> +        = (struct hvm_save_descriptor *)&(_h)->data[(_h)->cur];             \
> +    if ( (r = _hvm_check_entry((_h), HVM_SAVE_CODE(_x),                     \
> +               HVM_SAVE_LENGTH(_x), (_strict))) == 0 ) {                    \
> +        _hvm_read_entry((_h), (_dst), HVM_SAVE_LENGTH(_x));                 \
> +        if ( HVM_SAVE_HAS_COMPAT(_x) && d->length != HVM_SAVE_LENGTH(_x) )  \
> +            r = HVM_SAVE_FIX_COMPAT(_x, (_dst), d->length);                 \
> +    }                                                                       \
> +    else if (HVM_SAVE_HAS_COMPAT(_x)                                        \
> +             && (r = _hvm_check_entry((_h), HVM_SAVE_CODE(_x),              \
> +                       HVM_SAVE_LENGTH_COMPAT(_x), (_strict))) == 0 ) {     \
> +        _hvm_read_entry((_h), (_dst), HVM_SAVE_LENGTH_COMPAT(_x));          \
> +        r = HVM_SAVE_FIX_COMPAT(_x, (_dst), d->length);                     \
> +    }                                                                       \
>      r; })
>  
>  #define hvm_load_entry(_x, _h, _dst)            \


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

Re: [PATCH v3 3/4] xen/hvm: introduce a fpu_uninitialised field to the CPU save record

[Andrew Cooper]

On 18/11/15 16:37, Roger Pau Monne wrote:
> Introduce a new field to signal if the FPU has been initialised or not. Xen
> needs this new field in order to know whether to set the FPU as initialised
> or not during restore of CPU context. Previously Xen always wrongly assumed
> the FPU was initialised on restore.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH v3 1/4] xen/save: pass a size parameter to the HVM compat functions

[Jan Beulich]

>>> On 18.11.15 at 17:37, <roger.pau@citrix.com> wrote:
> --- a/xen/include/xen/hvm/save.h
> +++ b/xen/include/xen/hvm/save.h
> @@ -60,6 +60,8 @@ void _hvm_read_entry(struct hvm_domain_context *h,
>   */
>  #define _hvm_load_entry(_x, _h, _dst, _strict) ({                       \
>      int r;                                                              \
> +    struct hvm_save_descriptor *d                                       \
> +        = (struct hvm_save_descriptor *)&(_h)->data[(_h)->cur];         \

This would seem to belong in the else if() body below. Also I
don't think "d" is a suitable name for a variable inside a macro
(albeit the same would apply to "r" too).

Jan

Re: [PATCH v3 2/4] xen/save: allow the usage of zeroextend and a fixup function

[Jan Beulich]

>>> On 18.11.15 at 17:37, <roger.pau@citrix.com> wrote:
> +        if ( HVM_SAVE_HAS_COMPAT(_x) && d->length != HVM_SAVE_LENGTH(_x) )  \

If you broke this line at the && you could avoid all the churn to move
out the \ by a few spaces. That would also have avoided Andrew's
request for re-assurance that the addition here was the only actual
change.

Jan

Re: [PATCH v3 3/4] xen/hvm: introduce a fpu_uninitialised field to the CPU save record

[Jan Beulich]

>>> On 18.11.15 at 17:37, <roger.pau@citrix.com> wrote:
> @@ -2091,7 +2092,8 @@ static int hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
>          struct xsave_struct *xsave_area = v->arch.xsave_area;
>  
>          memcpy(v->arch.xsave_area, ctxt.fpu_regs, sizeof(ctxt.fpu_regs));
> -        xsave_area->xsave_hdr.xstate_bv = XSTATE_FP_SSE;
> +        xsave_area->xsave_hdr.xstate_bv = ctxt.fpu_initialised ?
> +                                                    XSTATE_FP_SSE : 0;
>      }
>      else
>          memcpy(v->arch.fpu_ctxt, ctxt.fpu_regs, sizeof(ctxt.fpu_regs));

Question is - are the memcpy()s here really meaningful/valid
when !ctxt.fpu_initialized? I.e. shouldn't this code rather be
skipped instead of getting modified?

> @@ -157,6 +159,8 @@ struct hvm_hw_cpu {
>      };
>      /* error code for pending event */
>      uint32_t error_code;
> +    /* is fpu initialised? */
> +    uint32_t fpu_initialised;

A whole uint32_t for just one bit? Didn't we talk about making this
new field a flags one, consuming just one bit from it?

> @@ -266,6 +270,7 @@ struct hvm_hw_cpu_compat {
>      };
>      /* error code for pending event */
>      uint32_t error_code;
> +    /*uint32_t fpu_initialised; COMPAT */
>  };

I think this is misleading - the compat structure never has this field.

Jan

Re: [PATCH v3 1/4] xen/save: pass a size parameter to the HVM compat functions

[Roger Pau Monné]

El 20/11/15 a les 16.37, Jan Beulich ha escrit:
>>>> On 18.11.15 at 17:37, <roger.pau@citrix.com> wrote:
>> --- a/xen/include/xen/hvm/save.h
>> +++ b/xen/include/xen/hvm/save.h
>> @@ -60,6 +60,8 @@ void _hvm_read_entry(struct hvm_domain_context *h,
>>   */
>>  #define _hvm_load_entry(_x, _h, _dst, _strict) ({                       \
>>      int r;                                                              \
>> +    struct hvm_save_descriptor *d                                       \
>> +        = (struct hvm_save_descriptor *)&(_h)->data[(_h)->cur];         \
> 
> This would seem to belong in the else if() body below. Also I
> don't think "d" is a suitable name for a variable inside a macro
> (albeit the same would apply to "r" too).

Patch #2 makes use of "d" in the first "if" branch also. I don't think
it makes much sense to put it inside of the "else if" branch if I have
to move it in the following patch.

Regarding the name, I don't have a strong opinion, desc is more
descriptive, but it's also longer which means I might have to break some
lines inside of the macro even more.

Roger.

Re: [PATCH v3 1/4] xen/save: pass a size parameter to the HVM compat functions

[Jan Beulich]

>>> On 24.11.15 at 13:54, <roger.pau@citrix.com> wrote:
> El 20/11/15 a les 16.37, Jan Beulich ha escrit:
>>>>> On 18.11.15 at 17:37, <roger.pau@citrix.com> wrote:
>>> --- a/xen/include/xen/hvm/save.h
>>> +++ b/xen/include/xen/hvm/save.h
>>> @@ -60,6 +60,8 @@ void _hvm_read_entry(struct hvm_domain_context *h,
>>>   */
>>>  #define _hvm_load_entry(_x, _h, _dst, _strict) ({                       \
>>>      int r;                                                              \
>>> +    struct hvm_save_descriptor *d                                       \
>>> +        = (struct hvm_save_descriptor *)&(_h)->data[(_h)->cur];         \
>> 
>> This would seem to belong in the else if() body below. Also I
>> don't think "d" is a suitable name for a variable inside a macro
>> (albeit the same would apply to "r" too).
> 
> Patch #2 makes use of "d" in the first "if" branch also. I don't think
> it makes much sense to put it inside of the "else if" branch if I have
> to move it in the following patch.

Yes, I saw that, and while you could move in patch 2 I agree it
doesn't make much sense.

> Regarding the name, I don't have a strong opinion, desc is more
> descriptive, but it's also longer which means I might have to break some
> lines inside of the macro even more.

At least d_ perhaps then?

Jan

Re: [PATCH v3 3/4] xen/hvm: introduce a fpu_uninitialised field to the CPU save record

[Roger Pau Monné]

El 20/11/15 a les 16.49, Jan Beulich ha escrit:
>>>> On 18.11.15 at 17:37, <roger.pau@citrix.com> wrote:
>> @@ -2091,7 +2092,8 @@ static int hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
>>          struct xsave_struct *xsave_area = v->arch.xsave_area;
>>  
>>          memcpy(v->arch.xsave_area, ctxt.fpu_regs, sizeof(ctxt.fpu_regs));
>> -        xsave_area->xsave_hdr.xstate_bv = XSTATE_FP_SSE;
>> +        xsave_area->xsave_hdr.xstate_bv = ctxt.fpu_initialised ?
>> +                                                    XSTATE_FP_SSE : 0;
>>      }
>>      else
>>          memcpy(v->arch.fpu_ctxt, ctxt.fpu_regs, sizeof(ctxt.fpu_regs));
> 
> Question is - are the memcpy()s here really meaningful/valid
> when !ctxt.fpu_initialized? I.e. shouldn't this code rather be
> skipped instead of getting modified?

If !fpu_initialized the fpu context save record is all zeroed out. I
don't think it matters much (apart from saving a few CPU cycles). I can
send a new version that doesn't save/restore the fpu context at all if
!fpu_initialised.

>> @@ -157,6 +159,8 @@ struct hvm_hw_cpu {
>>      };
>>      /* error code for pending event */
>>      uint32_t error_code;
>> +    /* is fpu initialised? */
>> +    uint32_t fpu_initialised;
> 
> A whole uint32_t for just one bit? Didn't we talk about making this
> new field a flags one, consuming just one bit from it?

AFAIK we agreed on adding this field to the tail and making it a
uint32_t so that when new fields are added they can be detected by
looking at the size of the structure:

http://marc.info/?l=xen-devel&m=144490321208291

>> @@ -266,6 +270,7 @@ struct hvm_hw_cpu_compat {
>>      };
>>      /* error code for pending event */
>>      uint32_t error_code;
>> +    /*uint32_t fpu_initialised; COMPAT */
>>  };
> 
> I think this is misleading - the compat structure never has this field.

Right, I will remove it.

Roger.

Re: [PATCH v3 1/4] xen/save: pass a size parameter to the HVM compat functions

[Roger Pau Monné]

El 24/11/15 a les 14.06, Jan Beulich ha escrit:
>>>> On 24.11.15 at 13:54, <roger.pau@citrix.com> wrote:
>> El 20/11/15 a les 16.37, Jan Beulich ha escrit:
>>>>>> On 18.11.15 at 17:37, <roger.pau@citrix.com> wrote:
>>>> --- a/xen/include/xen/hvm/save.h
>>>> +++ b/xen/include/xen/hvm/save.h
>>>> @@ -60,6 +60,8 @@ void _hvm_read_entry(struct hvm_domain_context *h,
>>>>   */
>>>>  #define _hvm_load_entry(_x, _h, _dst, _strict) ({                       \
>>>>      int r;                                                              \
>>>> +    struct hvm_save_descriptor *d                                       \
>>>> +        = (struct hvm_save_descriptor *)&(_h)->data[(_h)->cur];         \
>>>
>>> This would seem to belong in the else if() body below. Also I
>>> don't think "d" is a suitable name for a variable inside a macro
>>> (albeit the same would apply to "r" too).
>>
>> Patch #2 makes use of "d" in the first "if" branch also. I don't think
>> it makes much sense to put it inside of the "else if" branch if I have
>> to move it in the following patch.
> 
> Yes, I saw that, and while you could move in patch 2 I agree it
> doesn't make much sense.
> 
>> Regarding the name, I don't have a strong opinion, desc is more
>> descriptive, but it's also longer which means I might have to break some
>> lines inside of the macro even more.
> 
> At least d_ perhaps then?

I've called it "desc", I hope it's fine.

Roger.

Re: [PATCH v3 3/4] xen/hvm: introduce a fpu_uninitialised field to the CPU save record

[Jan Beulich]

>>> On 24.11.15 at 14:10, <roger.pau@citrix.com> wrote:
> El 20/11/15 a les 16.49, Jan Beulich ha escrit:
>>>>> On 18.11.15 at 17:37, <roger.pau@citrix.com> wrote:
>>> @@ -2091,7 +2092,8 @@ static int hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
>>>          struct xsave_struct *xsave_area = v->arch.xsave_area;
>>>  
>>>          memcpy(v->arch.xsave_area, ctxt.fpu_regs, sizeof(ctxt.fpu_regs));
>>> -        xsave_area->xsave_hdr.xstate_bv = XSTATE_FP_SSE;
>>> +        xsave_area->xsave_hdr.xstate_bv = ctxt.fpu_initialised ?
>>> +                                                    XSTATE_FP_SSE : 0;
>>>      }
>>>      else
>>>          memcpy(v->arch.fpu_ctxt, ctxt.fpu_regs, sizeof(ctxt.fpu_regs));
>> 
>> Question is - are the memcpy()s here really meaningful/valid
>> when !ctxt.fpu_initialized? I.e. shouldn't this code rather be
>> skipped instead of getting modified?
> 
> If !fpu_initialized the fpu context save record is all zeroed out. I
> don't think it matters much (apart from saving a few CPU cycles). I can
> send a new version that doesn't save/restore the fpu context at all if
> !fpu_initialised.

I'd appreciate that (ideally with if(!fpu_initialised) memset(); else if ...).

>>> @@ -157,6 +159,8 @@ struct hvm_hw_cpu {
>>>      };
>>>      /* error code for pending event */
>>>      uint32_t error_code;
>>> +    /* is fpu initialised? */
>>> +    uint32_t fpu_initialised;
>> 
>> A whole uint32_t for just one bit? Didn't we talk about making this
>> new field a flags one, consuming just one bit from it?
> 
> AFAIK we agreed on adding this field to the tail and making it a
> uint32_t so that when new fields are added they can be detected by
> looking at the size of the structure:
> 
> http://marc.info/?l=xen-devel&m=144490321208291 

Admittedly it's a little implicit, but that mail has, in its quoting parts,

"... (and validate unused tail bits are zero, so they can be used for
something later on)"

going back to that intention of using just a single bit here afaict.

Jan

Re: [PATCH v3 3/4] xen/hvm: introduce a fpu_uninitialised field to the CPU save record

[Roger Pau Monné]

El 24/11/15 a les 14.34, Jan Beulich ha escrit:
>>>> On 24.11.15 at 14:10, <roger.pau@citrix.com> wrote:
>> El 20/11/15 a les 16.49, Jan Beulich ha escrit:
>>>>>> On 18.11.15 at 17:37, <roger.pau@citrix.com> wrote:
>>>> @@ -2091,7 +2092,8 @@ static int hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
>>>>          struct xsave_struct *xsave_area = v->arch.xsave_area;
>>>>  
>>>>          memcpy(v->arch.xsave_area, ctxt.fpu_regs, sizeof(ctxt.fpu_regs));
>>>> -        xsave_area->xsave_hdr.xstate_bv = XSTATE_FP_SSE;
>>>> +        xsave_area->xsave_hdr.xstate_bv = ctxt.fpu_initialised ?
>>>> +                                                    XSTATE_FP_SSE : 0;
>>>>      }
>>>>      else
>>>>          memcpy(v->arch.fpu_ctxt, ctxt.fpu_regs, sizeof(ctxt.fpu_regs));
>>>
>>> Question is - are the memcpy()s here really meaningful/valid
>>> when !ctxt.fpu_initialized? I.e. shouldn't this code rather be
>>> skipped instead of getting modified?
>>
>> If !fpu_initialized the fpu context save record is all zeroed out. I
>> don't think it matters much (apart from saving a few CPU cycles). I can
>> send a new version that doesn't save/restore the fpu context at all if
>> !fpu_initialised.
> 
> I'd appreciate that (ideally with if(!fpu_initialised) memset(); else if ...).
> 
>>>> @@ -157,6 +159,8 @@ struct hvm_hw_cpu {
>>>>      };
>>>>      /* error code for pending event */
>>>>      uint32_t error_code;
>>>> +    /* is fpu initialised? */
>>>> +    uint32_t fpu_initialised;
>>>
>>> A whole uint32_t for just one bit? Didn't we talk about making this
>>> new field a flags one, consuming just one bit from it?
>>
>> AFAIK we agreed on adding this field to the tail and making it a
>> uint32_t so that when new fields are added they can be detected by
>> looking at the size of the structure:
>>
>> http://marc.info/?l=xen-devel&m=144490321208291 
> 
> Admittedly it's a little implicit, but that mail has, in its quoting parts,
> 
> "... (and validate unused tail bits are zero, so they can be used for
> something later on)"
> 
> going back to that intention of using just a single bit here afaict.

Ack. I have to admit I've misunderstood that part. Then I guess the
field should also have a more generic name, like "flags", and
fpu_initialised should be defined as (1U << 0). Or do you want me to use
the MSB in order to store the fpu_initialised bit?

Roger.

Re: [PATCH v3 3/4] xen/hvm: introduce a fpu_uninitialised field to the CPU save record

[Jan Beulich]

>>> On 24.11.15 at 15:38, <roger.pau@citrix.com> wrote:
> El 24/11/15 a les 14.34, Jan Beulich ha escrit:
>>>>> On 24.11.15 at 14:10, <roger.pau@citrix.com> wrote:
>>> El 20/11/15 a les 16.49, Jan Beulich ha escrit:
>>>>>>> On 18.11.15 at 17:37, <roger.pau@citrix.com> wrote:
>>>>> @@ -157,6 +159,8 @@ struct hvm_hw_cpu {
>>>>>      };
>>>>>      /* error code for pending event */
>>>>>      uint32_t error_code;
>>>>> +    /* is fpu initialised? */
>>>>> +    uint32_t fpu_initialised;
>>>>
>>>> A whole uint32_t for just one bit? Didn't we talk about making this
>>>> new field a flags one, consuming just one bit from it?
>>>
>>> AFAIK we agreed on adding this field to the tail and making it a
>>> uint32_t so that when new fields are added they can be detected by
>>> looking at the size of the structure:
>>>
>>> http://marc.info/?l=xen-devel&m=144490321208291 
>> 
>> Admittedly it's a little implicit, but that mail has, in its quoting parts,
>> 
>> "... (and validate unused tail bits are zero, so they can be used for
>> something later on)"
>> 
>> going back to that intention of using just a single bit here afaict.
> 
> Ack. I have to admit I've misunderstood that part. Then I guess the
> field should also have a more generic name, like "flags", and
> fpu_initialised should be defined as (1U << 0).

Yes.

Jan