From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH 04/18] xen/x86: add multiboot2 protocol support Date: Fri, 27 Mar 2015 11:20:10 +0000 Message-ID: <55154AFA020000780006E771__44536.2440812916$1427455263$gmane$org@mail.emea.novell.com> References: <1422640462-28103-1-git-send-email-daniel.kiper@oracle.com> <1422640462-28103-5-git-send-email-daniel.kiper@oracle.com> <54E76992020000780006222F@mail.emea.novell.com> <20150327105636.GE8294@olila.local.net-space.pl> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1YbSJC-0000zc-1V for xen-devel@lists.xenproject.org; Fri, 27 Mar 2015 11:20:14 +0000 In-Reply-To: <20150327105636.GE8294@olila.local.net-space.pl> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Daniel Kiper Cc: Juergen Gross , grub-devel@gnu.org, keir@xen.org, ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com, andrew.cooper3@citrix.com, roy.franz@linaro.org, ning.sun@intel.com, david.vrabel@citrix.com, phcoder@gmail.com, xen-devel@lists.xenproject.org, qiaowei.ren@intel.com, richard.l.maliszewski@intel.com, gang.wei@intel.com, fu.wei@linaro.org List-Id: xen-devel@lists.xenproject.org >>> On 27.03.15 at 11:56, wrote: > On Fri, Feb 20, 2015 at 04:06:26PM +0000, Jan Beulich wrote: >> >>> On 30.01.15 at 18:54, wrote: >> > --- a/xen/arch/x86/boot/Makefile >> > +++ b/xen/arch/x86/boot/Makefile >> > @@ -1,6 +1,7 @@ >> > obj-bin-y += head.o >> > >> > -RELOC_DEPS = $(BASEDIR)/include/asm-x86/config.h $(BASEDIR)/include/xen/multiboot.h >> > +RELOC_DEPS = $(BASEDIR)/include/asm-x86/config.h $(BASEDIR)/include/xen/compiler.h \ >> > + $(BASEDIR)/include/xen/multiboot.h $(BASEDIR)/include/xen/multiboot2.h >> >> Perhaps we should rather have the compiler generate the >> dependencies for us when compiling reloc.o? > > Hmmm... I am a bit confused. Here > http://lists.xen.org/archives/html/xen-devel/2014-10/msg02094.html > you told a bit different thing and relevant patch was accepted as commit > c42070df66c9fcedf477959b8371b85aa4ac4933 > (x86/boot: fix reloc.S build dependencies). I can try to do what you > suggest, this is not a problem > for me, however, I would like to be sure what is your final decision in that > case. First of all I wasn't anticipating that this list of dependencies would further grow. And then I didn't say "don't do this", I only pointed out (albeit maybe a little too implicitly) that this would be a more complex patch. >> > + .long MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO >> > + .long MULTIBOOT2_TAG_TYPE_MMAP >> > +.Lmultiboot2_info_req_end: >> > + >> > + .align MULTIBOOT2_TAG_ALIGN >> > + .short MULTIBOOT2_HEADER_TAG_MODULE_ALIGN >> > + .short MULTIBOOT2_HEADER_TAG_REQUIRED >> > + .long 8 /* Tag size. */ >> >> ... here and further down you hard-code it. Please be consistent >> (and if you go the calculation route, perhaps introduce some >> redundancy reducing macro). > > I did this deliberately. I calculate size using labels when it is really > needed, e.g. in tags which size depends on number of elements. However, > most tags have strictly defined sizes. So, that is why I dropped labels > and entered simple numbers. Though I agree that it can be improved. > I think that we can define relevant tag structures in multiboot2.h and > generate relevant constants using asm-offsets.c. Is it OK for you? That would mean exposing stuff to other parts of the tree which doesn't need to be exposed. I don't see why you can't just do proper size calculations here. >> > @@ -81,10 +135,51 @@ __start: >> > mov %ecx,%es >> > mov %ecx,%ss >> > >> > + /* Bootloaders may set multiboot[12].mem_lower to a nonzero value */ >> >> Above you meet coding style requirements, but here you stop to do >> so (ongoing below)? Even if pre-existing neighboring comments aren't >> well formed, please don't make the situation worse. > > Do you ask about ending fullstops? Am I right? Yes. >> > @@ -31,7 +38,16 @@ asm ( >> > ); >> > >> > typedef unsigned int u32; >> > +typedef unsigned long long u64; >> > + >> > +#include "../../../include/xen/compiler.h" >> >> Why? > > static multiboot_info_t __used *reloc(void *mbi_in, u32 mb_magic) > Because of this ------> ^^^^^^ And what keeps you from leaving out the "static", making the __used unnecessary? >> > + >> > +typedef struct { >> > + u32 type; >> > + u32 size; >> > + u32 mem_lower; >> > + u32 mem_upper; >> > +} multiboot2_tag_basic_meminfo_t; >> > + >> > +typedef struct __packed { >> >> Why __packed when all the others aren't? > > Ha! This thing was taken from GRUB2 source. > I was asking this question myself many times. > I have not found real explanation for this > but if you wish I can dive into code and > try to find one (if it exists). Or even better just drop the __packed. Jan