Hi,
I had the following painful experience. I declared
struct xen_mem_event_op {
uint8_t op; /* XENMEM_*_op_* */
domid_t domain;
uint64_t buffer;
uint64_t gfn; /* IN: gfn of page being operated on */
};
typedef struct xen_mem_event_op xen_mem_event_op_t;
to be passed as the argument of a memory op called form the toolstack. The
hypervisor is 64 bits and the toolstack is 32 bits. My toolstack code
simply:
xen_mem_event_op_t meo;
... set fields ...
return do_memory_op(xch, mode, &meo, sizeof(meo));
No joy because 32 bits was packing the struct differently than 64 bits.
Namely, both were adding a 1 byte pad between ''op'' and
''domain'', but when
compiled in 64 bits mode for the hypervisor, an additional 4 byte pad was
thrown between ''domain'' and ''buffer''.
The first question is, what is the preferred way around this. Declare pads
inside the struct?
Exploring the include/public/memory.h declarations and toolstack code, I
see that no current declare includes __attribute__((aligned)) or
__attribute__((packed)), or explicit pads.
So how come things don''t break more often for 32 bit toolstacks? pure
luck? Am I missing something?
Thanks!
Andres
On 19/01/2012 20:30, "Andres Lagar-Cavilla" <andres@lagarcavilla.org> wrote:> Hi, > I had the following painful experience. I declared > > struct xen_mem_event_op { > uint8_t op; /* XENMEM_*_op_* */ > domid_t domain; > uint64_t buffer; > uint64_t gfn; /* IN: gfn of page being operated on */ > }; > typedef struct xen_mem_event_op xen_mem_event_op_t; > > to be passed as the argument of a memory op called form the toolstack. The > hypervisor is 64 bits and the toolstack is 32 bits. My toolstack code > simply: > > xen_mem_event_op_t meo; > ... set fields ... > return do_memory_op(xch, mode, &meo, sizeof(meo)); > > No joy because 32 bits was packing the struct differently than 64 bits. > Namely, both were adding a 1 byte pad between ''op'' and ''domain'', but when > compiled in 64 bits mode for the hypervisor, an additional 4 byte pad was > thrown between ''domain'' and ''buffer''. > > The first question is, what is the preferred way around this. Declare pads > inside the struct?Yes.> Exploring the include/public/memory.h declarations and toolstack code, I > see that no current declare includes __attribute__((aligned)) or > __attribute__((packed)), or explicit pads. > > So how come things don''t break more often for 32 bit toolstacks? pure > luck? Am I missing something?Where older structs were not 32/64-bit invariant, compat shims were implemented. See common/compat/memory.c, for example. Well worth avoiding that! -- Keir> Thanks! > Andres > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel
On Thu, Jan 19, 2012 at 12:30:21PM -0800, Andres Lagar-Cavilla wrote:> Hi, > I had the following painful experience. I declared > > struct xen_mem_event_op { > uint8_t op; /* XENMEM_*_op_* */ > domid_t domain; > uint64_t buffer; > uint64_t gfn; /* IN: gfn of page being operated on */ > }; > typedef struct xen_mem_event_op xen_mem_event_op_t; > > to be passed as the argument of a memory op called form the toolstack. The > hypervisor is 64 bits and the toolstack is 32 bits. My toolstack code > simply: > > xen_mem_event_op_t meo; > ... set fields ... > return do_memory_op(xch, mode, &meo, sizeof(meo)); > > No joy because 32 bits was packing the struct differently than 64 bits. > Namely, both were adding a 1 byte pad between ''op'' and ''domain'', but when > compiled in 64 bits mode for the hypervisor, an additional 4 byte pad was > thrown between ''domain'' and ''buffer''. >> The first question is, what is the preferred way around this. Declare pads > inside the struct?Yes. And also use __attribute(__packed__);> > Exploring the include/public/memory.h declarations and toolstack code, I > see that no current declare includes __attribute__((aligned)) or > __attribute__((packed)), or explicit pads.Sometimes they have #pragma(4), which will make the alignment be 32-bit (4-bytes) for 64-bit builds as well.> > So how come things don''t break more often for 32 bit toolstacks? pure > luck? Am I missing something?If you do not use ''memcpy'' you can escape some of these misues.> > Thanks! > Andres > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel
> On 19/01/2012 20:30, "Andres Lagar-Cavilla" <andres@lagarcavilla.org> > wrote: > >> Hi, >> I had the following painful experience. I declared >> >> struct xen_mem_event_op { >> uint8_t op; /* XENMEM_*_op_* */ >> domid_t domain; >> uint64_t buffer; >> uint64_t gfn; /* IN: gfn of page being operated on */ >> }; >> typedef struct xen_mem_event_op xen_mem_event_op_t; >> >> to be passed as the argument of a memory op called form the toolstack. >> The >> hypervisor is 64 bits and the toolstack is 32 bits. My toolstack code >> simply: >> >> xen_mem_event_op_t meo; >> ... set fields ... >> return do_memory_op(xch, mode, &meo, sizeof(meo)); >> >> No joy because 32 bits was packing the struct differently than 64 bits. >> Namely, both were adding a 1 byte pad between ''op'' and ''domain'', but >> when >> compiled in 64 bits mode for the hypervisor, an additional 4 byte pad >> was >> thrown between ''domain'' and ''buffer''. >> >> The first question is, what is the preferred way around this. Declare >> pads >> inside the struct? > > Yes.Ok. And as Konrad says, I''ll add __attribute__((packed)). A first for memory.h Thanks! Andres> >> Exploring the include/public/memory.h declarations and toolstack code, I >> see that no current declare includes __attribute__((aligned)) or >> __attribute__((packed)), or explicit pads. >> >> So how come things don''t break more often for 32 bit toolstacks? pure >> luck? Am I missing something? > > Where older structs were not 32/64-bit invariant, compat shims were > implemented. See common/compat/memory.c, for example. Well worth avoiding > that! > > -- Keir > >> Thanks! >> Andres >> >> >> _______________________________________________ >> Xen-devel mailing list >> Xen-devel@lists.xensource.com >> http://lists.xensource.com/xen-devel > > >
On 19/01/2012 20:49, "Konrad Rzeszutek Wilk" <konrad@darnok.org> wrote:> On Thu, Jan 19, 2012 at 12:30:21PM -0800, Andres Lagar-Cavilla wrote: >> Hi, >> I had the following painful experience. I declared >> >> struct xen_mem_event_op { >> uint8_t op; /* XENMEM_*_op_* */ >> domid_t domain; >> uint64_t buffer; >> uint64_t gfn; /* IN: gfn of page being operated on */ >> }; >> typedef struct xen_mem_event_op xen_mem_event_op_t; >> >> to be passed as the argument of a memory op called form the toolstack. The >> hypervisor is 64 bits and the toolstack is 32 bits. My toolstack code >> simply: >> >> xen_mem_event_op_t meo; >> ... set fields ... >> return do_memory_op(xch, mode, &meo, sizeof(meo)); >> >> No joy because 32 bits was packing the struct differently than 64 bits. >> Namely, both were adding a 1 byte pad between ''op'' and ''domain'', but when >> compiled in 64 bits mode for the hypervisor, an additional 4 byte pad was >> thrown between ''domain'' and ''buffer''. >> > >> The first question is, what is the preferred way around this. Declare pads >> inside the struct? > > Yes. And also use __attribute(__packed__);Not used in any public header files.>> Exploring the include/public/memory.h declarations and toolstack code, I >> see that no current declare includes __attribute__((aligned)) or >> __attribute__((packed)), or explicit pads. > > Sometimes they have #pragma(4), which will make the alignment be > 32-bit (4-bytes) for 64-bit builds as well.Not in any public header files. It''s used in the auto-generated 32-on-64 compat headers, for the hypervisor''s private use.>> So how come things don''t break more often for 32 bit toolstacks? pure >> luck? Am I missing something? > > If you do not use ''memcpy'' you can escape some of these misues.Mainly you just have to be careful. -- Keir>> >> Thanks! >> Andres >> >> >> _______________________________________________ >> Xen-devel mailing list >> Xen-devel@lists.xensource.com >> http://lists.xensource.com/xen-devel > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel
On Thu, 2012-01-19 at 20:57 +0000, Andres Lagar-Cavilla wrote:> > On 19/01/2012 20:30, "Andres Lagar-Cavilla" <andres@lagarcavilla.org> > > wrote: > > > >> Hi, > >> I had the following painful experience. I declared > >> > >> struct xen_mem_event_op { > >> uint8_t op; /* XENMEM_*_op_* */ > >> domid_t domain; > >> uint64_t buffer; > >> uint64_t gfn; /* IN: gfn of page being operated on */ > >> }; > >> typedef struct xen_mem_event_op xen_mem_event_op_t; > >> > >> to be passed as the argument of a memory op called form the toolstack. > >> The > >> hypervisor is 64 bits and the toolstack is 32 bits. My toolstack code > >> simply: > >> > >> xen_mem_event_op_t meo; > >> ... set fields ... > >> return do_memory_op(xch, mode, &meo, sizeof(meo)); > >> > >> No joy because 32 bits was packing the struct differently than 64 bits. > >> Namely, both were adding a 1 byte pad between ''op'' and ''domain'', but > >> when > >> compiled in 64 bits mode for the hypervisor, an additional 4 byte pad > >> was > >> thrown between ''domain'' and ''buffer''. > >> > >> The first question is, what is the preferred way around this. Declare > >> pads > >> inside the struct? > > > > Yes. > > Ok. And as Konrad says, I''ll add __attribute__((packed)). A first for > memory.hI don''t think gcc extensions such as this are allowed in xen/include/public. You should explicitly pack the struct instead. Ian.> > Thanks! > Andres > > > > >> Exploring the include/public/memory.h declarations and toolstack code, I > >> see that no current declare includes __attribute__((aligned)) or > >> __attribute__((packed)), or explicit pads. > >> > >> So how come things don''t break more often for 32 bit toolstacks? pure > >> luck? Am I missing something? > > > > Where older structs were not 32/64-bit invariant, compat shims were > > implemented. See common/compat/memory.c, for example. Well worth avoiding > > that! > > > > -- Keir > > > >> Thanks! > >> Andres > >> > >> > >> _______________________________________________ > >> Xen-devel mailing list > >> Xen-devel@lists.xensource.com > >> http://lists.xensource.com/xen-devel > > > > > > > >
On Thu, 2012-01-19 at 21:07 +0000, Keir Fraser wrote:> > Yes. And also use __attribute(__packed__); > > Not used in any public header files.One seems to have crept into xen/include/public/arch-x86/hvm/save.h :-( Ian.
> On Thu, 2012-01-19 at 20:57 +0000, Andres Lagar-Cavilla wrote: >> > On 19/01/2012 20:30, "Andres Lagar-Cavilla" <andres@lagarcavilla.org> >> > wrote: >> > >> >> Hi, >> >> I had the following painful experience. I declared >> >> >> >> struct xen_mem_event_op { >> >> uint8_t op; /* XENMEM_*_op_* */ >> >> domid_t domain; >> >> uint64_t buffer; >> >> uint64_t gfn; /* IN: gfn of page being operated on */ >> >> }; >> >> typedef struct xen_mem_event_op xen_mem_event_op_t; >> >> >> >> to be passed as the argument of a memory op called form the >> toolstack. >> >> The >> >> hypervisor is 64 bits and the toolstack is 32 bits. My toolstack code >> >> simply: >> >> >> >> xen_mem_event_op_t meo; >> >> ... set fields ... >> >> return do_memory_op(xch, mode, &meo, sizeof(meo)); >> >> >> >> No joy because 32 bits was packing the struct differently than 64 >> bits. >> >> Namely, both were adding a 1 byte pad between ''op'' and ''domain'', but >> >> when >> >> compiled in 64 bits mode for the hypervisor, an additional 4 byte pad >> >> was >> >> thrown between ''domain'' and ''buffer''. >> >> >> >> The first question is, what is the preferred way around this. Declare >> >> pads >> >> inside the struct? >> > >> > Yes. >> >> Ok. And as Konrad says, I''ll add __attribute__((packed)). A first for >> memory.h > > I don''t think gcc extensions such as this are allowed in > xen/include/public. You should explicitly pack the struct instead.Ok, I''ll back off on ''packed''. It makes me uneasy though not being able to control the layout exactly. Thanks, Andres> > Ian. > >> >> Thanks! >> Andres >> >> > >> >> Exploring the include/public/memory.h declarations and toolstack >> code, I >> >> see that no current declare includes __attribute__((aligned)) or >> >> __attribute__((packed)), or explicit pads. >> >> >> >> So how come things don''t break more often for 32 bit toolstacks? pure >> >> luck? Am I missing something? >> > >> > Where older structs were not 32/64-bit invariant, compat shims were >> > implemented. See common/compat/memory.c, for example. Well worth >> avoiding >> > that! >> > >> > -- Keir >> > >> >> Thanks! >> >> Andres >> >> >> >> >> >> _______________________________________________ >> >> Xen-devel mailing list >> >> Xen-devel@lists.xensource.com >> >> http://lists.xensource.com/xen-devel >> > >> > >> > >> >> > > >
> On Thu, 2012-01-19 at 20:57 +0000, Andres Lagar-Cavilla wrote: >> > On 19/01/2012 20:30, "Andres Lagar-Cavilla" <andres@lagarcavilla.org> >> > wrote: >> > >> >> Hi, >> >> I had the following painful experience. I declared >> >> >> >> struct xen_mem_event_op { >> >> uint8_t op; /* XENMEM_*_op_* */ >> >> domid_t domain; >> >> uint64_t buffer; >> >> uint64_t gfn; /* IN: gfn of page being operated on */ >> >> }; >> >> typedef struct xen_mem_event_op xen_mem_event_op_t; >> >> >> >> to be passed as the argument of a memory op called form the >> toolstack. >> >> The >> >> hypervisor is 64 bits and the toolstack is 32 bits. My toolstack code >> >> simply: >> >> >> >> xen_mem_event_op_t meo; >> >> ... set fields ... >> >> return do_memory_op(xch, mode, &meo, sizeof(meo)); >> >> >> >> No joy because 32 bits was packing the struct differently than 64 >> bits. >> >> Namely, both were adding a 1 byte pad between ''op'' and ''domain'', but >> >> when >> >> compiled in 64 bits mode for the hypervisor, an additional 4 byte pad >> >> was >> >> thrown between ''domain'' and ''buffer''. >> >> >> >> The first question is, what is the preferred way around this. Declare >> >> pads >> >> inside the struct? >> > >> > Yes. >> >> Ok. And as Konrad says, I''ll add __attribute__((packed)). A first for >> memory.h > > I don''t think gcc extensions such as this are allowed in > xen/include/public. You should explicitly pack the struct instead.domctl.h is in a way spared, because __attribute__((aligned(8))) is allowed in 32 bits. And the header is spared the ansi test. Is there a rationale to allowing this ABI file do ''aligned'', but preventing that other header file from using it? I''m thinking uint64_aligned_t would solve my problem in memory.h. Andres> > Ian. > >> >> Thanks! >> Andres >> >> > >> >> Exploring the include/public/memory.h declarations and toolstack >> code, I >> >> see that no current declare includes __attribute__((aligned)) or >> >> __attribute__((packed)), or explicit pads. >> >> >> >> So how come things don''t break more often for 32 bit toolstacks? pure >> >> luck? Am I missing something? >> > >> > Where older structs were not 32/64-bit invariant, compat shims were >> > implemented. See common/compat/memory.c, for example. Well worth >> avoiding >> > that! >> > >> > -- Keir >> > >> >> Thanks! >> >> Andres >> >> >> >> >> >> _______________________________________________ >> >> Xen-devel mailing list >> >> Xen-devel@lists.xensource.com >> >> http://lists.xensource.com/xen-devel >> > >> > >> > >> >> > > >
On 19/01/2012 21:23, "Andres Lagar-Cavilla" <andres@lagarcavilla.org> wrote:>> >> I don''t think gcc extensions such as this are allowed in >> xen/include/public. You should explicitly pack the struct instead. > > domctl.h is in a way spared, because __attribute__((aligned(8))) is > allowed in 32 bits. And the header is spared the ansi test. > > Is there a rationale to allowing this ABI file do ''aligned'', but > preventing that other header file from using it? > > I''m thinking uint64_aligned_t would solve my problem in memory.h.Would like public headers to not be gcc specific. The toolstack is a more specific special case, it contains lots of gcc-isms anyway. Hence its sysctl/domctl hypercalls are allowed more leeway. Frankly, rather than hauling the mem_event toolstack operations out of domctl, you might be better just fixing the coarse-grained locking at least for the particular commands you care about. The big domctl lock is not needed for a quite a few of those domctl operations. -- Keir> Andres > >> >> Ian. >> >>> >>> Thanks! >>> Andres >>> >>>> >>>>> Exploring the include/public/memory.h declarations and toolstack >>> code, I >>>>> see that no current declare includes __attribute__((aligned)) or >>>>> __attribute__((packed)), or explicit pads. >>>>> >>>>> So how come things don''t break more often for 32 bit toolstacks? pure >>>>> luck? Am I missing something? >>>> >>>> Where older structs were not 32/64-bit invariant, compat shims were >>>> implemented. See common/compat/memory.c, for example. Well worth >>> avoiding >>>> that! >>>> >>>> -- Keir >>>> >>>>> Thanks! >>>>> Andres >>>>> >>>>> >>>>> _______________________________________________ >>>>> Xen-devel mailing list >>>>> Xen-devel@lists.xensource.com >>>>> http://lists.xensource.com/xen-devel >>>> >>>> >>>> >>> >>> >> >> >> > >
On 19/01/2012 21:12, "Ian Campbell" <Ian.Campbell@citrix.com> wrote:> On Thu, 2012-01-19 at 21:07 +0000, Keir Fraser wrote: >>> Yes. And also use __attribute(__packed__); >> >> Not used in any public header files. > > One seems to have crept into xen/include/public/arch-x86/hvm/save.h :-(Thanks, I''ll get rid of it. It looks like it should have no effect on that struct anyway. -- Keir> Ian. > >
On 19/01/2012 21:56, "Keir Fraser" <keir.xen@gmail.com> wrote:> On 19/01/2012 21:23, "Andres Lagar-Cavilla" <andres@lagarcavilla.org> wrote: > >>> >>> I don''t think gcc extensions such as this are allowed in >>> xen/include/public. You should explicitly pack the struct instead. >> >> domctl.h is in a way spared, because __attribute__((aligned(8))) is >> allowed in 32 bits. And the header is spared the ansi test. >> >> Is there a rationale to allowing this ABI file do ''aligned'', but >> preventing that other header file from using it? >> >> I''m thinking uint64_aligned_t would solve my problem in memory.h. > > Would like public headers to not be gcc specific. The toolstack is a more > specific special case, it contains lots of gcc-isms anyway. Hence its > sysctl/domctl hypercalls are allowed more leeway. > > Frankly, rather than hauling the mem_event toolstack operations out of > domctl, you might be better just fixing the coarse-grained locking at least > for the particular commands you care about. The big domctl lock is not > needed for a quite a few of those domctl operations.As an alternative, you could declare a tools-only section for public/memory.h. See public/hvm/hvm_op.h for example, which therefore gets to use uint64_aligned_t in those sections. If your struct is for general consumption by any guest then you''re SOL and have to do it the hard way. -- Keir> -- Keir > >> Andres >> >>> >>> Ian. >>> >>>> >>>> Thanks! >>>> Andres >>>> >>>>> >>>>>> Exploring the include/public/memory.h declarations and toolstack >>>> code, I >>>>>> see that no current declare includes __attribute__((aligned)) or >>>>>> __attribute__((packed)), or explicit pads. >>>>>> >>>>>> So how come things don''t break more often for 32 bit toolstacks? pure >>>>>> luck? Am I missing something? >>>>> >>>>> Where older structs were not 32/64-bit invariant, compat shims were >>>>> implemented. See common/compat/memory.c, for example. Well worth >>>> avoiding >>>>> that! >>>>> >>>>> -- Keir >>>>> >>>>>> Thanks! >>>>>> Andres >>>>>> >>>>>> >>>>>> _______________________________________________ >>>>>> Xen-devel mailing list >>>>>> Xen-devel@lists.xensource.com >>>>>> http://lists.xensource.com/xen-devel >>>>> >>>>> >>>>> >>>> >>>> >>> >>> >>> >> >> > >
On 19/01/2012 22:00, "Keir Fraser" <keir.xen@gmail.com> wrote:>> Would like public headers to not be gcc specific. The toolstack is a more >> specific special case, it contains lots of gcc-isms anyway. Hence its >> sysctl/domctl hypercalls are allowed more leeway. >> >> Frankly, rather than hauling the mem_event toolstack operations out of >> domctl, you might be better just fixing the coarse-grained locking at least >> for the particular commands you care about. The big domctl lock is not >> needed for a quite a few of those domctl operations. > > As an alternative, you could declare a tools-only section for > public/memory.h. See public/hvm/hvm_op.h for example, which therefore gets > to use uint64_aligned_t in those sections.Also, if you are adding toolstack commands to an existing general-purpose hypercall, wrapping it in defined(__XEN_TOOLS__) is useful documentation. -- Keir> If your struct is for general consumption by any guest then you''re SOL and > have to do it the hard way.
> On 19/01/2012 21:56, "Keir Fraser" <keir.xen@gmail.com> wrote: > >> On 19/01/2012 21:23, "Andres Lagar-Cavilla" <andres@lagarcavilla.org> >> wrote: >> >>>> >>>> I don''t think gcc extensions such as this are allowed in >>>> xen/include/public. You should explicitly pack the struct instead. >>> >>> domctl.h is in a way spared, because __attribute__((aligned(8))) is >>> allowed in 32 bits. And the header is spared the ansi test. >>> >>> Is there a rationale to allowing this ABI file do ''aligned'', but >>> preventing that other header file from using it? >>> >>> I''m thinking uint64_aligned_t would solve my problem in memory.h. >> >> Would like public headers to not be gcc specific. The toolstack is a >> more >> specific special case, it contains lots of gcc-isms anyway. Hence its >> sysctl/domctl hypercalls are allowed more leeway. >> >> Frankly, rather than hauling the mem_event toolstack operations out of >> domctl, you might be better just fixing the coarse-grained locking at >> least >> for the particular commands you care about. The big domctl lock is not >> needed for a quite a few of those domctl operations. > > As an alternative, you could declare a tools-only section for > public/memory.h. See public/hvm/hvm_op.h for example, which therefore gets > to use uint64_aligned_t in those sections.This sounds like the way to go. Luckily not for general consumption and not SOL :) Thanks! Andres> > If your struct is for general consumption by any guest then you''re SOL and > have to do it the hard way. > > -- Keir > >> -- Keir >> >>> Andres >>> >>>> >>>> Ian. >>>> >>>>> >>>>> Thanks! >>>>> Andres >>>>> >>>>>> >>>>>>> Exploring the include/public/memory.h declarations and toolstack >>>>> code, I >>>>>>> see that no current declare includes __attribute__((aligned)) or >>>>>>> __attribute__((packed)), or explicit pads. >>>>>>> >>>>>>> So how come things don''t break more often for 32 bit toolstacks? >>>>>>> pure >>>>>>> luck? Am I missing something? >>>>>> >>>>>> Where older structs were not 32/64-bit invariant, compat shims were >>>>>> implemented. See common/compat/memory.c, for example. Well worth >>>>> avoiding >>>>>> that! >>>>>> >>>>>> -- Keir >>>>>> >>>>>>> Thanks! >>>>>>> Andres >>>>>>> >>>>>>> >>>>>>> _______________________________________________ >>>>>>> Xen-devel mailing list >>>>>>> Xen-devel@lists.xensource.com >>>>>>> http://lists.xensource.com/xen-devel >>>>>> >>>>>> >>>>>> >>>>> >>>>> >>>> >>>> >>>> >>> >>> >> >> > > >
>>> On 19.01.12 at 22:56, Keir Fraser <keir.xen@gmail.com> wrote: > On 19/01/2012 21:12, "Ian Campbell" <Ian.Campbell@citrix.com> wrote: > >> On Thu, 2012-01-19 at 21:07 +0000, Keir Fraser wrote: >>>> Yes. And also use __attribute(__packed__); >>> >>> Not used in any public header files. >> >> One seems to have crept into xen/include/public/arch-x86/hvm/save.h :-( > > Thanks, I''ll get rid of it. It looks like it should have no effect on that > struct anyway.While it indeed looks pointless there, all the hvm/save.h headers are supposedly tools interface only anyway, so would generally be free to use extensions (and are therefore excluded from the ANSI correctness test). Jan
On 20/01/2012 10:12, "Jan Beulich" <JBeulich@suse.com> wrote:>>>> Not used in any public header files. >>> >>> One seems to have crept into xen/include/public/arch-x86/hvm/save.h :-( >> >> Thanks, I''ll get rid of it. It looks like it should have no effect on that >> struct anyway. > > While it indeed looks pointless there, all the hvm/save.h headers are > supposedly tools interface only anyway, so would generally be free > to use extensions (and are therefore excluded from the ANSI > correctness test).True. But since it is not needed and is the only one in the whole public/ directory, I got rid of it anyway. -- Keir
> > luck? Am I missing something? > > Where older structs were not 32/64-bit invariant, compat shims were > implemented. See common/compat/memory.c, for example. Well worth avoiding > that!So for the "Fun" of it I tried to see if the ''struct xen_processor_performance'' has some 32/64-bit issues and was surprised to find they do. What I am more surprised to find is that nobody seems to have had any troubles with this as it seems to have been there since it was initially implemented. The major issue would have been with the ''shared_type'', ''domain_info'' and the pointer to the ''states'' being at different offsets (So when running a 32-bit dom0 with a 64-bit hypervisor). The attached little C program has the identified issues and the // FIX is my attempt at making the structs of the same size on 32 and 64 bit builds. Right now when built as 32-bit the struct is 96 bytes, while on 64-bit it is 104. Was wondering what is the right fix? The thoughts I had was to either leave them as be and the domain would have to figure out whether the hypervisor is 32-bit or 64-bit and provide the _right_ structure. Or perhaps fix it and provide a "version" hypercall, but that would not be backwards compatible. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 22/01/2012 20:37, "Konrad Rzeszutek Wilk" <konrad@darnok.org> wrote:>>> luck? Am I missing something? >> >> Where older structs were not 32/64-bit invariant, compat shims were >> implemented. See common/compat/memory.c, for example. Well worth avoiding >> that! > > So for the "Fun" of it I tried to see if the ''struct > xen_processor_performance'' has some 32/64-bit issues and was surprised > to find they do. What I am more surprised to find is that nobody seems > to have had any troubles with this as it seems to have been there since > it was initially implemented. The major issue would have been with the > ''shared_type'', ''domain_info'' and the pointer to the ''states'' being at > different offsets (So when running a 32-bit dom0 with a 64-bit > hypervisor).It works because x86/64 Xen has a compat shim for that platform hypercall command, which it uses when the caller is a 32-bit guest. See xen/include/xlat.lst, xen/include/compat/platform.h (auto-generated), xen/arch/x86/x86_64/platform_hypercall.c. For more details, ask Jan -- he implemented most of it. ;-) -- Keir> The attached little C program has the identified issues and the > // FIX is my attempt at making the structs of the same size on 32 and 64 > bit builds. Right now when built as 32-bit the struct is 96 bytes, while on > 64-bit it is 104. > > Was wondering what is the right fix? The thoughts I had was to either > leave them as be and the domain would have to figure out whether the > hypervisor > is 32-bit or 64-bit and provide the _right_ structure. > > Or perhaps fix it and provide a "version" hypercall, but that would not > be backwards compatible.
>>> On 22.01.12 at 21:37, Konrad Rzeszutek Wilk <konrad@darnok.org> wrote: > So for the "Fun" of it I tried to see if the ''struct > xen_processor_performance'' has some 32/64-bit issues and was surprised > to find they do. What I am more surprised to find is that nobody seems > to have had any troubles with this as it seems to have been there since > it was initially implemented. The major issue would have been with the > ''shared_type'', ''domain_info'' and the pointer to the ''states'' being at > different offsets (So when running a 32-bit dom0 with a 64-bit > hypervisor).Are you having an actual problem with the layout differences, or did you just observe them? As Keir said, this is being dealt with inside the hypervisor - the (Dom0) kernel doesn''t need to care at all (which was the primary requirement when the 32-on-64 support got added). Jan
On Mon, Jan 23, 2012 at 09:24:29AM +0000, Jan Beulich wrote:> >>> On 22.01.12 at 21:37, Konrad Rzeszutek Wilk <konrad@darnok.org> wrote: > > So for the "Fun" of it I tried to see if the ''struct > > xen_processor_performance'' has some 32/64-bit issues and was surprised > > to find they do. What I am more surprised to find is that nobody seems > > to have had any troubles with this as it seems to have been there since > > it was initially implemented. The major issue would have been with the > > ''shared_type'', ''domain_info'' and the pointer to the ''states'' being at > > different offsets (So when running a 32-bit dom0 with a 64-bit > > hypervisor). > > Are you having an actual problem with the layout differences, or > did you just observe them? As Keir said, this is being dealt with > inside the hypervisor - the (Dom0) kernel doesn''t need to care at > all (which was the primary requirement when the 32-on-64 > support got added).I just observed them when I was trying to use memcpy on the Linux<->Xen structs and saw some padding issues. The further I looked the more I saw of it and was wondering why nobody had tripped over it. But the compat layer that Keir pointed to me does fix the 32/64 bit issue I observed - so wheew, no bugs! Thanks!> > Jan > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel