| Message ID | 20241007141539.1899350-6-frediano.ziglio@cloud.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Reuse 32 bit C code more safely | expand |
On 07/10/2024 3:15 pm, Frediano Ziglio wrote: > No more need to pass from assembly code. > > Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com> > --- > Changes since v1: > - split the 2 variable changes into 2 commits. > --- > xen/arch/x86/boot/head.S | 13 ++++--------- > xen/arch/x86/boot/reloc.c | 9 ++++++--- > 2 files changed, 10 insertions(+), 12 deletions(-) > > diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S > index ade2c5c43d..32b658fa2b 100644 > --- a/xen/arch/x86/boot/head.S > +++ b/xen/arch/x86/boot/head.S > @@ -510,22 +510,17 @@ trampoline_setup: > mov %esi, sym_esi(xen_phys_start) > mov %esi, sym_esi(trampoline_xen_phys_start) > > - /* Get bottom-most low-memory stack address. */ > - mov sym_esi(trampoline_phys), %ecx > - add $TRAMPOLINE_SPACE,%ecx > - > + /* Boot video info to be filled from MB2. */ > #ifdef CONFIG_VIDEO > - lea sym_esi(boot_vid_info), %edx > + lea sym_esi(boot_vid_info), %ecx > #else > - xor %edx, %edx > + xor %ecx, %ecx > #endif > > /* Save Multiboot / PVH info struct (after relocation) for later use. */ > - push %edx /* Boot video info to be filled from MB2. */ > mov %ebx, %edx /* Multiboot / PVH information address. */ > - /* reloc(magic/eax, info/edx, trampoline/ecx, video/stk) using fastcall. */ > + /* reloc(magic/eax, info/edx, video/stk) using fastcall. */ > call reloc > - add $4, %esp Sorry - I was expecting you split the patches the other way around. If you drop video first, then trampoline second, there's no churn changing video from being on the stack to being in ecx. As it stands the "video/stk" needs to become "video/ecx". > > #ifdef CONFIG_PVH_GUEST > cmpb $0, sym_esi(pvh_boot) > diff --git a/xen/arch/x86/boot/reloc.c b/xen/arch/x86/boot/reloc.c > index 94b078d7b1..4cca61adec 100644 > --- a/xen/arch/x86/boot/reloc.c > +++ b/xen/arch/x86/boot/reloc.c > @@ -19,6 +19,9 @@ > #include <xen/kconfig.h> > #include <xen/multiboot.h> > #include <xen/multiboot2.h> > +#include <xen/page-size.h> > + > +#include <asm/trampoline.h> > > #include <public/arch-x86/hvm/start_info.h> > > @@ -346,10 +349,10 @@ static multiboot_info_t *mbi2_reloc(uint32_t mbi_in, uint32_t video_out, memctx > } > > /* SAF-1-safe */ > -void *reloc(uint32_t magic, uint32_t in, uint32_t trampoline, > - uint32_t video_info) > +void *reloc(uint32_t magic, uint32_t in, uint32_t video_info) > { > - memctx ctx = { trampoline }; > + /* Get bottom-most low-memory stack address. */ > + memctx ctx = { (uint32_t)((long)trampoline_phys + TRAMPOLINE_SPACE) }; You don't need any casts here. The result can't exceed 1<<20. Again, I agree this is a correct transform of the code, but the comment you've moved is incorrect AFAICT, and conflicts with typedef struct memctx { /* * Simple bump allocator. * * It starts from the base of the trampoline and allocates downwards. */ uint32_t ptr; } memctx; which is probably my fault, but needs resolving. What does the trampoline layout actually look like? Can we see about correcting the comments? ~Andrew
On Thu, Oct 10, 2024 at 1:57 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > On 07/10/2024 3:15 pm, Frediano Ziglio wrote: > > No more need to pass from assembly code. > > > > Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com> > > --- > > Changes since v1: > > - split the 2 variable changes into 2 commits. > > --- > > xen/arch/x86/boot/head.S | 13 ++++--------- > > xen/arch/x86/boot/reloc.c | 9 ++++++--- > > 2 files changed, 10 insertions(+), 12 deletions(-) > > > > diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S > > index ade2c5c43d..32b658fa2b 100644 > > --- a/xen/arch/x86/boot/head.S > > +++ b/xen/arch/x86/boot/head.S > > @@ -510,22 +510,17 @@ trampoline_setup: > > mov %esi, sym_esi(xen_phys_start) > > mov %esi, sym_esi(trampoline_xen_phys_start) > > > > - /* Get bottom-most low-memory stack address. */ > > - mov sym_esi(trampoline_phys), %ecx > > - add $TRAMPOLINE_SPACE,%ecx > > - > > + /* Boot video info to be filled from MB2. */ > > #ifdef CONFIG_VIDEO > > - lea sym_esi(boot_vid_info), %edx > > + lea sym_esi(boot_vid_info), %ecx > > #else > > - xor %edx, %edx > > + xor %ecx, %ecx > > #endif > > > > /* Save Multiboot / PVH info struct (after relocation) for later use. */ > > - push %edx /* Boot video info to be filled from MB2. */ > > mov %ebx, %edx /* Multiboot / PVH information address. */ > > - /* reloc(magic/eax, info/edx, trampoline/ecx, video/stk) using fastcall. */ > > + /* reloc(magic/eax, info/edx, video/stk) using fastcall. */ > > call reloc > > - add $4, %esp > > Sorry - I was expecting you split the patches the other way around. > I'll do it again. > If you drop video first, then trampoline second, there's no churn > changing video from being on the stack to being in ecx. > > As it stands the "video/stk" needs to become "video/ecx". > I'll take care. > > > > #ifdef CONFIG_PVH_GUEST > > cmpb $0, sym_esi(pvh_boot) > > diff --git a/xen/arch/x86/boot/reloc.c b/xen/arch/x86/boot/reloc.c > > index 94b078d7b1..4cca61adec 100644 > > --- a/xen/arch/x86/boot/reloc.c > > +++ b/xen/arch/x86/boot/reloc.c > > @@ -19,6 +19,9 @@ > > #include <xen/kconfig.h> > > #include <xen/multiboot.h> > > #include <xen/multiboot2.h> > > +#include <xen/page-size.h> > > + > > +#include <asm/trampoline.h> > > > > #include <public/arch-x86/hvm/start_info.h> > > > > @@ -346,10 +349,10 @@ static multiboot_info_t *mbi2_reloc(uint32_t mbi_in, uint32_t video_out, memctx > > } > > > > /* SAF-1-safe */ > > -void *reloc(uint32_t magic, uint32_t in, uint32_t trampoline, > > - uint32_t video_info) > > +void *reloc(uint32_t magic, uint32_t in, uint32_t video_info) > > { > > - memctx ctx = { trampoline }; > > + /* Get bottom-most low-memory stack address. */ > > + memctx ctx = { (uint32_t)((long)trampoline_phys + TRAMPOLINE_SPACE) }; > > You don't need any casts here. The result can't exceed 1<<20. > I'll do > > Again, I agree this is a correct transform of the code, but the comment > you've moved is incorrect AFAICT, and conflicts with > > typedef struct memctx { > /* > * Simple bump allocator. > * > * It starts from the base of the trampoline and allocates downwards. > */ > uint32_t ptr; > } memctx; > > which is probably my fault, but needs resolving. > > What does the trampoline layout actually look like? Can we see about > correcting the comments? > Can you suggest what I should write? > ~Andrew Frediano
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S index ade2c5c43d..32b658fa2b 100644 --- a/xen/arch/x86/boot/head.S +++ b/xen/arch/x86/boot/head.S @@ -510,22 +510,17 @@ trampoline_setup: mov %esi, sym_esi(xen_phys_start) mov %esi, sym_esi(trampoline_xen_phys_start) - /* Get bottom-most low-memory stack address. */ - mov sym_esi(trampoline_phys), %ecx - add $TRAMPOLINE_SPACE,%ecx - + /* Boot video info to be filled from MB2. */ #ifdef CONFIG_VIDEO - lea sym_esi(boot_vid_info), %edx + lea sym_esi(boot_vid_info), %ecx #else - xor %edx, %edx + xor %ecx, %ecx #endif /* Save Multiboot / PVH info struct (after relocation) for later use. */ - push %edx /* Boot video info to be filled from MB2. */ mov %ebx, %edx /* Multiboot / PVH information address. */ - /* reloc(magic/eax, info/edx, trampoline/ecx, video/stk) using fastcall. */ + /* reloc(magic/eax, info/edx, video/stk) using fastcall. */ call reloc - add $4, %esp #ifdef CONFIG_PVH_GUEST cmpb $0, sym_esi(pvh_boot) diff --git a/xen/arch/x86/boot/reloc.c b/xen/arch/x86/boot/reloc.c index 94b078d7b1..4cca61adec 100644 --- a/xen/arch/x86/boot/reloc.c +++ b/xen/arch/x86/boot/reloc.c @@ -19,6 +19,9 @@ #include <xen/kconfig.h> #include <xen/multiboot.h> #include <xen/multiboot2.h> +#include <xen/page-size.h> + +#include <asm/trampoline.h> #include <public/arch-x86/hvm/start_info.h> @@ -346,10 +349,10 @@ static multiboot_info_t *mbi2_reloc(uint32_t mbi_in, uint32_t video_out, memctx } /* SAF-1-safe */ -void *reloc(uint32_t magic, uint32_t in, uint32_t trampoline, - uint32_t video_info) +void *reloc(uint32_t magic, uint32_t in, uint32_t video_info) { - memctx ctx = { trampoline }; + /* Get bottom-most low-memory stack address. */ + memctx ctx = { (uint32_t)((long)trampoline_phys + TRAMPOLINE_SPACE) }; switch ( magic ) {
No more need to pass from assembly code. Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com> --- Changes since v1: - split the 2 variable changes into 2 commits. --- xen/arch/x86/boot/head.S | 13 ++++--------- xen/arch/x86/boot/reloc.c | 9 ++++++--- 2 files changed, 10 insertions(+), 12 deletions(-)