All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4] x86: relocate pvh_info
@ 2018-01-22 16:13 Wei Liu
  2018-01-22 16:56 ` Jan Beulich
  2018-01-22 18:09 ` Andrew Cooper
  0 siblings, 2 replies; 9+ messages in thread
From: Wei Liu @ 2018-01-22 16:13 UTC (permalink / raw)
  To: Xen-devel
  Cc: Doug Goldstein, Andrew Cooper, Wei Liu, Jan Beulich,
	Roger Pau Monné

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@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Roger Pau Monné <roger.pau@citrix.com>
Cc: Doug Goldstein <cardoe@cardoe.com>

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
 
+ifeq ($(CONFIG_PVH_GUEST),y)
+RELOC_DEPS += $(BASEDIR)/include/public/arch-x86/hvm/start_info.h
+endif
+
 head.o: cmdline.S reloc.S
 
 cmdline.S: cmdline.c $(CMDLINE_DEPS)
diff --git a/xen/arch/x86/boot/defs.h b/xen/arch/x86/boot/defs.h
index 6abdc15446..05921a64a3 100644
--- a/xen/arch/x86/boot/defs.h
+++ b/xen/arch/x86/boot/defs.h
@@ -51,6 +51,9 @@ typedef unsigned short u16;
 typedef unsigned int u32;
 typedef unsigned long long u64;
 typedef unsigned int size_t;
+typedef u8 uint8_t;
+typedef u32 uint32_t;
+typedef u64 uint64_t;
 
 #define U16_MAX		((u16)(~0U))
 #define UINT_MAX	(~0U)
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index 0f652cea11..aa2e2a93c8 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -414,6 +414,7 @@ __pvh_start:
 
         /* Set trampoline_phys to use mfn 1 to avoid having a mapping at VA 0 */
         movw    $0x1000, sym_esi(trampoline_phys)
+        movl    (%ebx), %eax /* mov $XEN_HVM_START_MAGIC_VALUE, %eax */
         jmp     trampoline_setup
 
 #endif /* CONFIG_PVH_GUEST */
@@ -578,18 +579,20 @@ trampoline_setup:
         /* Get bottom-most low-memory stack address. */
         add     $TRAMPOLINE_SPACE,%ecx
 
-#ifdef CONFIG_PVH_GUEST
-        cmpb    $0, sym_fs(pvh_boot)
-        jne     1f
-#endif
-
-        /* Save the Multiboot info struct (after relocation) for later use. */
+        /* Save Multiboot / PVH info struct (after relocation) for later use. */
         push    %ecx                /* Bottom-most low-memory stack address. */
-        push    %ebx                /* Multiboot information address. */
-        push    %eax                /* Multiboot magic. */
+        push    %ebx                /* Multiboot / PVH information address. */
+        push    %eax                /* Magic number. */
         call    reloc
-        mov     %eax,sym_fs(multiboot_ptr)
+#ifdef CONFIG_PVH_GUEST
+        cmp     $0,sym_fs(pvh_boot)
+        je      1f
+        mov     %eax,sym_fs(pvh_start_info_pa)
+        jmp     2f
+#endif
 1:
+        mov     %eax,sym_fs(multiboot_ptr)
+2:
 
         /*
          * Now trampoline_phys points to the following structure (lowest address
@@ -598,12 +601,12 @@ trampoline_setup:
          * +------------------------+
          * | TRAMPOLINE_STACK_SPACE |
          * +------------------------+
-         * |        mbi data        |
+         * |     Data (MBI / PVH)   |
          * +- - - - - - - - - - - - +
          * |    TRAMPOLINE_SPACE    |
          * +------------------------+
          *
-         * mbi data grows downwards from the highest address of TRAMPOLINE_SPACE
+         * Data grows downwards from the highest address of TRAMPOLINE_SPACE
          * region to the end of the trampoline. The rest of TRAMPOLINE_SPACE is
          * reserved for trampoline code and data.
          */
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
+
+#include <public/arch-x86/hvm/start_info.h>
+
+static struct hvm_start_info *pvh_info_reloc(u32 in)
+{
+    struct hvm_start_info *out;
+
+    out = _p(copy_mem(in, sizeof(*out)));
+
+    if ( out->cmdline_paddr )
+        out->cmdline_paddr = copy_string(out->cmdline_paddr);
+
+    if ( out->nr_modules )
+    {
+        unsigned int i;
+        struct hvm_modlist_entry *mods;
+
+        out->modlist_paddr =
+            copy_mem(out->modlist_paddr,
+                     out->nr_modules * sizeof(struct hvm_modlist_entry));
+
+        mods = _p(out->modlist_paddr);
+
+        for ( i = 0; i < out->nr_modules; i++ )
+        {
+            if ( mods[i].cmdline_paddr )
+                mods[i].cmdline_paddr = copy_string(mods[i].cmdline_paddr);
+        }
+    }
+
+    return out;
+}
+#endif
+
 static multiboot_info_t *mbi_reloc(u32 mbi_in)
 {
     int i;
@@ -226,14 +263,27 @@ static multiboot_info_t *mbi2_reloc(u32 mbi_in)
     return mbi_out;
 }
 
-multiboot_info_t __stdcall *reloc(u32 mb_magic, u32 mbi_in, u32 trampoline)
+void __stdcall *reloc(u32 magic, u32 in, u32 trampoline)
 {
     alloc = trampoline;
 
-    if ( mb_magic == MULTIBOOT2_BOOTLOADER_MAGIC )
-        return mbi2_reloc(mbi_in);
-    else
-        return mbi_reloc(mbi_in);
+    switch ( magic )
+    {
+    case MULTIBOOT_BOOTLOADER_MAGIC:
+        return mbi_reloc(in);
+
+    case MULTIBOOT2_BOOTLOADER_MAGIC:
+        return mbi2_reloc(in);
+
+#ifdef CONFIG_PVH_GUEST
+    case XEN_HVM_START_MAGIC_VALUE:
+        return pvh_info_reloc(in);
+#endif
+
+    default:
+        /* Nothing we can do */
+        return NULL;
+    }
 }
 
 /*
-- 
2.11.0


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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v4] x86: relocate pvh_info
  2018-01-22 16:13 [PATCH v4] x86: relocate pvh_info Wei Liu
@ 2018-01-22 16:56 ` Jan Beulich
  2018-01-22 18:09 ` Andrew Cooper
  1 sibling, 0 replies; 9+ messages in thread
From: Jan Beulich @ 2018-01-22 16:56 UTC (permalink / raw)
  To: Wei Liu; +Cc: AndrewCooper, Doug Goldstein, Xen-devel, Roger Pau Monné

>>> On 22.01.18 at 17:13, <wei.liu2@citrix.com> 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@citrix.com>

As before
Reviewed-by: Jan Beulich <jbeulich@suse.com>
with the caveat that this shouldn't go in without Andrew
withdrawing his general objection.

Jan


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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v4] x86: relocate pvh_info
  2018-01-22 16:13 [PATCH v4] x86: relocate pvh_info Wei Liu
  2018-01-22 16:56 ` Jan Beulich
@ 2018-01-22 18:09 ` Andrew Cooper
  2018-01-22 18:17   ` Wei Liu
  1 sibling, 1 reply; 9+ messages in thread
From: Andrew Cooper @ 2018-01-22 18:09 UTC (permalink / raw)
  To: Wei Liu, Xen-devel; +Cc: Doug Goldstein, Jan Beulich, Roger Pau Monné

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@citrix.com>
> ---
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Doug Goldstein <cardoe@cardoe.com>
>
> 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 ...

>  
> +ifeq ($(CONFIG_PVH_GUEST),y)
> +RELOC_DEPS += $(BASEDIR)/include/public/arch-x86/hvm/start_info.h
> +endif

and this unconditionally, and ...

> +
>  head.o: cmdline.S reloc.S
>  
>  cmdline.S: cmdline.c $(CMDLINE_DEPS)
> diff --git a/xen/arch/x86/boot/defs.h b/xen/arch/x86/boot/defs.h
> index 6abdc15446..05921a64a3 100644
> --- a/xen/arch/x86/boot/defs.h
> +++ b/xen/arch/x86/boot/defs.h
> @@ -51,6 +51,9 @@ typedef unsigned short u16;
>  typedef unsigned int u32;
>  typedef unsigned long long u64;
>  typedef unsigned int size_t;
> +typedef u8 uint8_t;
> +typedef u32 uint32_t;
> +typedef u64 uint64_t;
>  
>  #define U16_MAX		((u16)(~0U))
>  #define UINT_MAX	(~0U)
> diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
> index 0f652cea11..aa2e2a93c8 100644
> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -414,6 +414,7 @@ __pvh_start:
>  
>          /* Set trampoline_phys to use mfn 1 to avoid having a mapping at VA 0 */
>          movw    $0x1000, sym_esi(trampoline_phys)
> +        movl    (%ebx), %eax /* mov $XEN_HVM_START_MAGIC_VALUE, %eax */
>          jmp     trampoline_setup
>  
>  #endif /* CONFIG_PVH_GUEST */
> @@ -578,18 +579,20 @@ trampoline_setup:
>          /* Get bottom-most low-memory stack address. */
>          add     $TRAMPOLINE_SPACE,%ecx
>  
> -#ifdef CONFIG_PVH_GUEST
> -        cmpb    $0, sym_fs(pvh_boot)
> -        jne     1f
> -#endif
> -
> -        /* Save the Multiboot info struct (after relocation) for later use. */
> +        /* Save Multiboot / PVH info struct (after relocation) for later use. */
>          push    %ecx                /* Bottom-most low-memory stack address. */
> -        push    %ebx                /* Multiboot information address. */
> -        push    %eax                /* Multiboot magic. */
> +        push    %ebx                /* Multiboot / PVH information address. */
> +        push    %eax                /* Magic number. */
>          call    reloc
> -        mov     %eax,sym_fs(multiboot_ptr)
> +#ifdef CONFIG_PVH_GUEST
> +        cmp     $0,sym_fs(pvh_boot)
> +        je      1f
> +        mov     %eax,sym_fs(pvh_start_info_pa)
> +        jmp     2f
> +#endif
>  1:
> +        mov     %eax,sym_fs(multiboot_ptr)
> +2:

For new code, please have spaces after commas for readibility.

>  
>          /*
>           * Now trampoline_phys points to the following structure (lowest address
> @@ -598,12 +601,12 @@ trampoline_setup:
>           * +------------------------+
>           * | TRAMPOLINE_STACK_SPACE |
>           * +------------------------+
> -         * |        mbi data        |
> +         * |     Data (MBI / PVH)   |
>           * +- - - - - - - - - - - - +
>           * |    TRAMPOLINE_SPACE    |
>           * +------------------------+
>           *
> -         * mbi data grows downwards from the highest address of TRAMPOLINE_SPACE
> +         * Data grows downwards from the highest address of TRAMPOLINE_SPACE
>           * region to the end of the trampoline. The rest of TRAMPOLINE_SPACE is
>           * reserved for trampoline code and data.
>           */
> 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 ...

> +
> +#include <public/arch-x86/hvm/start_info.h>
> +
> +static struct hvm_start_info *pvh_info_reloc(u32 in)
> +{
> +    struct hvm_start_info *out;
> +
> +    out = _p(copy_mem(in, sizeof(*out)));
> +
> +    if ( out->cmdline_paddr )
> +        out->cmdline_paddr = copy_string(out->cmdline_paddr);
> +
> +    if ( out->nr_modules )
> +    {
> +        unsigned int i;
> +        struct hvm_modlist_entry *mods;
> +
> +        out->modlist_paddr =
> +            copy_mem(out->modlist_paddr,
> +                     out->nr_modules * sizeof(struct hvm_modlist_entry));
> +
> +        mods = _p(out->modlist_paddr);
> +
> +        for ( i = 0; i < out->nr_modules; i++ )
> +        {
> +            if ( mods[i].cmdline_paddr )
> +                mods[i].cmdline_paddr = copy_string(mods[i].cmdline_paddr);
> +        }
> +    }
> +
> +    return out;
> +}
> +#endif
> +
>  static multiboot_info_t *mbi_reloc(u32 mbi_in)
>  {
>      int i;
> @@ -226,14 +263,27 @@ static multiboot_info_t *mbi2_reloc(u32 mbi_in)
>      return mbi_out;
>  }
>  
> -multiboot_info_t __stdcall *reloc(u32 mb_magic, u32 mbi_in, u32 trampoline)
> +void __stdcall *reloc(u32 magic, u32 in, u32 trampoline)
>  {
>      alloc = trampoline;
>  
> -    if ( mb_magic == MULTIBOOT2_BOOTLOADER_MAGIC )
> -        return mbi2_reloc(mbi_in);
> -    else
> -        return mbi_reloc(mbi_in);
> +    switch ( magic )
> +    {
> +    case MULTIBOOT_BOOTLOADER_MAGIC:
> +        return mbi_reloc(in);
> +
> +    case MULTIBOOT2_BOOTLOADER_MAGIC:
> +        return mbi2_reloc(in);
> +
> +#ifdef CONFIG_PVH_GUEST
> +    case XEN_HVM_START_MAGIC_VALUE:
> +        return pvh_info_reloc(in);
> +#endif

Rearrange this as:

case XEN_HVM_START_MAGIC_VALUE:
    if ( IS_ENABLED(CONFIG_PVH_GUEST) )
        return pvh_info_reloc(in);
    /* Fallthrough */

Which drops all of the ifdefary.

~Andrew

> +
> +    default:
> +        /* Nothing we can do */
> +        return NULL;
> +    }
>  }
>  
>  /*


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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v4] x86: relocate pvh_info
  2018-01-22 18:09 ` Andrew Cooper
@ 2018-01-22 18:17   ` Wei Liu
  2018-01-22 18:19     ` Andrew Cooper
  0 siblings, 1 reply; 9+ messages in thread
From: Wei Liu @ 2018-01-22 18:17 UTC (permalink / raw)
  To: Andrew Cooper
  Cc: Doug Goldstein, Xen-devel, Wei Liu, Jan Beulich, Roger Pau Monné

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@citrix.com>
> > ---
> > Cc: Jan Beulich <jbeulich@suse.com>
> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > Cc: Roger Pau Monné <roger.pau@citrix.com>
> > Cc: Doug Goldstein <cardoe@cardoe.com>
> >
> > 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.

Wei.

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v4] x86: relocate pvh_info
  2018-01-22 18:17   ` Wei Liu
@ 2018-01-22 18:19     ` Andrew Cooper
  2018-01-22 18:31       ` Wei Liu
  0 siblings, 1 reply; 9+ messages in thread
From: Andrew Cooper @ 2018-01-22 18:19 UTC (permalink / raw)
  To: Wei Liu; +Cc: Xen-devel, Doug Goldstein, Jan Beulich, Roger Pau Monné

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@citrix.com>
>>> ---
>>> Cc: Jan Beulich <jbeulich@suse.com>
>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Cc: Roger Pau Monné <roger.pau@citrix.com>
>>> Cc: Doug Goldstein <cardoe@cardoe.com>
>>>
>>> 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())

~Andrew

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v4] x86: relocate pvh_info
  2018-01-22 18:19     ` Andrew Cooper
@ 2018-01-22 18:31       ` Wei Liu
  2018-01-23  9:14         ` Jan Beulich
  0 siblings, 1 reply; 9+ messages in thread
From: Wei Liu @ 2018-01-22 18:31 UTC (permalink / raw)
  To: Andrew Cooper
  Cc: Doug Goldstein, Xen-devel, Wei Liu, Jan Beulich, Roger Pau Monné

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@citrix.com>
> >>> ---
> >>> Cc: Jan Beulich <jbeulich@suse.com>
> >>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> >>> Cc: Roger Pau Monné <roger.pau@citrix.com>
> >>> Cc: Doug Goldstein <cardoe@cardoe.com>
> >>>
> >>> 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@citrix.com>
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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v4] x86: relocate pvh_info
  2018-01-22 18:31       ` Wei Liu
@ 2018-01-23  9:14         ` Jan Beulich
  2018-01-23 11:44           ` Wei Liu
  0 siblings, 1 reply; 9+ messages in thread
From: Jan Beulich @ 2018-01-23  9:14 UTC (permalink / raw)
  To: Andrew Cooper, Wei Liu; +Cc: Xen-devel, Doug Goldstein, Roger Pau Monné

>>> On 22.01.18 at 19:31, <wei.liu2@citrix.com> wrote:
> On Mon, Jan 22, 2018 at 06:19:43PM +0000, Andrew Cooper wrote:
>> On 22/01/18 18:17, Wei Liu wrote:
>> > 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.

Well, DCE doesn't make any promises towards what it is able to
eliminate, which is why generally I prefer to help the compiler in
cases like the one here.

Jan


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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v4] x86: relocate pvh_info
  2018-01-23  9:14         ` Jan Beulich
@ 2018-01-23 11:44           ` Wei Liu
  2018-01-23 13:19             ` Jan Beulich
  0 siblings, 1 reply; 9+ messages in thread
From: Wei Liu @ 2018-01-23 11:44 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Andrew Cooper, Wei Liu, Xen-devel, Doug Goldstein, Roger Pau Monné

On Tue, Jan 23, 2018 at 02:14:51AM -0700, Jan Beulich wrote:
> >>> On 22.01.18 at 19:31, <wei.liu2@citrix.com> wrote:
> > On Mon, Jan 22, 2018 at 06:19:43PM +0000, Andrew Cooper wrote:
> >> On 22/01/18 18:17, Wei Liu wrote:
> >> > 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.
> 
> Well, DCE doesn't make any promises towards what it is able to
> eliminate, which is why generally I prefer to help the compiler in
> cases like the one here.
> 

If I read this correctly, this means you prefer the ifdef CONFIG_PVH_GUEST
version?

Wei.

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v4] x86: relocate pvh_info
  2018-01-23 11:44           ` Wei Liu
@ 2018-01-23 13:19             ` Jan Beulich
  0 siblings, 0 replies; 9+ messages in thread
From: Jan Beulich @ 2018-01-23 13:19 UTC (permalink / raw)
  To: Wei Liu; +Cc: Andrew Cooper, Doug Goldstein, Xen-devel, Roger Pau Monné

>>> On 23.01.18 at 12:44, <wei.liu2@citrix.com> wrote:
> On Tue, Jan 23, 2018 at 02:14:51AM -0700, Jan Beulich wrote:
>> >>> On 22.01.18 at 19:31, <wei.liu2@citrix.com> wrote:
>> > On Mon, Jan 22, 2018 at 06:19:43PM +0000, Andrew Cooper wrote:
>> >> On 22/01/18 18:17, Wei Liu wrote:
>> >> > 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.
>> 
>> Well, DCE doesn't make any promises towards what it is able to
>> eliminate, which is why generally I prefer to help the compiler in
>> cases like the one here.
> 
> If I read this correctly, this means you prefer the ifdef CONFIG_PVH_GUEST
> version?

Well, in an ideal world I'd prefer what Andrew did suggest. If the
compiler turns out to be incapable of removing dead code
properly, I'd be slightly in favor of the #ifdef approach, but with
Andrew apparently strongly feeling the other way around, I
wouldn't mind that variant (the more that it's all .init.* contents).

Jan


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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2018-01-23 13:19 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-22 16:13 [PATCH v4] x86: relocate pvh_info Wei Liu
2018-01-22 16:56 ` Jan Beulich
2018-01-22 18:09 ` Andrew Cooper
2018-01-22 18:17   ` Wei Liu
2018-01-22 18:19     ` Andrew Cooper
2018-01-22 18:31       ` Wei Liu
2018-01-23  9:14         ` Jan Beulich
2018-01-23 11:44           ` Wei Liu
2018-01-23 13:19             ` Jan Beulich

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.