From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1ab8vl-00085p-Hm for mharc-grub-devel@gnu.org; Wed, 02 Mar 2016 10:43:17 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59922) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ab8vi-00084Z-8k for grub-devel@gnu.org; Wed, 02 Mar 2016 10:43:15 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ab8ve-00010X-7N for grub-devel@gnu.org; Wed, 02 Mar 2016 10:43:14 -0500 Received: from mx2.suse.de ([195.135.220.15]:45564) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ab8vd-00010B-U5 for grub-devel@gnu.org; Wed, 02 Mar 2016 10:43:10 -0500 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 9E53BAD7D; Wed, 2 Mar 2016 15:43:07 +0000 (UTC) Subject: Re: [PATCH v4 10/11] xen: modify page table construction To: Daniel Kiper References: <1456120999-5639-11-git-send-email-jgross@suse.com> <20160222091748.GO3482@olila.local.net-space.pl> <56CAD4E0.80304@suse.com> <20160222121858.GQ3482@olila.local.net-space.pl> <56CAFF66.1080709@suse.com> <20160222124805.GS3482@olila.local.net-space.pl> <56CB09AE.3040301@suse.com> <56CF4903.6020304@gmail.com> <56D40BB6.6090904@suse.com> <56D4374F.5050209@suse.com> <20160302091248.GH3500@olila.local.net-space.pl> From: Juergen Gross Message-ID: <56D70A0B.9000704@suse.com> Date: Wed, 2 Mar 2016 16:43:07 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: <20160302091248.GH3500@olila.local.net-space.pl> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x (no timestamps) [generic] X-Received-From: 195.135.220.15 Cc: The development of GNU GRUB , mchang@suse.com, xen-devel@lists.xen.org, phcoder@gmail.com X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list Reply-To: The development of GNU GRUB List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 02 Mar 2016 15:43:15 -0000 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 =D0=BF=D0=B8=D1=88=D0=B5=D1=82: >>>>> 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/xe= n/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 tw= o 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 l= ength >>>>>>> being a multiple of that alignment. Otherwise arrays wouldn't wor= k. >>>>>> >>>>>> 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 li= ke >>>>> "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 m= emory >>>>> 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 inte= rnal >>>> 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 "pac= ked" >>>>> 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 pad= ding >>>> 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 wron= g? >>> >>> 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 alway= s >> a multiple of the object=E2=80=98s alignment. >> >> I don't think any x86 C-compiler will violate the x86 ABI. >=20 > You just cited only part of paragraph. Here is full paragraph: >=20 > [...] >=20 > Aggregates and Unions >=20 > Structures and unions assume the alignment of their most strictly align= ed component. > Each member is assigned to the lowest available offset with the appropr= iate > alignment. The size of any object is always a multiple of the object=E2= =80=98s alignment. > An array uses the same alignment as its elements, except that a local o= r global > array variable of length at least 16 bytes or a C99 variable-length arr= ay variable > always has alignment of at least 16 bytes. > Structure and union objects can require padding to meet size and alignm= ent > constraints. The contents of any padding is undefined. >=20 > [...] >=20 > Well, this is a bit hard to understand, so, please look here > http://www.catb.org/esr/structure-packing/#_structure_alignment_and_pad= ding > what can happen if struct has members with different sizes and you do > not use packed attribute. >=20 > Luckily you use struct members with the same sizes, so, everything work= s. This wasn't luck, it was on purpose. ;-) > However, if you/somebody will try to change grub_relocator_xen_paging_a= rea > layout and add a member with different size in the middle or the beginn= ing > 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