memop struct packing, 32/64 bits

memop struct packing, 32/64 bits

[Andres Lagar-Cavilla]

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

Re: memop struct packing, 32/64 bits

[Keir Fraser]

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

Re: memop struct packing, 32/64 bits

[Konrad Rzeszutek Wilk]

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

Re: memop struct packing, 32/64 bits

[Andres Lagar-Cavilla]

> 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
>
>
>

Re: memop struct packing, 32/64 bits

[Keir Fraser]

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

Re: memop struct packing, 32/64 bits

[Ian Campbell]

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.

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
> >
> >
> >
> 
> 

Re: memop struct packing, 32/64 bits

[Ian Campbell]

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.

Re: memop struct packing, 32/64 bits

[Andres Lagar-Cavilla]

> 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
>> >
>> >
>> >
>>
>>
>
>
>

Re: memop struct packing, 32/64 bits

[Andres Lagar-Cavilla]

> 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
>> >
>> >
>> >
>>
>>
>
>
>

Re: memop struct packing, 32/64 bits

[Keir Fraser]

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
>>>> 
>>>> 
>>>> 
>>> 
>>> 
>> 
>> 
>> 
> 
> 

Re: memop struct packing, 32/64 bits

[Keir Fraser]

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.
> 
> 

Re: memop struct packing, 32/64 bits

[Keir Fraser]

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
>>>>> 
>>>>> 
>>>>> 
>>>> 
>>>> 
>>> 
>>> 
>>> 
>> 
>> 
> 
> 

Re: memop struct packing, 32/64 bits

[Keir Fraser]

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.

Re: memop struct packing, 32/64 bits

[Andres Lagar-Cavilla]

> 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
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>
>>
>
>
>

Re: memop struct packing, 32/64 bits

[Jan Beulich]

>>> 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

Re: memop struct packing, 32/64 bits

[Keir Fraser]

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

Re: memop struct packing, 32/64 bits

[Konrad Rzeszutek Wilk]

[-- Attachment #1: Type: text/plain, Size: 1212 bytes --]

> > 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.

[-- Attachment #2: psd.c --]
[-- Type: text/x-csrc, Size: 4718 bytes --]


#include <stdio.h>
#include <stdint.h>
#include <stddef.h>

/* Turn this on/off to see how it would look like on 32 or 64 bit */
#define CONFIG_X86_64

#ifdef CONFIG_X86_64
#else
#pragma pack(4)
#endif

#define __DEFINE_GUEST_HANDLE(name, type) \
    typedef struct { type *p; } __guest_handle_ ## name

#define DEFINE_GUEST_HANDLE_STRUCT(name) \
	__DEFINE_GUEST_HANDLE(name, struct name)
#define DEFINE_GUEST_HANDLE(name) __DEFINE_GUEST_HANDLE(name, name)
#define GUEST_HANDLE(name)        __guest_handle_ ## name

struct xen_power_register {
	uint32_t     space_id;
	uint32_t     bit_width;
	uint32_t     bit_offset;
	uint32_t     access_size;
	uint64_t     address;
};

struct xen_processor_csd {
	uint32_t    domain;      /* domain number of one dependent group */
	uint32_t    coord_type;  /* coordination type */
	uint32_t    num;         /* number of processors in same domain */
};
DEFINE_GUEST_HANDLE_STRUCT(xen_processor_csd);

struct xen_processor_cx {
	struct xen_power_register  reg; /* GAS for Cx trigger register */
	uint8_t     type;     /* cstate value, c0: 0, c1: 1, ... */
	// FIX: Mega pad. No 64/32 bit problems.
	uint8_t		_pad1[3];
	uint32_t    latency;  /* worst latency (ms) to enter/exit this cstate */
	uint32_t    power;    /* average power consumption(mW) */
	uint32_t    dpcnt;    /* number of dependency entries */
	GUEST_HANDLE(xen_processor_csd) dp; /* NULL if no dependency */
};
DEFINE_GUEST_HANDLE_STRUCT(xen_processor_cx);

struct xen_processor_flags {
	uint32_t bm_control:1;
	uint32_t bm_check:1;
	uint32_t has_cst:1;
	uint32_t power_setup_done:1;
	uint32_t bm_rld_set:1;
};

struct xen_processor_power {
	uint32_t count;  /* number of C state entries in array below */
	struct xen_processor_flags flags;  /* global flags of this processor */
	GUEST_HANDLE(xen_processor_cx) states; /* supported c states */
};

struct xen_pct_register {
	uint8_t  descriptor;
	// FIX: Do not memcpy the (acpi_pct_register) as its packing is different */
	uint8_t	_pad1; /* ARGH */
	uint16_t length;
	uint8_t  space_id;
	uint8_t  bit_width;
	uint8_t  bit_offset;
	uint8_t  reserved;
	uint64_t address;
} __attribute__((__packed__));

struct xen_processor_px {
	uint64_t core_frequency; /* megahertz */
	uint64_t power;      /* milliWatts */
	uint64_t transition_latency; /* microseconds */
	uint64_t bus_master_latency; /* microseconds */
	uint64_t control;        /* control value */
	uint64_t status;     /* success indicator */
};
DEFINE_GUEST_HANDLE_STRUCT(xen_processor_px);

struct xen_psd_package {
	uint64_t num_entries;
	uint64_t revision;
	uint64_t domain;
	uint64_t coord_type;
	uint64_t num_processors;
}

struct xen_processor_performance {
	uint32_t flags;     /* flag for Px sub info type */
	uint32_t platform_limit;  /* Platform limitation on freq usage */
	struct xen_pct_register control_register;
	struct xen_pct_register status_register;
	uint32_t state_count;     /* total available performance states */
#ifndef CONFIG_X86_64
	// FIX
	uint32_t pad1;
#endif
	GUEST_HANDLE(xen_processor_px) states;
	struct xen_psd_package domain_info;
	uint32_t shared_type;     /* coordination type of this processor */
#ifndef CONFIG_X86_64
	// FIX
	uint32_t pad2;
#endif
};
int main(void) {
	printf("%d==104|96  %d==16  %d==40 %d==16 %d==48\n",
		sizeof(struct xen_processor_performance), /* 104 when 64, 96 when 32 */
		sizeof(struct xen_pct_register),
		sizeof(struct xen_psd_package),
		sizeof(struct xen_processor_power),
		sizeof(struct xen_processor_cx));

	printf("%d==0, %d==4, %d==10,%d==26 %d==40\n",
		offsetof(struct xen_processor_performance, flags),
		offsetof(struct xen_processor_performance, platform_limit),
		offsetof(struct xen_processor_performance, control_register.length),
		offsetof(struct xen_processor_performance, status_register.length),
		offsetof(struct xen_processor_performance, state_count));

	printf("%d==48|44, %d==56|52 %d==96|92\n",
		offsetof(struct xen_processor_performance, states),
		offsetof(struct xen_processor_performance, domain_info),
		offsetof(struct xen_processor_performance, shared_type));
	printf("%d==4, %d==8\n",
		offsetof(struct xen_processor_power, flags),
		offsetof(struct xen_processor_power, states));

	printf("%d==0 %d==0 %d==4 %d==16 %d==24 %d==28 %d==32 %d==36\n",
		offsetof(struct xen_processor_cx, reg),
		offsetof(struct xen_processor_cx, reg.space_id),
		offsetof(struct xen_processor_cx, reg.bit_width),
		offsetof(struct xen_processor_cx, reg.address),
		offsetof(struct xen_processor_cx, type), // 16
		offsetof(struct xen_processor_cx, latency), // 24
		offsetof(struct xen_processor_cx, power), // 28
		offsetof(struct xen_processor_cx, dpcnt), //32
		offsetof(struct xen_processor_cx, dp)); //36
	return 0;
}

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: memop struct packing, 32/64 bits

[Keir Fraser]

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.

Re: memop struct packing, 32/64 bits

[Jan Beulich]

>>> 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

Re: memop struct packing, 32/64 bits

[Konrad Rzeszutek Wilk]

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