diff mbox

[v2] x86/HVM: change the flags cpu context field to uint64_t

Message ID 1453129771-78737-1-git-send-email-roger.pau@citrix.com (mailing list archive)
State New, archived
Headers show

Commit Message

Roger Pau Monné Jan. 18, 2016, 3:09 p.m. UTC
So that the size of the structure is the same on 32 and 64bit.

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: Andrew Cooper <andrew.cooper3@citrix.com>
---
This should fix the issues seen on OSSTest when using a 32bit toolstack on
a 64bit hypervisor to create a Windows 7 HVM guest.
---
Changes since v1:
 - Instead of adding padding, change the flags field to be a uint64_t.
---
 xen/arch/x86/hvm/hvm.c                 | 2 +-
 xen/include/public/arch-x86/hvm/save.h | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

Comments

Andrew Cooper Jan. 18, 2016, 3:11 p.m. UTC | #1
On 18/01/16 15:09, Roger Pau Monne wrote:
> So that the size of the structure is the same on 32 and 64bit.
>
> 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: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Jan Beulich Jan. 18, 2016, 3:21 p.m. UTC | #2
>>> On 18.01.16 at 16:09, <roger.pau@citrix.com> wrote:
> --- a/xen/include/public/arch-x86/hvm/save.h
> +++ b/xen/include/public/arch-x86/hvm/save.h
> @@ -161,8 +161,8 @@ struct hvm_hw_cpu {
>      uint32_t error_code;
>  
>  #define _XEN_X86_FPU_INITIALISED        0
> -#define XEN_X86_FPU_INITIALISED         (1U<<_XEN_X86_FPU_INITIALISED)
> -    uint32_t flags;
> +#define XEN_X86_FPU_INITIALISED         (1UL<<_XEN_X86_FPU_INITIALISED)
> +    uint64_t flags;
>  };

How is the UL going to make this safe for a 32-bit consumer?
Makes me think that, other than just said in reply to v1, it'll
indeed be better to have a separate field (with a separate
zero-check)... The (undesirable imo) alternative being to use
1L instead.

Jan
Andrew Cooper Jan. 18, 2016, 3:24 p.m. UTC | #3
On 18/01/16 15:21, Jan Beulich wrote:
>>>> On 18.01.16 at 16:09, <roger.pau@citrix.com> wrote:
>> --- a/xen/include/public/arch-x86/hvm/save.h
>> +++ b/xen/include/public/arch-x86/hvm/save.h
>> @@ -161,8 +161,8 @@ struct hvm_hw_cpu {
>>      uint32_t error_code;
>>  
>>  #define _XEN_X86_FPU_INITIALISED        0
>> -#define XEN_X86_FPU_INITIALISED         (1U<<_XEN_X86_FPU_INITIALISED)
>> -    uint32_t flags;
>> +#define XEN_X86_FPU_INITIALISED         (1UL<<_XEN_X86_FPU_INITIALISED)
>> +    uint64_t flags;
>>  };
> How is the UL going to make this safe for a 32-bit consumer?
> Makes me think that, other than just said in reply to v1, it'll
> indeed be better to have a separate field (with a separate
> zero-check)... The (undesirable imo) alternative being to use
> 1L instead.

I am happy either way.  My R-b stands.

~Andrew
Roger Pau Monné Jan. 18, 2016, 3:38 p.m. UTC | #4
El 18/01/16 a les 16.24, Andrew Cooper ha escrit:
> On 18/01/16 15:21, Jan Beulich wrote:
>>>>> On 18.01.16 at 16:09, <roger.pau@citrix.com> wrote:
>>> --- a/xen/include/public/arch-x86/hvm/save.h
>>> +++ b/xen/include/public/arch-x86/hvm/save.h
>>> @@ -161,8 +161,8 @@ struct hvm_hw_cpu {
>>>      uint32_t error_code;
>>>  
>>>  #define _XEN_X86_FPU_INITIALISED        0
>>> -#define XEN_X86_FPU_INITIALISED         (1U<<_XEN_X86_FPU_INITIALISED)
>>> -    uint32_t flags;
>>> +#define XEN_X86_FPU_INITIALISED         (1UL<<_XEN_X86_FPU_INITIALISED)
>>> +    uint64_t flags;
>>>  };
>> How is the UL going to make this safe for a 32-bit consumer?
>> Makes me think that, other than just said in reply to v1, it'll
>> indeed be better to have a separate field (with a separate
>> zero-check)... The (undesirable imo) alternative being to use
>> 1L instead.
> 
> I am happy either way.  My R-b stands.

What about using ULL or simply casting to uint64_t?

Roger.
Jan Beulich Jan. 18, 2016, 4:24 p.m. UTC | #5
>>> On 18.01.16 at 16:38, <roger.pau@citrix.com> wrote:
> El 18/01/16 a les 16.24, Andrew Cooper ha escrit:
>> On 18/01/16 15:21, Jan Beulich wrote:
>>>>>> On 18.01.16 at 16:09, <roger.pau@citrix.com> wrote:
>>>> --- a/xen/include/public/arch-x86/hvm/save.h
>>>> +++ b/xen/include/public/arch-x86/hvm/save.h
>>>> @@ -161,8 +161,8 @@ struct hvm_hw_cpu {
>>>>      uint32_t error_code;
>>>>  
>>>>  #define _XEN_X86_FPU_INITIALISED        0
>>>> -#define XEN_X86_FPU_INITIALISED         (1U<<_XEN_X86_FPU_INITIALISED)
>>>> -    uint32_t flags;
>>>> +#define XEN_X86_FPU_INITIALISED         (1UL<<_XEN_X86_FPU_INITIALISED)
>>>> +    uint64_t flags;
>>>>  };
>>> How is the UL going to make this safe for a 32-bit consumer?
>>> Makes me think that, other than just said in reply to v1, it'll
>>> indeed be better to have a separate field (with a separate
>>> zero-check)... The (undesirable imo) alternative being to use
>>> 1L instead.
>> 
>> I am happy either way.  My R-b stands.
> 
> What about using ULL or simply casting to uint64_t?

ULL might not be supported by pre-C99 compilers. Casting to
uint64_t is, well, ugly. The flags field really has no business
being wider then 32 bits.

Jan
Roger Pau Monné Jan. 18, 2016, 4:33 p.m. UTC | #6
El 18/01/16 a les 17.24, Jan Beulich ha escrit:
>>>> On 18.01.16 at 16:38, <roger.pau@citrix.com> wrote:
>> El 18/01/16 a les 16.24, Andrew Cooper ha escrit:
>>> On 18/01/16 15:21, Jan Beulich wrote:
>>>>>>> On 18.01.16 at 16:09, <roger.pau@citrix.com> wrote:
>>>>> --- a/xen/include/public/arch-x86/hvm/save.h
>>>>> +++ b/xen/include/public/arch-x86/hvm/save.h
>>>>> @@ -161,8 +161,8 @@ struct hvm_hw_cpu {
>>>>>      uint32_t error_code;
>>>>>  
>>>>>  #define _XEN_X86_FPU_INITIALISED        0
>>>>> -#define XEN_X86_FPU_INITIALISED         (1U<<_XEN_X86_FPU_INITIALISED)
>>>>> -    uint32_t flags;
>>>>> +#define XEN_X86_FPU_INITIALISED         (1UL<<_XEN_X86_FPU_INITIALISED)
>>>>> +    uint64_t flags;
>>>>>  };
>>>> How is the UL going to make this safe for a 32-bit consumer?
>>>> Makes me think that, other than just said in reply to v1, it'll
>>>> indeed be better to have a separate field (with a separate
>>>> zero-check)... The (undesirable imo) alternative being to use
>>>> 1L instead.
>>>
>>> I am happy either way.  My R-b stands.
>>
>> What about using ULL or simply casting to uint64_t?
> 
> ULL might not be supported by pre-C99 compilers. Casting to
> uint64_t is, well, ugly. The flags field really has no business
> being wider then 32 bits.

Right, although Xen uses gnu99 this is a public header.

You should see a v3 somewhere with a proper check for the padding filed.
While doing this I've also realised that the padding fields in the other
structs in the same file don't seem to be checked at all.

Roger.
Jan Beulich Jan. 18, 2016, 4:43 p.m. UTC | #7
>>> On 18.01.16 at 17:33, <roger.pau@citrix.com> wrote:
> El 18/01/16 a les 17.24, Jan Beulich ha escrit:
>> ULL might not be supported by pre-C99 compilers. Casting to
>> uint64_t is, well, ugly. The flags field really has no business
>> being wider then 32 bits.
> 
> Right, although Xen uses gnu99 this is a public header.
> 
> You should see a v3 somewhere with a proper check for the padding filed.

Thanks.

> While doing this I've also realised that the padding fields in the other
> structs in the same file don't seem to be checked at all.

For ones which already went out in a release we may not be able to
do anything retroactively.

Jan
diff mbox

Patch

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index a99edc2..1364d16 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -2017,7 +2017,7 @@  static int hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
 
     if ( (ctxt.flags & ~XEN_X86_FPU_INITIALISED) != 0 )
     {
-        gprintk(XENLOG_ERR, "bad flags value in CPU context: %#x\n",
+        gprintk(XENLOG_ERR, "bad flags value in CPU context: %#lx\n",
                 ctxt.flags);
         return -EINVAL;
     }
diff --git a/xen/include/public/arch-x86/hvm/save.h b/xen/include/public/arch-x86/hvm/save.h
index b6b1bf8..3fac45b 100644
--- a/xen/include/public/arch-x86/hvm/save.h
+++ b/xen/include/public/arch-x86/hvm/save.h
@@ -161,8 +161,8 @@  struct hvm_hw_cpu {
     uint32_t error_code;
 
 #define _XEN_X86_FPU_INITIALISED        0
-#define XEN_X86_FPU_INITIALISED         (1U<<_XEN_X86_FPU_INITIALISED)
-    uint32_t flags;
+#define XEN_X86_FPU_INITIALISED         (1UL<<_XEN_X86_FPU_INITIALISED)
+    uint64_t flags;
 };
 
 struct hvm_hw_cpu_compat {