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

[PATCH RFC 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.

Since I don't have a pre-3.4 Xen box, I would like to request if this patch 
series can be tested by the XenServer CI loop, which I've heard tests 
migrations between very old Xen versions.

Thanks, Roger.

[PATCH RFC 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..411871e 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, _code) __HVM_SAVE_FIX_COMPAT_##_x(_dst, _code)
 #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 RFC 2/3] xen/hvm: introduce a fpu_initialised filed 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>
---
 xen/arch/x86/hvm/hvm.c                 |   5 +-
 xen/include/public/arch-x86/hvm/save.h | 146 ++++++++++++++++++++++++++++++---
 2 files changed, 139 insertions(+), 12 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 3fa2280..e71570c 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1796,6 +1796,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;
@@ -1975,7 +1976,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. */
@@ -2118,7 +2119,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..c4863be 100644
--- a/xen/include/public/arch-x86/hvm/save.h
+++ b/xen/include/public/arch-x86/hvm/save.h
@@ -47,7 +47,8 @@ DECLARE_HVM_SAVE_TYPE(HEADER, 1, struct hvm_save_header);
 /*
  * Processor
  *
- * Compat: Pre-3.4 didn't have msr_tsc_aux
+ * Compat2: Pre-4.7 didn't have fpu_initialised
+ * Compat1: Pre-3.4 didn't have msr_tsc_aux
  */
 
 struct hvm_hw_cpu {
@@ -157,9 +158,121 @@ struct hvm_hw_cpu {
     };
     /* error code for pending event */
     uint32_t error_code;
+    /* is fpu initialised? */
+    uint8_t fpu_initialised:1;
 };
 
-struct hvm_hw_cpu_compat {
+struct hvm_hw_cpu_compat2 {
+    uint8_t  fpu_regs[512];
+
+    uint64_t rax;
+    uint64_t rbx;
+    uint64_t rcx;
+    uint64_t rdx;
+    uint64_t rbp;
+    uint64_t rsi;
+    uint64_t rdi;
+    uint64_t rsp;
+    uint64_t r8;
+    uint64_t r9;
+    uint64_t r10;
+    uint64_t r11;
+    uint64_t r12;
+    uint64_t r13;
+    uint64_t r14;
+    uint64_t r15;
+
+    uint64_t rip;
+    uint64_t rflags;
+
+    uint64_t cr0;
+    uint64_t cr2;
+    uint64_t cr3;
+    uint64_t cr4;
+
+    uint64_t dr0;
+    uint64_t dr1;
+    uint64_t dr2;
+    uint64_t dr3;
+    uint64_t dr6;
+    uint64_t dr7;    
+
+    uint32_t cs_sel;
+    uint32_t ds_sel;
+    uint32_t es_sel;
+    uint32_t fs_sel;
+    uint32_t gs_sel;
+    uint32_t ss_sel;
+    uint32_t tr_sel;
+    uint32_t ldtr_sel;
+
+    uint32_t cs_limit;
+    uint32_t ds_limit;
+    uint32_t es_limit;
+    uint32_t fs_limit;
+    uint32_t gs_limit;
+    uint32_t ss_limit;
+    uint32_t tr_limit;
+    uint32_t ldtr_limit;
+    uint32_t idtr_limit;
+    uint32_t gdtr_limit;
+
+    uint64_t cs_base;
+    uint64_t ds_base;
+    uint64_t es_base;
+    uint64_t fs_base;
+    uint64_t gs_base;
+    uint64_t ss_base;
+    uint64_t tr_base;
+    uint64_t ldtr_base;
+    uint64_t idtr_base;
+    uint64_t gdtr_base;
+
+    uint32_t cs_arbytes;
+    uint32_t ds_arbytes;
+    uint32_t es_arbytes;
+    uint32_t fs_arbytes;
+    uint32_t gs_arbytes;
+    uint32_t ss_arbytes;
+    uint32_t tr_arbytes;
+    uint32_t ldtr_arbytes;
+
+    uint64_t sysenter_cs;
+    uint64_t sysenter_esp;
+    uint64_t sysenter_eip;
+
+    /* msr for em64t */
+    uint64_t shadow_gs;
+
+    /* msr content saved/restored. */
+    uint64_t msr_flags;
+    uint64_t msr_lstar;
+    uint64_t msr_star;
+    uint64_t msr_cstar;
+    uint64_t msr_syscall_mask;
+    uint64_t msr_efer;
+    uint64_t msr_tsc_aux;
+
+    /* guest's idea of what rdtsc() would return */
+    uint64_t tsc;
+
+    /* pending event, if any */
+    union {
+        uint32_t pending_event;
+        struct {
+            uint8_t  pending_vector:8;
+            uint8_t  pending_type:3;
+            uint8_t  pending_error_valid:1;
+            uint32_t pending_reserved:19;
+            uint8_t  pending_valid:1;
+        };
+    };
+    /* error code for pending event */
+    uint32_t error_code;
+    /*uint8_t fpu_initialised:1; COMPAT */
+};
+
+struct hvm_hw_cpu_compat1 {
     uint8_t  fpu_regs[512];
 
     uint64_t rax;
@@ -266,27 +379,40 @@ struct hvm_hw_cpu_compat {
     };
     /* error code for pending event */
     uint32_t error_code;
+    /*uint8_t fpu_initialised:1; COMPAT */
+};
+
+union hvm_hw_cpu_compat {
+    struct hvm_hw_cpu_compat1 cmp1;
+    struct hvm_hw_cpu_compat2 cmp2;
 };
 
 static inline int _hvm_hw_fix_cpu(void *h, int size) {
 
     union hvm_hw_cpu_union {
         struct hvm_hw_cpu nat;
-        struct hvm_hw_cpu_compat cmp;
+        union 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_compat1) )
+    {
+        /*
+         * If we copy from the end backwards, we should
+         * be able to do the modification in-place.
+         */
+        ucpu->nat.error_code = ucpu->cmp.cmp1.error_code;
+        ucpu->nat.pending_event = ucpu->cmp.cmp1.pending_event;
+        ucpu->nat.tsc = ucpu->cmp.cmp1.tsc;
+        ucpu->nat.msr_tsc_aux = 0;
+    }
+    /* Mimic the old behaviour by unconditionally setting fpu_initialised. */
+    ucpu->nat.fpu_initialised = 1;
 
     return 0;
 }
 
 DECLARE_HVM_SAVE_TYPE_COMPAT(CPU, 2, struct hvm_hw_cpu, \
-                             struct hvm_hw_cpu_compat, _hvm_hw_fix_cpu);
+                             union hvm_hw_cpu_compat, _hvm_hw_fix_cpu);
 
 /*
  * PIC
-- 
1.9.5 (Apple Git-50.3)


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

[PATCH RFC 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.

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 034abe0..dd331bf 100644
--- a/tools/libxc/xc_dom_x86.c
+++ b/tools/libxc/xc_dom_x86.c
@@ -841,27 +841,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;
 
@@ -929,23 +908,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 RFC 3/3] Revert "libxc: create an initial FPU state for HVM guests"

[Wei Liu]

On Wed, Oct 14, 2015 at 06:24:40PM +0200, 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.

I think it is better to say in the commit message that a proper fix is
in place so we can revert the stop-gap patch instead of copying the
commit message from the patch this is being reverted.

Assuming I'm right about a proper fix will be committed before this
patch and the commit message fixed:

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

Re: [PATCH RFC 2/3] xen/hvm: introduce a fpu_initialised filed to the CPU save record

[Andrew Cooper]

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

"field", I am guessing?

> diff --git a/xen/include/public/arch-x86/hvm/save.h b/xen/include/public/arch-x86/hvm/save.h
> index c7560f2..c4863be 100644
> --- a/xen/include/public/arch-x86/hvm/save.h
> +++ b/xen/include/public/arch-x86/hvm/save.h
> @@ -47,7 +47,8 @@ DECLARE_HVM_SAVE_TYPE(HEADER, 1, struct hvm_save_header);
>  /*
>   * Processor
>   *
> - * Compat: Pre-3.4 didn't have msr_tsc_aux
> + * Compat2: Pre-4.7 didn't have fpu_initialised
> + * Compat1: Pre-3.4 didn't have msr_tsc_aux

I would suggest reversing the Compat1 and 2 lines, to match chronology.

>   */
>  
>  struct hvm_hw_cpu {
> @@ -157,9 +158,121 @@ struct hvm_hw_cpu {
>      };
>      /* error code for pending event */
>      uint32_t error_code;
> +    /* is fpu initialised? */
> +    uint8_t fpu_initialised:1;

Bitfields can't be used in the public ABI, and please don't leave
trailing implicit padding.

I would recommend uint32_t flags and specify that bit 0 indicates that
fpu context is initialised.

~Andrew

Re: [PATCH RFC 1/3] xen/save: pass a size paramter to the HVM compat functions

[Jan Beulich]

>>> On 14.10.15 at 18:24, <roger.pau@citrix.com> 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.

Having peeked at patch 2, this won't help once another bit gets added
to the tail of that structure. Also it doesn't seem logical that the
previous compat handling got around without being passed the size
explicitly. I.e. while perhaps more involved, I think the compat
handling needs to be extended to allow for multiple versions.

Or, since we have this under control going forward, don't even declare
all the various compat structures in the public header (and only ever
add to the tail). Then staying with the passing of size probably makes
sense, but the fixup function then should use offsetof() instead of
sizeof() (and validate unused tail bits are zero, so they can be used for
something later on).

Jan

Re: [PATCH RFC 2/3] xen/hvm: introduce a fpu_initialised filed to the CPU save record

[Jan Beulich]

>>> On 14.10.15 at 18:51, <andrew.cooper3@citrix.com> wrote:
> On 14/10/15 17:24, Roger Pau Monne wrote:
>> @@ -157,9 +158,121 @@ struct hvm_hw_cpu {
>>      };
>>      /* error code for pending event */
>>      uint32_t error_code;
>> +    /* is fpu initialised? */
>> +    uint8_t fpu_initialised:1;
> 
> Bitfields can't be used in the public ABI, and please don't leave
> trailing implicit padding.
> 
> I would recommend uint32_t flags and specify that bit 0 indicates that
> fpu context is initialised.

Seconded, despite the bad precedent in the pending event sub-union.

Jan

Re: [PATCH RFC 1/3] xen/save: pass a size paramter to the HVM compat functions

[Roger Pau Monné]

El 15/10/15 a les 10.12, Jan Beulich ha escrit:
>>>> On 14.10.15 at 18:24, <roger.pau@citrix.com> 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.
> 
> Having peeked at patch 2, this won't help once another bit gets added
> to the tail of that structure. Also it doesn't seem logical that the
> previous compat handling got around without being passed the size
> explicitly. I.e. while perhaps more involved, I think the compat
> handling needs to be extended to allow for multiple versions.

Yes, patch #2 needs to be fixed by making fpu_initialised an uint32_t.
Adding versions would be the best option IMHO, but I don't think I have
enough time to do that myself right now.

> Or, since we have this under control going forward, don't even declare
> all the various compat structures in the public header (and only ever
> add to the tail). Then staying with the passing of size probably makes
> sense, but the fixup function then should use offsetof() instead of
> sizeof() (and validate unused tail bits are zero, so they can be used for
> something later on).

Since we use hvm_load_entry_zeroextend I think we can assert that all
new tail bits are going to be 0, but I'm not sure I follow you regarding
the usage of offsetof instead of sizeof. What are we going to compare
with offsetof?

Also, we already have a compat version of hvm_hw_cpu that didn't add the
new field to the end.

Roger.

Re: [PATCH RFC 2/3] xen/hvm: introduce a fpu_initialised filed to the CPU save record

[Roger Pau Monné]

El 14/10/15 a les 18.51, Andrew Cooper ha escrit:
> On 14/10/15 17:24, Roger Pau Monne wrote:
>> Introduce a new filed to signal if the FPU has been initialised or not. Xen
> 
> "field", I am guessing?

Yes.

>> diff --git a/xen/include/public/arch-x86/hvm/save.h b/xen/include/public/arch-x86/hvm/save.h
>> index c7560f2..c4863be 100644
>> --- a/xen/include/public/arch-x86/hvm/save.h
>> +++ b/xen/include/public/arch-x86/hvm/save.h
>> @@ -47,7 +47,8 @@ DECLARE_HVM_SAVE_TYPE(HEADER, 1, struct hvm_save_header);
>>  /*
>>   * Processor
>>   *
>> - * Compat: Pre-3.4 didn't have msr_tsc_aux
>> + * Compat2: Pre-4.7 didn't have fpu_initialised
>> + * Compat1: Pre-3.4 didn't have msr_tsc_aux
> 
> I would suggest reversing the Compat1 and 2 lines, to match chronology.

OK, that's fine.

>>   */
>>  
>>  struct hvm_hw_cpu {
>> @@ -157,9 +158,121 @@ struct hvm_hw_cpu {
>>      };
>>      /* error code for pending event */
>>      uint32_t error_code;
>> +    /* is fpu initialised? */
>> +    uint8_t fpu_initialised:1;
> 
> Bitfields can't be used in the public ABI, and please don't leave
> trailing implicit padding.

Sadly we already have quite a lot of bitfields in the same file (see
hvm_hw_vpic) but I understand why they shouldn't be used, specially if
we do the versioning based on the size of the structure.

> I would recommend uint32_t flags and specify that bit 0 indicates that
> fpu context is initialised.

Right.

Roger.

Re: [PATCH RFC 1/3] xen/save: pass a size paramter to the HVM compat functions

[Jan Beulich]

>>> On 15.10.15 at 11:49, <roger.pau@citrix.com> wrote:
> El 15/10/15 a les 10.12, Jan Beulich ha escrit:
>>>>> On 14.10.15 at 18:24, <roger.pau@citrix.com> 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.
>> 
>> Having peeked at patch 2, this won't help once another bit gets added
>> to the tail of that structure. Also it doesn't seem logical that the
>> previous compat handling got around without being passed the size
>> explicitly. I.e. while perhaps more involved, I think the compat
>> handling needs to be extended to allow for multiple versions.
> 
> Yes, patch #2 needs to be fixed by making fpu_initialised an uint32_t.
> Adding versions would be the best option IMHO, but I don't think I have
> enough time to do that myself right now.

Understood. An I don't think we really need versioning here.

>> Or, since we have this under control going forward, don't even declare
>> all the various compat structures in the public header (and only ever
>> add to the tail). Then staying with the passing of size probably makes
>> sense, but the fixup function then should use offsetof() instead of
>> sizeof() (and validate unused tail bits are zero, so they can be used for
>> something later on).
> 
> Since we use hvm_load_entry_zeroextend I think we can assert that all
> new tail bits are going to be 0, but I'm not sure I follow you regarding
> the usage of offsetof instead of sizeof. What are we going to compare
> with offsetof?

Instead of introducing compat1 and compat2 structures, just add
the new field to the existing structure, and use offsetof() to compare
the passed in size with the that of the structure in its original state.

> Also, we already have a compat version of hvm_hw_cpu that didn't add the
> new field to the end.

Right, but we can avoid making the same mistake again.

Jan

Re: [PATCH RFC 1/3] xen/save: pass a size paramter to the HVM compat functions

[Andrew Cooper]

On 15/10/15 10:59, Jan Beulich wrote:
>>>> On 15.10.15 at 11:49, <roger.pau@citrix.com> wrote:
>> El 15/10/15 a les 10.12, Jan Beulich ha escrit:
>>>>>> On 14.10.15 at 18:24, <roger.pau@citrix.com> 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.
>>> Having peeked at patch 2, this won't help once another bit gets added
>>> to the tail of that structure. Also it doesn't seem logical that the
>>> previous compat handling got around without being passed the size
>>> explicitly. I.e. while perhaps more involved, I think the compat
>>> handling needs to be extended to allow for multiple versions.
>> Yes, patch #2 needs to be fixed by making fpu_initialised an uint32_t.
>> Adding versions would be the best option IMHO, but I don't think I have
>> enough time to do that myself right now.
> Understood. An I don't think we really need versioning here.
>
>>> Or, since we have this under control going forward, don't even declare
>>> all the various compat structures in the public header (and only ever
>>> add to the tail). Then staying with the passing of size probably makes
>>> sense, but the fixup function then should use offsetof() instead of
>>> sizeof() (and validate unused tail bits are zero, so they can be used for
>>> something later on).
>> Since we use hvm_load_entry_zeroextend I think we can assert that all
>> new tail bits are going to be 0, but I'm not sure I follow you regarding
>> the usage of offsetof instead of sizeof. What are we going to compare
>> with offsetof?
> Instead of introducing compat1 and compat2 structures, just add
> the new field to the existing structure, and use offsetof() to compare
> the passed in size with the that of the structure in its original state.
>
>> Also, we already have a compat version of hvm_hw_cpu that didn't add the
>> new field to the end.
> Right, but we can avoid making the same mistake again.

Please be aware that, in due course, I will be replacing all of this,
migration v2 style, as a prerequisite of getting cpuid policies
functioning correctly during migrate.

While we shouldn't paint ourselves into a corner, I wouldn't worry too
much about their longevity.

~Andrew

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

[Ian Campbell]

On Wed, 2015-10-14 at 17:30 +0100, Wei Liu wrote:
> On Wed, Oct 14, 2015 at 06:24:40PM +0200, 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.
> 
> I think it is better to say in the commit message that a proper fix is
> in place so we can revert the stop-gap patch instead of copying the
> commit message from the patch this is being reverted.
> 
> Assuming I'm right about a proper fix will be committed before this
> patch and the commit message fixed:

Roger, Is the "proper fix" in tree now? If so then please can we get an
updated commit message which references that as Wei suggests.

I can update the message upon commit if you just want to provide the words,
or feel free to resend of course.

Ian.


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

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

[Roger Pau Monné]

El 16/11/15 a les 12.51, Ian Campbell ha escrit:
> On Wed, 2015-10-14 at 17:30 +0100, Wei Liu wrote:
>> On Wed, Oct 14, 2015 at 06:24:40PM +0200, 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.
>>
>> I think it is better to say in the commit message that a proper fix is
>> in place so we can revert the stop-gap patch instead of copying the
>> commit message from the patch this is being reverted.
>>
>> Assuming I'm right about a proper fix will be committed before this
>> patch and the commit message fixed:
> 
> Roger, Is the "proper fix" in tree now? If so then please can we get an
> updated commit message which references that as Wei suggests.
> 
> I can update the message upon commit if you just want to provide the words,
> or feel free to resend of course.

No, a proper fix is still not in place, sorry. Let me see if I can get a
new version out today.

Roger.