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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.11.15 at 16:18, <roger.pau@xxxxxxxxxx> 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, &seg);
>  
> +    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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel