Re: [Xen-devel] [PATCH v4 10/11] xen: modify page table construction

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Wed, Mar 02, 2016 at 04:43:07PM +0100, Juergen Gross wrote:
> On 02/03/16 10:12, Daniel Kiper wrote:
> > On Mon, Feb 29, 2016 at 01:19:27PM +0100, Juergen Gross wrote:
> >> On 29/02/16 10:13, Juergen Gross wrote:
> >>> On 25/02/16 19:33, Andrei Borzenkov wrote:
> >>>> 22.02.2016 16:14, Juergen Gross пишет:
> >>>>> On 22/02/16 13:48, Daniel Kiper wrote:
> >>>>>> On Mon, Feb 22, 2016 at 01:30:30PM +0100, Juergen Gross wrote:
> >>>>>>> On 22/02/16 13:18, Daniel Kiper wrote:
> >>>>>>>> On Mon, Feb 22, 2016 at 10:29:04AM +0100, Juergen Gross wrote:
> >>>>>>>>> On 22/02/16 10:17, Daniel Kiper wrote:
> >>>>>>>>>> On Mon, Feb 22, 2016 at 07:03:18AM +0100, Juergen Gross wrote:
> >>>>>>>>>>> diff --git a/grub-core/lib/xen/relocator.c 
> >>>>>>>>>>> b/grub-core/lib/xen/relocator.c
> >>>>>>>>>>> index 8f427d3..a05b253 100644
> >>>>>>>>>>> --- a/grub-core/lib/xen/relocator.c
> >>>>>>>>>>> +++ b/grub-core/lib/xen/relocator.c
> >>>>>>>>>>> @@ -29,6 +29,11 @@
> >>>>>>>>>>>
> >>>>>>>>>>>  typedef grub_addr_t grub_xen_reg_t;
> >>>>>>>>>>>
> >>>>>>>>>>> +struct grub_relocator_xen_paging_area {
> >>>>>>>>>>> +  grub_xen_reg_t start;
> >>>>>>>>>>> +  grub_xen_reg_t size;
> >>>>>>>>>>> +};
> >>>>>>>>>>> +
> >>>>>>>>>>
> >>>>>>>>>> ... this should have GRUB_PACKED because compiler may
> >>>>>>>>>> add padding to align size member.
> >>>>>>>>>
> >>>>>>>>> Why would the compiler add padding to a structure containing two 
> >>>>>>>>> items
> >>>>>>>>> of the same type? I don't think the C standard would allow this.
> >>>>>>>>>
> >>>>>>>>> grub_xen_reg_t is either unsigned (32 bit) or unsigned long (64 
> >>>>>>>>> bit).
> >>>>>>>>> There is no way this could require any padding.
> >>>>>>>>
> >>>>>>>> You are right but we should add this here just in case.
> >>>>>>>
> >>>>>>> Sorry, I don't think this makes any sense. The C standard is very 
> >>>>>>> clear
> >>>>>>> in this case: a type requiring a special alignment has always a length
> >>>>>>> being a multiple of that alignment. Otherwise arrays wouldn't work.
> >>>>>>
> >>>>>> Sorry, I am not sure what do you mean by that.
> >>>>>
> >>>>> The size of any C type (no matter whether it is an integral type like
> >>>>> "int" or a structure) has always the same alignment restriction as the
> >>>>> type itself. So a type requiring 8 byte alignment will always have a
> >>>>> size of a multiple of 8 bytes. This is mandatory for arrays to work, as
> >>>>> otherwise either the elements wouldn't be placed consecutively in memory
> >>>>> or the alignment restrictions wouldn't be obeyed for all elements.
> >>>>>
> >>>>
> >>>> I too not follow how it is relevant to this case. We talk about internal
> >>>> padding between structure members, not between array elements.
> >>>>
> >>>>> For our case it means that two structure elements of the same type will
> >>>>> never require a padding between them, thus the annotation with "packed"
> >>>>> can't serve any purpose.
> >>>>>
> >>>>
> >>>> Well, I am not aware of any requirement. Compiler may add arbitrary
> >>>> padding between structure elements; it is only prohibited to add padding
> >>>> at the beginning. Sure, it would be unusual, but never say "never" ...
> >>>> also should Xen ever be ported to architecture where types are not
> >>>> self-aligned it will become an issue.
> >>>
> >>> So you are telling me that _all_ interfaces between e.g. Linux, grub2,
> >>> Xen and all wire protocols not attributed with "packed" are just wrong?
> >>>
> >>> Sorry, I don't think this is true.
> >>
> >> Okay, just found a reference: The x86 ABI states:
> >>
> >> Aggregates and Unions
> >> ---------------------
> >> Structures and unions assume the alignment of their most strictly
> >> aligned component. Each member is assigned to the lowest available
> >> offset with the appropriate alignment. The size of any object is always
> >> a multiple of the object‘s alignment.
> >>
> >> I don't think any x86 C-compiler will violate the x86 ABI.
> >
> > You just cited only part of paragraph. Here is full paragraph:
> >
> > [...]
> >
> > Aggregates and Unions
> >
> > Structures and unions assume the alignment of their most strictly aligned 
> > component.
> > Each member is assigned to the lowest available offset with the appropriate
> > alignment. The size of any object is always a multiple of the object‘s 
> > alignment.
> > An array uses the same alignment as its elements, except that a local or 
> > global
> > array variable of length at least 16 bytes or a C99 variable-length array 
> > variable
> > always has alignment of at least 16 bytes.
> > Structure and union objects can require padding to meet size and alignment
> > constraints. The contents of any padding is undefined.
> >
> > [...]
> >
> > Well, this is a bit hard to understand, so, please look here
> > http://www.catb.org/esr/structure-packing/#_structure_alignment_and_padding
> > what can happen if struct has members with different sizes and you do
> > not use packed attribute.
> >
> > Luckily you use struct members with the same sizes, so, everything works.
>
> This wasn't luck, it was on purpose. ;-)
>
> > However, if you/somebody will try to change grub_relocator_xen_paging_area
> > layout and add a member with different size in the middle or the beginning
> > of struct then suddenly everything will stop working. So, I think that in
>
> Of course. It doesn't matter whether the sizes are different or where
> the new item is introduced. Every change of this structure needs to be
> reflected in the assembly file.
>
> > cases when you create an interface between C and assembly using struct you
> > should use packed attribute (or separate variables if it makes sense). Even
> > if it works without it right now. Please do it to save your and others time
> > for more useful things than debugging issues like lack of packed attr.
>
> I'm tired of this discussion. I'll add the packed attribute if you are
> insisting on it, even if I still don't see the need.
>
> > Additionally, I still think that you do not need alignment before
> > grub_relocator_xen_paging_area struct struct in assembly file.
>
> I don't need it. It's just "best practice" (why does the compiler
> align variables?). I'll drop those as well, just to get the patch in.

Thanks a lot!

Daniel

PS Shortly I will send my GRUB2 patch series, so, you will have a joy of 
reviewing... :-)))

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