[PATCH v2 0/3] Introduce a fpu_initilised filed to HVM CPU context

[PATCH v2 0/3] Introduce a fpu_initilised filed 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 v2 1/3] xen/save: pass a size paramter to the HVM compat functions

[Roger Pau Monne]

In order to cope with types having multiple compat versions pass a 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>
---
 xen/include/public/arch-x86/hvm/save.h |  2 +-
 xen/include/public/hvm/save.h          | 10 ++++++----
 xen/include/xen/hvm/save.h             |  2 +-
 3 files changed, 8 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..c7560f2 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, int 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..01885b4 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, int 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, int 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..14e6754 100644
--- a/xen/include/xen/hvm/save.h
+++ b/xen/include/xen/hvm/save.h
@@ -67,7 +67,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), (_h)->size);                  \
     }                                                                   \
     r; })
 
-- 
1.9.5 (Apple Git-50.3)


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

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

[Roger Pau Monne]

Introduce a new filed to signal if the FPU has been initialised or not. Xen
needs this new filed 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.
---
 xen/arch/x86/hvm/hvm.c                 |  5 +++--
 xen/include/public/arch-x86/hvm/save.h | 26 +++++++++++++++++++-------
 2 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index ea982e2..bac0962 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. */
@@ -2122,7 +2123,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 c7560f2..83752f9 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, int size) {
@@ -275,12 +280,19 @@ static inline int _hvm_hw_fix_cpu(void *h, int 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 v2 3/3] 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 is 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>
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 v2 1/3] xen/save: pass a size paramter to the HVM compat functions

[Andrew Cooper]

On 17/11/15 18:44, Roger Pau Monne wrote:
> In order to cope with types having multiple compat versions pass a 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>

s/int/uint32_t/g to match hvm_domain_context->size.

With that fixed, 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 v2 2/3] xen/hvm: introduce a fpu_uninitialised field to the CPU save record

[Andrew Cooper]

On 17/11/15 18:44, Roger Pau Monne wrote:
> Introduce a new filed to signal if the FPU has been initialised or not. Xen

field

> needs this new filed 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.

How can this work?

Making it zeroextended means that any short record will be padded with
zeroes.  As a result, the compat checking logic is skipped.

(This HVM_SAVE_* infrastructure is truly horrifying code which should
never have been accepted.  I think I have correctly followed what it is
doing, but I could be mistaken.)

~Andrew

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

Re: [PATCH v2 3/3] Revert "libxc: create an initial FPU state for HVM guests"

[Andrew Cooper]

On 17/11/15 18:44, Roger Pau Monne wrote:
> 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 is a stop-gap measure in order to unblock OSSTest Windows 7 failures

This was a

> 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>
> 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>
> ---

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 v2 3/3] Revert "libxc: create an initial FPU state for HVM guests"

[Wei Liu]

On Tue, Nov 17, 2015 at 06:44:39PM +0000, Roger Pau Monne wrote:
> 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 is 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>
> 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>

Acked-by: Wei Liu <wei.liu2@citrix.com>

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

[Roger Pau Monné]

El 17/11/15 a les 19.02, Andrew Cooper ha escrit:
> On 17/11/15 18:44, Roger Pau Monne wrote:
>> Introduce a new filed to signal if the FPU has been initialised or not. Xen
> 
> field
> 
>> needs this new filed 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.
> 
> How can this work?
> 
> Making it zeroextended means that any short record will be padded with
> zeroes.  As a result, the compat checking logic is skipped.
> 
> (This HVM_SAVE_* infrastructure is truly horrifying code which should
> never have been accepted.  I think I have correctly followed what it is
> doing, but I could be mistaken.)

No, you are completely right, it was an oversight on my side. So neither
hvm_load_entry or hvm_load_entry_zeroextend will do what I want/need.
The options I see so far are:

 - Make hvm_hw_cpu_compat hvm_hw_cpu minus the fpu_initalised field
(this means dropping support for Xen pre-3.4).

 - Rewrite part of the hvm_load_entry_zeroextend so that it will load
records with "len < expected len" and still call the fixup function.

TBH the mess with the hvm_hw_cpu_compat structure is very bad. Adding
the msr_tsc_aux in the middle of the structure makes all the compat
handling much more complicated that what it needs to be.

Roger.

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

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

[Andrew Cooper]

On 18/11/15 10:51, Roger Pau Monné wrote:
> El 17/11/15 a les 19.02, Andrew Cooper ha escrit:
>> On 17/11/15 18:44, Roger Pau Monne wrote:
>>> Introduce a new filed to signal if the FPU has been initialised or not. Xen
>> field
>>
>>> needs this new filed 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.
>> How can this work?
>>
>> Making it zeroextended means that any short record will be padded with
>> zeroes.  As a result, the compat checking logic is skipped.
>>
>> (This HVM_SAVE_* infrastructure is truly horrifying code which should
>> never have been accepted.  I think I have correctly followed what it is
>> doing, but I could be mistaken.)
> No, you are completely right, it was an oversight on my side. So neither
> hvm_load_entry or hvm_load_entry_zeroextend will do what I want/need.
> The options I see so far are:
>
>  - Make hvm_hw_cpu_compat hvm_hw_cpu minus the fpu_initalised field
> (this means dropping support for Xen pre-3.4).

Sorry - not an option.

>
>  - Rewrite part of the hvm_load_entry_zeroextend so that it will load
> records with "len < expected len" and still call the fixup function.
>
> TBH the mess with the hvm_hw_cpu_compat structure is very bad. Adding
> the msr_tsc_aux in the middle of the structure makes all the compat
> handling much more complicated that what it needs to be.

The inclusion of msr_tsc_aux in its current location was a very
regrettable mistake.


As for the problem at hand, I don't see what was wrong with v1.

Fundamentally, we have three different variations of the same structure;
two of which require special compat handling.  Pretending otherwise is
just silly.

~Andrew

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

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

[Jan Beulich]

>>> On 18.11.15 at 12:00, <andrew.cooper3@citrix.com> wrote:
> As for the problem at hand, I don't see what was wrong with v1.
> 
> Fundamentally, we have three different variations of the same structure;
> two of which require special compat handling.  Pretending otherwise is
> just silly.

And if we gain a few more additions, we'll end up with half a dozen
slightly different structure declarations in the public interface? If
you consider the first step (adding tsc_aux) a mistake, let's not
repeat the same mistake again (even if right now it might _seem_
to be reasonable to some of us).

Jan

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

[Andrew Cooper]

On 18/11/15 11:04, Jan Beulich wrote:
>>>> On 18.11.15 at 12:00, <andrew.cooper3@citrix.com> wrote:
>> As for the problem at hand, I don't see what was wrong with v1.
>>
>> Fundamentally, we have three different variations of the same structure;
>> two of which require special compat handling.  Pretending otherwise is
>> just silly.
> And if we gain a few more additions, we'll end up with half a dozen
> slightly different structure declarations in the public interface? If
> you consider the first step (adding tsc_aux) a mistake, let's not
> repeat the same mistake again (even if right now it might _seem_
> to be reasonable to some of us).

As part of cpuid handling fixes, I will be removing all of this from the
hypervisor

The result doesn't need to live very long; I would recommend the route
which has more-obvious-correct code.  I am not fussed as to which route
this ends up being.

~Andrew