Re: [Xen-devel] [PATCH v4 3/4] xen/hvm: introduce a flags field in the CPU save record

2015-11-30 Thread Jan Beulich 
>>> On 27.11.15 at 17:15,  wrote:
> El 26/11/15 a les 15.32, Jan Beulich ha escrit:
> On 25.11.15 at 16:18,  wrote:
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -2085,16 +2091,17 @@ static int hvm_load_cpu_ctxt(struct domain *d, 
> hvm_domain_context_t *h)
>>>  seg.attr.bytes = ctxt.ldtr_arbytes;
>>>  hvm_set_segment_register(v, x86_seg_ldtr, );
>>>  
>>> +v->fpu_initialised = !!(ctxt.flags & XEN_X86_FPU_INITIALISED);
>>>  /* In case xsave-absent save file is restored on a xsave-capable host 
>>> */
>>> -if ( cpu_has_xsave && !xsave_enabled(v) )
>>> +if ( cpu_has_xsave && !xsave_enabled(v) && v->fpu_initialised )
>> 
>> Hmm, didn't I pretty explicitly ask for this to become
>> 
>> if ( !v->fpu_initialised )
>> memset();
> 
> I don't think this is possible with the current code.
> 
> Sadly the XSTATE stuff is kind of messy IMHO.

Agreed.

> vcpu_init_fpu calls
> xstate_alloc_save_area which on a XSAVE capable CPUs allocates _and_
> initializes the FPU registers, while on non-XSAVE capable CPUs
> vcpu_init_fpu just allocates the FPU memory, but doesn't initialize the
> registers.

Well, I can't see registers being initialized. All I can see is MXCSR
and FCW getting values set in the memory image, which I suppose
is what you meant. But that can be easily mirrored here (which
then would also get closer to what hvm_vcpu_reset_state() does).

> So either xstate_alloc_save_area also sets v->fpu_initialised = 1 (this
> is the simplest solution), or xstate_alloc_save_area is reworked so it
> only allocates the XSAVE area, but doesn't initialize it. Then XSAVE
> area initialization should be done in vcpu_restore_fpu_lazy.

Re-working along those lines certainly is also an option.

Jan


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

Re: [Xen-devel] [PATCH v4 3/4] xen/hvm: introduce a flags field in the CPU save record

2015-11-26 Thread Jan Beulich 
>>> On 25.11.15 at 16:18,  wrote:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -1798,8 +1798,7 @@ static int hvm_save_cpu_ctxt(struct domain *d, 
> hvm_domain_context_t *h)
>  
>  if ( v->fpu_initialised )
>  memcpy(ctxt.fpu_regs, v->arch.fpu_ctxt, sizeof(ctxt.fpu_regs));
> -else 
> -memset(ctxt.fpu_regs, 0, sizeof(ctxt.fpu_regs));
> +ctxt.flags = v->fpu_initialised ? XEN_X86_FPU_INITIALISED : 0;

By dropping the memset() you'll leak hypervisor stack contents to
the tool stack / into the save file. Also I think two conditionals
using the same expression would better be combined.

> @@ -2085,16 +2091,17 @@ static int hvm_load_cpu_ctxt(struct domain *d, 
> hvm_domain_context_t *h)
>  seg.attr.bytes = ctxt.ldtr_arbytes;
>  hvm_set_segment_register(v, x86_seg_ldtr, );
>  
> +v->fpu_initialised = !!(ctxt.flags & XEN_X86_FPU_INITIALISED);
>  /* In case xsave-absent save file is restored on a xsave-capable host */
> -if ( cpu_has_xsave && !xsave_enabled(v) )
> +if ( cpu_has_xsave && !xsave_enabled(v) && v->fpu_initialised )

Hmm, didn't I pretty explicitly ask for this to become

if ( !v->fpu_initialised )
memset();
else if ( ... ) ...
else ...

>  {
>  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;
>  }
> -else
> -memcpy(v->arch.fpu_ctxt, ctxt.fpu_regs, sizeof(ctxt.fpu_regs));
> +else if ( v->fpu_initialised )
> +memcpy(v->arch.fpu_ctxt, ctxt.fpu_regs, sizeof(ctxt.fpu_regs));

And in no case should you break indentation here.

Jan


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

[Xen-devel] [PATCH v4 3/4] xen/hvm: introduce a flags field in the CPU save record

2015-11-25 Thread Roger Pau Monne 
Introduce a new flags field and use bit 0 to signal if the FPU has been
initialised or not. Previously Xen always wrongly assumed the FPU was
initialised on restore.

Signed-off-by: Roger Pau Monné 
Cc: Jan Beulich 
Cc: Andrew Cooper 
---
Changes since v3:
 - Don't add a comment in the compat structure regaring the fpu_initialised
   field.
 - Rename fpu_initialised to flags and use it as a bit field. Bit 0 will be
   used to signal whether the fpu is initialised.
 - Only save the fpu context if it's initialised.
 - Only restore the fpu context from the save record if the fpu is
   initialised.
 - Check that unused bits in the flags field are 0.

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 | 20 +---
 xen/include/public/arch-x86/hvm/save.h | 27 ---
 2 files changed, 33 insertions(+), 14 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 141a130..d966074 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1798,8 +1798,7 @@ static int hvm_save_cpu_ctxt(struct domain *d, 
hvm_domain_context_t *h)
 
 if ( v->fpu_initialised )
 memcpy(ctxt.fpu_regs, v->arch.fpu_ctxt, sizeof(ctxt.fpu_regs));
-else 
-memset(ctxt.fpu_regs, 0, sizeof(ctxt.fpu_regs));
+ctxt.flags = v->fpu_initialised ? XEN_X86_FPU_INITIALISED : 0;
 
 ctxt.rax = v->arch.user_regs.eax;
 ctxt.rbx = v->arch.user_regs.ebx;
@@ -1979,7 +1978,7 @@ static int hvm_load_cpu_ctxt(struct domain *d, 
hvm_domain_context_t *h)
 return -EINVAL;
 }
 
-if ( hvm_load_entry(CPU, h, ) != 0 ) 
+if ( hvm_load_entry_zeroextend(CPU, h, ) != 0 )
 return -EINVAL;
 
 /* Sanity check some control registers. */
@@ -2007,6 +2006,13 @@ static int hvm_load_cpu_ctxt(struct domain *d, 
hvm_domain_context_t *h)
 return -EINVAL;
 }
 
+if ( (ctxt.flags & ~XEN_X86_FPU_INITIALISED) != 0 )
+{
+gprintk(XENLOG_ERR, "bad flags value in CPU context: %#x\n",
+ctxt.flags);
+return -EINVAL;
+}
+
 /* Older Xen versions used to save the segment arbytes directly 
  * from the VMCS on Intel hosts.  Detect this and rearrange them
  * into the struct segment_register format. */
@@ -2085,16 +2091,17 @@ static int hvm_load_cpu_ctxt(struct domain *d, 
hvm_domain_context_t *h)
 seg.attr.bytes = ctxt.ldtr_arbytes;
 hvm_set_segment_register(v, x86_seg_ldtr, );
 
+v->fpu_initialised = !!(ctxt.flags & XEN_X86_FPU_INITIALISED);
 /* In case xsave-absent save file is restored on a xsave-capable host */
-if ( cpu_has_xsave && !xsave_enabled(v) )
+if ( cpu_has_xsave && !xsave_enabled(v) && v->fpu_initialised )
 {
 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;
 }
-else
-memcpy(v->arch.fpu_ctxt, ctxt.fpu_regs, sizeof(ctxt.fpu_regs));
+else if ( v->fpu_initialised )
+memcpy(v->arch.fpu_ctxt, ctxt.fpu_regs, sizeof(ctxt.fpu_regs));
 
 v->arch.user_regs.eax = ctxt.rax;
 v->arch.user_regs.ebx = ctxt.rbx;
@@ -2122,7 +2129,6 @@ 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;
 
 /* 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..b6b1bf8 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,10 @@ struct hvm_hw_cpu {
 };
 /* error code for pending event */
 uint32_t error_code;
+
+#define _XEN_X86_FPU_INITIALISED0
+#define XEN_X86_FPU_INITIALISED (1U<<_XEN_X86_FPU_INITIALISED)
+uint32_t flags;
 };
 
 struct hvm_hw_cpu_compat {
@@ -275,12 +281,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
- *