[PATCH] libxc: create an initial FPU state for HVM guests

[PATCH] libxc: create an initial FPU state for HVM guests

[Roger Pau Monne]

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.

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 | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
index dd331bf..f8d580c 100644
--- a/tools/libxc/xc_dom_x86.c
+++ b/tools/libxc/xc_dom_x86.c
@@ -841,6 +841,23 @@ static int vcpu_hvm(struct xc_dom_image *dom)
         struct hvm_save_descriptor end_d;
         HVM_SAVE_TYPE(END) end;
     } bsp_ctx;
+    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;
 
@@ -908,6 +925,23 @@ 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] libxc: create an initial FPU state for HVM guests

[Andrew Cooper]

On 13/10/15 14:32, Roger Pau Monne wrote:
> 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.
>
> 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>

It should be noted that this is the root cause for the current issue
OSSTest has identified while booting windows.

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

This will do until we fix the hvm save/load code properly.

> ---
>  tools/libxc/xc_dom_x86.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
>
> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
> index dd331bf..f8d580c 100644
> --- a/tools/libxc/xc_dom_x86.c
> +++ b/tools/libxc/xc_dom_x86.c
> @@ -841,6 +841,23 @@ static int vcpu_hvm(struct xc_dom_image *dom)
>          struct hvm_save_descriptor end_d;
>          HVM_SAVE_TYPE(END) end;
>      } bsp_ctx;
> +    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;
>  
> @@ -908,6 +925,23 @@ 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;
>  


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

Re: [PATCH] libxc: create an initial FPU state for HVM guests

[Ian Campbell]

On Tue, 2015-10-13 at 15:32 +0200, Roger Pau Monne wrote:
> 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.
> 
> 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 | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
> index dd331bf..f8d580c 100644
> --- a/tools/libxc/xc_dom_x86.c
> +++ b/tools/libxc/xc_dom_x86.c
> @@ -841,6 +841,23 @@ static int vcpu_hvm(struct xc_dom_image *dom)
>          struct hvm_save_descriptor end_d;
>          HVM_SAVE_TYPE(END) end;
>      } bsp_ctx;
> +    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;
>  
> @@ -908,6 +925,23 @@ 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.

I suppose we aren't just doing that now because there is some complexity in
doing so? Can that be mentioned in the commit log please.

Other than that if the hypervisor folks are happy with a) the approach and
b) the specifics of fpu_ctxt described below then it's fine by me too.


> +     *
> +     * 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;
>  

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

Re: [PATCH] libxc: create an initial FPU state for HVM guests

[Roger Pau Monné]

El 13/10/15 a les 15.42, Ian Campbell ha escrit:
> On Tue, 2015-10-13 at 15:32 +0200, Roger Pau Monne wrote:
>> 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.
>>
>> 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 | 34 ++++++++++++++++++++++++++++++++++
>>  1 file changed, 34 insertions(+)
>>
>> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
>> index dd331bf..f8d580c 100644
>> --- a/tools/libxc/xc_dom_x86.c
>> +++ b/tools/libxc/xc_dom_x86.c
>> @@ -841,6 +841,23 @@ static int vcpu_hvm(struct xc_dom_image *dom)
>>          struct hvm_save_descriptor end_d;
>>          HVM_SAVE_TYPE(END) end;
>>      } bsp_ctx;
>> +    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;
>>  
>> @@ -908,6 +925,23 @@ 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.
> 
> I suppose we aren't just doing that now because there is some complexity in
> doing so? Can that be mentioned in the commit log please.

Yes, this is the easiest way for me to unblock the push gate.

I'm going to work on fixing the CPU save/restore stuff ASAP, but it's
probably going to be a more complex patch(es) involving compatibility
layers and stuff.

I've added the following to the commit message:

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.

Roger.

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

Re: [PATCH] libxc: create an initial FPU state for HVM guests

[Jan Beulich]

>>> On 13.10.15 at 15:32, <roger.pau@citrix.com> wrote:
> --- a/tools/libxc/xc_dom_x86.c
> +++ b/tools/libxc/xc_dom_x86.c
> @@ -841,6 +841,23 @@ static int vcpu_hvm(struct xc_dom_image *dom)
>          struct hvm_save_descriptor end_d;
>          HVM_SAVE_TYPE(END) end;
>      } bsp_ctx;
> +    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;

I think a comment should be added here that this layout is the 64-bit
one, no matter what bitness the tool stack. Or perhaps leave out all
pieces that you don't need; the ones you care about live at the same
offsets in both 32- and 64-bit variants.

Jan

Re: [PATCH] libxc: create an initial FPU state for HVM guests

[Roger Pau Monné]

El 13/10/15 a les 16.43, Jan Beulich ha escrit:
>>>> On 13.10.15 at 15:32, <roger.pau@citrix.com> wrote:
>> --- a/tools/libxc/xc_dom_x86.c
>> +++ b/tools/libxc/xc_dom_x86.c
>> @@ -841,6 +841,23 @@ static int vcpu_hvm(struct xc_dom_image *dom)
>>          struct hvm_save_descriptor end_d;
>>          HVM_SAVE_TYPE(END) end;
>>      } bsp_ctx;
>> +    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;
> 
> I think a comment should be added here that this layout is the 64-bit
> one, no matter what bitness the tool stack. Or perhaps leave out all
> pieces that you don't need; the ones you care about live at the same
> offsets in both 32- and 64-bit variants.

The layout of this structure is exactly the same for 32 and 64bits, I'm
going to add a comment stating this.

Roger.

Re: [PATCH] libxc: create an initial FPU state for HVM guests

[Jan Beulich]

>>> On 13.10.15 at 18:18, <roger.pau@citrix.com> wrote:
> El 13/10/15 a les 16.43, Jan Beulich ha escrit:
>>>>> On 13.10.15 at 15:32, <roger.pau@citrix.com> wrote:
>>> --- a/tools/libxc/xc_dom_x86.c
>>> +++ b/tools/libxc/xc_dom_x86.c
>>> @@ -841,6 +841,23 @@ static int vcpu_hvm(struct xc_dom_image *dom)
>>>          struct hvm_save_descriptor end_d;
>>>          HVM_SAVE_TYPE(END) end;
>>>      } bsp_ctx;
>>> +    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;
>> 
>> I think a comment should be added here that this layout is the 64-bit
>> one, no matter what bitness the tool stack. Or perhaps leave out all
>> pieces that you don't need; the ones you care about live at the same
>> offsets in both 32- and 64-bit variants.
> 
> The layout of this structure is exactly the same for 32 and 64bits, I'm
> going to add a comment stating this.

Oh, sorry, I mixed this up with the FSAVE/FRSTOR layout, of which we
still have traces in our tree. I'll go clean this up.

Jan