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

[Juergen Gross <jgross@suse.com>]

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.


Juergen