From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH RFC 5/7] xen/x86: Add multiboot2 protocol support Date: Tue, 9 Sep 2014 14:43:13 +0100 Message-ID: <540F03F1.40705@citrix.com> References: <1407539046-16910-1-git-send-email-daniel.kiper@oracle.com> <1407539046-16910-6-git-send-email-daniel.kiper@oracle.com> <53E7AAAB.7020706@citrix.com> <20140909133444.GQ3459@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.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1XRLha-00077b-Ll for xen-devel@lists.xenproject.org; Tue, 09 Sep 2014 13:43:22 +0000 In-Reply-To: <20140909133444.GQ3459@olila.local.net-space.pl> 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: keir@xen.org, ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com, roy.franz@linaro.org, ning.sun@intel.com, jbeulich@suse.com, ross.philipson@citrix.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 09/09/14 14:34, Daniel Kiper wrote: > On Sun, Aug 10, 2014 at 06:23:55PM +0100, Andrew Cooper wrote: >> On 09/08/14 00:04, Daniel Kiper wrote: >>> Add multiboot2 protocol support. This way we are laying the foundation >>> for EFI + GRUB2 + Xen development. >>> >>> Signed-off-by: Daniel Kiper >>> --- >>> xen/arch/x86/boot/head.S | 112 ++++++++++++++++++++++++++++++++++++++++++++- >>> xen/arch/x86/boot/reloc.c | 103 ++++++++++++++++++++++++++++++++++++++++- >>> 2 files changed, 212 insertions(+), 3 deletions(-) >>> >>> diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S > [...] > >>> @@ -81,10 +144,55 @@ __start: >>> mov %ecx,%es >>> mov %ecx,%ss >>> >>> + /* Set mem_lower to 0 */ >>> + xor %edx,%edx >>> + >> How does this affect mem_lower? it doesn't appear relevant in context. > %edx is used to store mem_lower value from multiboot[12] protocol. It must > have sensible value even if multiboot loader do not pass mem_lower value. > That is why %edx is zeroed here. > >>> /* Check for Multiboot bootloader */ >>> - cmp $0x2BADB002,%eax >>> - jne not_multiboot >>> + cmp $MULTIBOOT_BOOTLOADER_MAGIC,%eax >>> + je 1f >> This $MULTIBOOT_BOOTLOADER_MAGIC should probably be separate, as there >> are other bits of code which should use multiboot manifest constants. > You mean? A cleanup patch changing $0x2BADB002 -> $MULTIBOOT_BOOTLOADER_MAGIC should be separate from a functional change of adding multiboot 2 support. There are other places where you have mixed cleanup with adding new functionality. Cleanup is fine, but it *must* be separate from functional changes. Reviewing asm code is hard enough without having to work out whether the change is functional or cleanup, or a mix of the two. ~Andrew