| Message ID | 1453129771-78737-1-git-send-email-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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>
>>> 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
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
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.
>>> 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
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.
>>> 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 --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 {
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(-)