From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1aaJtX-0006tC-9Q for mharc-grub-devel@gnu.org; Mon, 29 Feb 2016 04:13:35 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46927) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aaJtU-0006sl-PY for grub-devel@gnu.org; Mon, 29 Feb 2016 04:13:34 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aaJtR-00054O-G2 for grub-devel@gnu.org; Mon, 29 Feb 2016 04:13:32 -0500 Received: from mx2.suse.de ([195.135.220.15]:45147) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aaJtR-00054K-4w for grub-devel@gnu.org; Mon, 29 Feb 2016 04:13:29 -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 1AFC2AC5B; Mon, 29 Feb 2016 09:13:28 +0000 (UTC) Subject: Re: [PATCH v4 10/11] xen: modify page table construction To: The development of GNU GRUB , Daniel Kiper References: <1456120999-5639-1-git-send-email-jgross@suse.com> <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> From: Juergen Gross Message-ID: <56D40BB6.6090904@suse.com> Date: Mon, 29 Feb 2016 10:13:26 +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: <56CF4903.6020304@gmail.com> 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: phcoder@gmail.com, mchang@suse.com, xen-devel@lists.xen.org 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: Mon, 29 Feb 2016 09:13:34 -0000 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/xen/r= elocator.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 i= tems >>>>>> 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 bi= t). >>>>>> 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 cl= ear >>>> in this case: a type requiring a special alignment has always a leng= th >>>> 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, a= s >> otherwise either the elements wouldn't be placed consecutively in memo= ry >> or the alignment restrictions wouldn't be obeyed for all elements. >> >=20 > I too not follow how it is relevant to this case. We talk about interna= l > padding between structure members, not between array elements. >=20 >> For our case it means that two structure elements of the same type wil= l >> never require a padding between them, thus the annotation with "packed= " >> can't serve any purpose. >> >=20 > Well, I am not aware of any requirement. Compiler may add arbitrary > padding between structure elements; it is only prohibited to add paddin= g > 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 lon= ger. >> >=20 > It seems inconsistent through the code, really. But I think in this cas= e > 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