Re: [Xen-devel] [PATCH v4] x86: relocate pvh_info

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Mon, Jan 22, 2018 at 06:19:43PM +0000, Andrew Cooper wrote:
> On 22/01/18 18:17, Wei Liu wrote:
> > On Mon, Jan 22, 2018 at 06:09:14PM +0000, Andrew Cooper wrote:
> >> On 22/01/18 16:13, Wei Liu wrote:
> >>> Modify early boot code to relocate pvh info as well, so that we can be
> >>> sure __va in __start_xen works.
> >>>
> >>> Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> >>> ---
> >>> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> >>> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> >>> Cc: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> >>> Cc: Doug Goldstein <cardoe@xxxxxxxxxx>
> >>>
> >>> v4: inlcude autoconf.h directly. The code itself is unchanged.
> >>> ---
> >>>  xen/arch/x86/boot/Makefile |  4 +++
> >>>  xen/arch/x86/boot/defs.h   |  3 +++
> >>>  xen/arch/x86/boot/head.S   | 25 ++++++++++--------
> >>>  xen/arch/x86/boot/reloc.c  | 64 
> >>> +++++++++++++++++++++++++++++++++++++++++-----
> >>>  4 files changed, 78 insertions(+), 18 deletions(-)
> >>>
> >>> diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
> >>> index c6246c85d2..1b3f121a2f 100644
> >>> --- a/xen/arch/x86/boot/Makefile
> >>> +++ b/xen/arch/x86/boot/Makefile
> >>> @@ -7,6 +7,10 @@ CMDLINE_DEPS = $(DEFS_H_DEPS) video.h
> >>>  RELOC_DEPS = $(DEFS_H_DEPS) $(BASEDIR)/include/xen/multiboot.h \
> >>>        $(BASEDIR)/include/xen/multiboot2.h
> >> + autoconf.h
> >>
> >> However, it would much better to take xen/kconfig.h ...
> >>
> > This is fine by me.
> >
> >>>  
> >>> +ifeq ($(CONFIG_PVH_GUEST),y)
> >>> +RELOC_DEPS += $(BASEDIR)/include/public/arch-x86/hvm/start_info.h
> >>> +endif
> > [...]
> >>> diff --git a/xen/arch/x86/boot/reloc.c b/xen/arch/x86/boot/reloc.c
> >>> index b992678b5e..1fe19294ad 100644
> >>> --- a/xen/arch/x86/boot/reloc.c
> >>> +++ b/xen/arch/x86/boot/reloc.c
> >>> @@ -14,8 +14,8 @@
> >>>  
> >>>  /*
> >>>   * This entry point is entered from xen/arch/x86/boot/head.S with:
> >>> - *   - 0x4(%esp) = MULTIBOOT_MAGIC,
> >>> - *   - 0x8(%esp) = MULTIBOOT_INFORMATION_ADDRESS,
> >>> + *   - 0x4(%esp) = MAGIC,
> >>> + *   - 0x8(%esp) = INFORMATION_ADDRESS,
> >>>   *   - 0xc(%esp) = TOPMOST_LOW_MEMORY_STACK_ADDRESS.
> >>>   */
> >>>  asm (
> >>> @@ -29,6 +29,8 @@ asm (
> >>>  #include "../../../include/xen/multiboot.h"
> >>>  #include "../../../include/xen/multiboot2.h"
> >>>  
> >>> +#include "../../../include/generated/autoconf.h"
> >>> +
> >>>  #define get_mb2_data(tag, type, member)   (((multiboot2_tag_##type##_t 
> >>> *)(tag))->member)
> >>>  #define get_mb2_string(tag, type, member) ((u32)get_mb2_data(tag, type, 
> >>> member))
> >>>  
> >>> @@ -71,6 +73,41 @@ static u32 copy_string(u32 src)
> >>>      return copy_mem(src, p - src + 1);
> >>>  }
> >>>  
> >>> +#ifdef CONFIG_PVH_GUEST
> >> ... drop this ifdef and ...
> >>
> > So you want reloc.o to contain pvh_info_reloc unconditionally?
> >
> > Fundamentally I don't think I care enough about all the bikeshedding so
> > if Jan and you agree on this I will just make the change.
> 
> It wont.  The function will be dropped due to DCE, but we'll spot build
> breakages far more easily.  (The important bit is that the function call
> is guarded by the IS_ENABLED())

reloc.o will still have that function in non-PVH build on my machine.
And that's with the following diff applied.

Again. I don't care to argue one way or the other. I have both versions.
You and Jan need to decide which version you like.

---8<---
From 44300edc0e85d841ea2fd1404758d37a46ab0524 Mon Sep 17 00:00:00 2001
From: Wei Liu <wei.liu2@xxxxxxxxxx>
Date: Mon, 22 Jan 2018 18:30:16 +0000
Subject: [PATCH] xxx

---
 xen/arch/x86/boot/Makefile |  7 ++-----
 xen/arch/x86/boot/head.S   |  6 +++---
 xen/arch/x86/boot/reloc.c  | 14 +++++---------
 3 files changed, 10 insertions(+), 17 deletions(-)

diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
index 1b3f121a2f..e10388282f 100644
--- a/xen/arch/x86/boot/Makefile
+++ b/xen/arch/x86/boot/Makefile
@@ -5,11 +5,8 @@ DEFS_H_DEPS = defs.h $(BASEDIR)/include/xen/stdbool.h
 CMDLINE_DEPS = $(DEFS_H_DEPS) video.h
 
 RELOC_DEPS = $(DEFS_H_DEPS) $(BASEDIR)/include/xen/multiboot.h \
-            $(BASEDIR)/include/xen/multiboot2.h
-
-ifeq ($(CONFIG_PVH_GUEST),y)
-RELOC_DEPS += $(BASEDIR)/include/public/arch-x86/hvm/start_info.h
-endif
+            $(BASEDIR)/include/xen/multiboot2.h \
+            $(BASEDIR)/include/public/arch-x86/hvm/start_info.h
 
 head.o: cmdline.S reloc.S
 
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index 9219067231..3cb66fc06b 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -585,13 +585,13 @@ trampoline_setup:
         push    %eax                /* Magic number. */
         call    reloc
 #ifdef CONFIG_PVH_GUEST
-        cmp     $0,sym_fs(pvh_boot)
+        cmp     $0, sym_fs(pvh_boot)
         je      1f
-        mov     %eax,sym_fs(pvh_start_info_pa)
+        mov     %eax, sym_fs(pvh_start_info_pa)
         jmp     2f
 #endif
 1:
-        mov     %eax,sym_fs(multiboot_ptr)
+        mov     %eax, sym_fs(multiboot_ptr)
 2:
 
         /*
diff --git a/xen/arch/x86/boot/reloc.c b/xen/arch/x86/boot/reloc.c
index 1fe19294ad..8a66390383 100644
--- a/xen/arch/x86/boot/reloc.c
+++ b/xen/arch/x86/boot/reloc.c
@@ -29,7 +29,8 @@ asm (
 #include "../../../include/xen/multiboot.h"
 #include "../../../include/xen/multiboot2.h"
 
-#include "../../../include/generated/autoconf.h"
+#include "../../../include/xen/kconfig.h"
+#include <public/arch-x86/hvm/start_info.h>
 
 #define get_mb2_data(tag, type, member)   (((multiboot2_tag_##type##_t 
*)(tag))->member)
 #define get_mb2_string(tag, type, member) ((u32)get_mb2_data(tag, type, 
member))
@@ -73,10 +74,6 @@ static u32 copy_string(u32 src)
     return copy_mem(src, p - src + 1);
 }
 
-#ifdef CONFIG_PVH_GUEST
-
-#include <public/arch-x86/hvm/start_info.h>
-
 static struct hvm_start_info *pvh_info_reloc(u32 in)
 {
     struct hvm_start_info *out;
@@ -106,7 +103,6 @@ static struct hvm_start_info *pvh_info_reloc(u32 in)
 
     return out;
 }
-#endif
 
 static multiboot_info_t *mbi_reloc(u32 mbi_in)
 {
@@ -275,10 +271,10 @@ void __stdcall *reloc(u32 magic, u32 in, u32 trampoline)
     case MULTIBOOT2_BOOTLOADER_MAGIC:
         return mbi2_reloc(in);
 
-#ifdef CONFIG_PVH_GUEST
     case XEN_HVM_START_MAGIC_VALUE:
-        return pvh_info_reloc(in);
-#endif
+        if ( IS_ENABLED(CONFIG_PVH_GUEST) )
+            return pvh_info_reloc(in);
+        /* Fallthrough */
 
     default:
         /* Nothing we can do */
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel