Re: [Xen-devel] [PATCH v4] x86: relocate pvh_info
>>> On 23.01.18 at 12:44,wrote: > On Tue, Jan 23, 2018 at 02:14:51AM -0700, Jan Beulich wrote: >> >>> On 22.01.18 at 19:31, wrote: >> > On Mon, Jan 22, 2018 at 06:19:43PM +, 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
Re: [Xen-devel] [PATCH v4] x86: relocate pvh_info
On Tue, Jan 23, 2018 at 02:14:51AM -0700, Jan Beulich wrote: > >>> On 22.01.18 at 19:31,wrote: > > On Mon, Jan 22, 2018 at 06:19:43PM +, 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
Re: [Xen-devel] [PATCH v4] x86: relocate pvh_info
>>> On 22.01.18 at 19:31,wrote: > On Mon, Jan 22, 2018 at 06:19:43PM +, 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
Re: [Xen-devel] [PATCH v4] x86: relocate pvh_info
On Mon, Jan 22, 2018 at 06:19:43PM +, Andrew Cooper wrote: > On 22/01/18 18:17, Wei Liu wrote: > > On Mon, Jan 22, 2018 at 06:09:14PM +, 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> >>> --- > >>> Cc: Jan Beulich > >>> Cc: Andrew Cooper > >>> Cc: Roger Pau Monné > >>> Cc: Doug Goldstein > >>> > >>> 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 Date: Mon, 22 Jan 2018 18:30:16 + 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. */ callreloc #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) +
Re: [Xen-devel] [PATCH v4] x86: relocate pvh_info
On 22/01/18 18:17, Wei Liu wrote: > On Mon, Jan 22, 2018 at 06:09:14PM +, 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>>> --- >>> Cc: Jan Beulich >>> Cc: Andrew Cooper >>> Cc: Roger Pau Monné >>> Cc: Doug Goldstein >>> >>> 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
Re: [Xen-devel] [PATCH v4] x86: relocate pvh_info
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> --- > Cc: Jan Beulich > Cc: Andrew Cooper > Cc: Roger Pau Monné > Cc: Doug Goldstein > > 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. */ > callreloc > -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
Re: [Xen-devel] [PATCH v4] x86: relocate pvh_info
On Mon, Jan 22, 2018 at 06:09:14PM +, 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> > --- > > Cc: Jan Beulich > > Cc: Andrew Cooper > > Cc: Roger Pau Monné > > Cc: Doug Goldstein > > > > 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
Re: [Xen-devel] [PATCH v4] x86: relocate pvh_info
>>> On 22.01.18 at 17:13,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 As before Reviewed-by: Jan Beulich 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