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

[Juergen Gross <jgross@suse.com>]

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.

And before starting this weird attributing I'd like to see e.g. the
structures in multiboot.h and multiboot2.h to be updated according to
this philosophy.

BTW: Linux kernel isn't using "packed" for network protocols and other
structures where the same reasoning would apply.

>>>> Adding GRUB_PACKED would make the code less clear IMO. Finding such a
>>>> qualifier in some code I want to modify would make me search for the
>>>> reason for it which isn't existing.
>>>
>>> If maintainers do not object I am not going to insist on that any longer.
>>
> 
> It seems inconsistent through the code, really. But I think in this case
> it should be packed. It does not look like it is performance critical
> and it ensures we always match assembler code.

In case you still insist on it, I'll change it. But I'm far from
convinced this is a good move.


Juergen