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

[Juergen Gross <jgross@xxxxxxxx>]

On 01/03/16 04:52, Andrei Borzenkov wrote:
> 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?

http://www.x86-64.org/documentation/abi.pdf
Chapter 3.1.2, page 13

Juergen

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