From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Kiper Subject: Re: [PATCH v2 5/5] xen/x86: Add multiboot2 protocol support Date: Thu, 25 Sep 2014 20:42:16 +0200 Message-ID: <20140925184216.GW3459@olila.local.net-space.pl> References: <1411579162-27503-1-git-send-email-daniel.kiper@oracle.com> <1411579162-27503-6-git-send-email-daniel.kiper@oracle.com> <542317FB.5070905@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1XXE01-0001k2-I6 for xen-devel@lists.xenproject.org; Thu, 25 Sep 2014 18:42:41 +0000 Content-Disposition: inline In-Reply-To: <542317FB.5070905@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Andrew Cooper Cc: keir@xen.org, ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com, ross.philipson@citrix.com, roy.franz@linaro.org, ning.sun@intel.com, jbeulich@suse.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 Wed, Sep 24, 2014 at 08:14:03PM +0100, Andrew Cooper wrote: > On 24/09/14 18:19, Daniel Kiper wrote: > > Add multiboot2 protocol support. This way we are laying the foundation > > for EFI + GRUB2 + Xen development. > > > > Signed-off-by: Daniel Kiper > > Much clearer than before. > > Can I suggest retroactively renaming the old multiboot bits to Do you mean before or after this (rewritten) patch? > multiboot1 to make a more marked difference between between > "multiboot1_proto" and "multiboot2_proto" ? Do you think about labels in this file only or about all stuff related to multiboot(1) protocol? > It would also be better to split the changing of multiboot1 and > shuffling of memory calculations away from the introduction of multiboot2. > > > --- > > v2 - suggestions/fixes: > > - use "for" instead of "while" for loops > > (suggested by Jan Beulich), > > - properly parenthesize macro arguments > > > > char *boot_loader_name; > > (suggested by Jan Beulich), > > - change some local variables types > > (suggested by Jan Beulich), > > - use meaningful labels > > (suggested by Andrew Cooper and Jan Beulich), > > - use local labels > > (suggested by Jan Beulich), > > - fix coding style > > (suggested by Jan Beulich), > > - patch split rearrangement > > (suggested by Andrew Cooper and Jan Beulich). > > --- > > xen/arch/x86/boot/head.S | 117 +++++++++++++- > > xen/arch/x86/boot/reloc.c | 103 +++++++++++- > > xen/include/xen/multiboot2.h | 354 ++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 567 insertions(+), 7 deletions(-) > > create mode 100644 xen/include/xen/multiboot2.h > > > > diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S > > index 7e48833..4331821 100644 > > --- a/xen/arch/x86/boot/head.S > > +++ b/xen/arch/x86/boot/head.S [...] > > @@ -81,10 +144,56 @@ __start: > > mov %ecx,%es > > mov %ecx,%ss > > > > + /* Assume multiboot[12].mem_lower is 0 if not set by bootloader */ > > + xor %edx,%edx > > + > > This change does not match its comment, and seems to have appeared from > nowhere. I still do not understand what is wrong with that. We need to set %edx to known value here (0 in this case). If MBI_MEMLIMITS or MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO is not set by bootloader and we do not initialize %edx then we will use uninitialized value later. > > /* Check for Multiboot bootloader */ > > cmp $MULTIBOOT_BOOTLOADER_MAGIC,%eax > > - jne not_multiboot > > + je multiboot_proto > > + > > + /* Check for Multiboot2 bootloader */ > > + cmp $MULTIBOOT2_BOOTLOADER_MAGIC,%eax > > + je multiboot2_proto > > + > > + jmp not_multiboot > > + > > +multiboot_proto: > > + /* Get mem_lower from Multiboot information */ > > + testb $MBI_MEMLIMITS,(%ebx) > > + jz trampoline_setup /* not available? BDA value will be fine */ > > + > > + mov 4(%ebx),%edx > > + shl $10-4,%edx > > + jmp trampoline_setup > > > > +multiboot2_proto: > > + /* Get Multiboot2 information address */ > > + mov %ebx,%ecx > > + > > + /* Skip Multiboot2 information fixed part */ > > + add $8,%ecx > > + > > +0: > > + /* Get mem_lower from Multiboot2 information */ > > + cmpl $MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO,(%ecx) > > + jne 1f > > + > > + mov 8(%ecx),%edx > > + shl $10-4,%edx > > + jmp trampoline_setup > > + > > +1: > > + /* Is it the end of Multiboot2 information? */ > > + cmpl $MULTIBOOT2_TAG_TYPE_END,(%ecx) > > + je trampoline_setup > > + > > + /* Go to next Multiboot2 information tag */ > > + add 4(%ecx),%ecx > > + add $(MULTIBOOT2_TAG_ALIGN-1),%ecx > > + and $~(MULTIBOOT2_TAG_ALIGN-1),%ecx > > + jmp 0b > > + > > +trampoline_setup: > > /* Set up trampoline segment 64k below EBDA */ > > movzwl 0x40e,%ecx /* EBDA segment */ > > cmp $0xa000,%ecx /* sanity check (high) */ > > @@ -99,12 +208,8 @@ __start: > > * Compare the value in the BDA with the information from the > > * multiboot structure (if available) and use the smallest. > > */ > > - testb $MBI_MEMLIMITS,(%ebx) > > - jz 2f /* not available? BDA value will be fine */ > > - mov 4(%ebx),%edx > > - cmp $0x100,%edx /* is the multiboot value too small? */ > > + cmp $0x4000,%edx /* is the multiboot value too small? */ > > You are going to need some justification for changing this lower bound > of memory. It is not changed. I did "shl $10-4,%edx" above so I must change relevant value here too. However, it looks that it left from my earlier work and I am able to remove that change safely. [...] > > diff --git a/xen/include/xen/multiboot2.h b/xen/include/xen/multiboot2.h > > This multiboot2.h is huge - can we get away with only including struct > definitions for tags we currently use/support? I did that because multiboot.h contains all multiboot(1) related stuff. I think that it is nice to have all defs for multiboot2 protocol too, however, I do not insist on that. Daniel