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

[Andrei Borzenkov]

29.02.2016 15:19, Juergen Gross пишет:
> 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.
> 

Thank you! I was not really objecting, more thinking loud, because I
missed such explicit statemrnt. Could you post link?