Initialise multiboot_ptr and pvh_start_info_pa from C code.
Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
---
xen/arch/x86/boot/build32.lds.S | 3 +++
xen/arch/x86/boot/head.S | 10 --------
xen/arch/x86/boot/reloc.c | 28 ++++++++++++++++++-----
xen/arch/x86/include/asm/guest/pvh-boot.h | 1 +
4 files changed, 26 insertions(+), 16 deletions(-)
diff --git a/xen/arch/x86/boot/build32.lds.S b/xen/arch/x86/boot/build32.lds.S
index 1e59732edd..1726c17c88 100644
--- a/xen/arch/x86/boot/build32.lds.S
+++ b/xen/arch/x86/boot/build32.lds.S
@@ -51,6 +51,9 @@ SECTIONS
DECLARE_IMPORT(__trampoline_seg_stop);
DECLARE_IMPORT(trampoline_phys);
DECLARE_IMPORT(boot_vid_info);
+ DECLARE_IMPORT(multiboot_ptr);
+ DECLARE_IMPORT(pvh_boot);
+ DECLARE_IMPORT(pvh_start_info_pa);
. = . + GAP;
*(.text)
*(.text.*)
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index 1b3bd16fe5..510b3cfe6c 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -517,16 +517,6 @@ trampoline_setup:
/* reloc(magic/eax, info/edx) using fastcall. */
call reloc
-#ifdef CONFIG_PVH_GUEST
- cmpb $0, sym_esi(pvh_boot)
- je 1f
- mov %eax, sym_esi(pvh_start_info_pa)
- jmp 2f
-#endif
-1:
- mov %eax, sym_esi(multiboot_ptr)
-2:
-
/* Interrogate CPU extended features via CPUID. */
mov $1, %eax
cpuid
diff --git a/xen/arch/x86/boot/reloc.c b/xen/arch/x86/boot/reloc.c
index 7a375ad41c..8f757813ee 100644
--- a/xen/arch/x86/boot/reloc.c
+++ b/xen/arch/x86/boot/reloc.c
@@ -17,13 +17,15 @@
#include <xen/types.h>
#include <xen/kconfig.h>
-#include <xen/multiboot.h>
#include <xen/multiboot2.h>
#include <xen/page-size.h>
+#include <xen/bug.h>
#include <asm/trampoline.h>
+#include <asm/setup.h>
#include <public/arch-x86/hvm/start_info.h>
+#include <asm/guest/pvh-boot.h>
#ifdef CONFIG_VIDEO
# include "video.h"
@@ -347,27 +349,41 @@ static multiboot_info_t *mbi2_reloc(uint32_t mbi_in, memctx *ctx)
}
/* SAF-1-safe */
-void *reloc(uint32_t magic, uint32_t in)
+void reloc(uint32_t magic, uint32_t in)
{
memctx ctx = { trampoline_phys + TRAMPOLINE_HEAP_END };
+ void *res;
+
switch ( magic )
{
case MULTIBOOT_BOOTLOADER_MAGIC:
- return mbi_reloc(in, &ctx);
+ res = mbi_reloc(in, &ctx);
+ break;
case MULTIBOOT2_BOOTLOADER_MAGIC:
- return mbi2_reloc(in, &ctx);
+ res = mbi2_reloc(in, &ctx);
+ break;
case XEN_HVM_START_MAGIC_VALUE:
if ( IS_ENABLED(CONFIG_PVH_GUEST) )
- return pvh_info_reloc(in, &ctx);
+ {
+ res = pvh_info_reloc(in, &ctx);
+ break;
+ }
/* Fallthrough */
default:
/* Nothing we can do */
- return NULL;
+ res = NULL;
}
+
+#ifdef CONFIG_PVH_GUEST
+ if ( pvh_boot )
+ pvh_start_info_pa = (unsigned long)res;
+#endif
+
+ multiboot_ptr = (unsigned long)res;
}
/*
diff --git a/xen/arch/x86/include/asm/guest/pvh-boot.h b/xen/arch/x86/include/asm/guest/pvh-boot.h
index 247ba6899e..b5ad2b11a4 100644
--- a/xen/arch/x86/include/asm/guest/pvh-boot.h
+++ b/xen/arch/x86/include/asm/guest/pvh-boot.h
@@ -13,6 +13,7 @@
#ifdef CONFIG_PVH_GUEST
extern bool pvh_boot;
+extern uint32_t pvh_start_info_pa;
void pvh_init(multiboot_info_t **mbi, module_t **mod);
void pvh_print_info(void);
--
2.34.1On 22.11.2024 10:33, Frediano Ziglio wrote:
> Initialise multiboot_ptr and pvh_start_info_pa from C code.
>
> Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
> ---
> xen/arch/x86/boot/build32.lds.S | 3 +++
> xen/arch/x86/boot/head.S | 10 --------
> xen/arch/x86/boot/reloc.c | 28 ++++++++++++++++++-----
> xen/arch/x86/include/asm/guest/pvh-boot.h | 1 +
> 4 files changed, 26 insertions(+), 16 deletions(-)
From the diffstat alone - is this really a win?
> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -517,16 +517,6 @@ trampoline_setup:
> /* reloc(magic/eax, info/edx) using fastcall. */
> call reloc
>
> -#ifdef CONFIG_PVH_GUEST
> - cmpb $0, sym_esi(pvh_boot)
> - je 1f
> - mov %eax, sym_esi(pvh_start_info_pa)
> - jmp 2f
> -#endif
> -1:
> - mov %eax, sym_esi(multiboot_ptr)
> -2:
> -
> /* Interrogate CPU extended features via CPUID. */
> mov $1, %eax
> cpuid
> --- a/xen/arch/x86/boot/reloc.c
> +++ b/xen/arch/x86/boot/reloc.c
> @@ -17,13 +17,15 @@
> #include <xen/types.h>
>
> #include <xen/kconfig.h>
> -#include <xen/multiboot.h>
> #include <xen/multiboot2.h>
> #include <xen/page-size.h>
> +#include <xen/bug.h>
>
> #include <asm/trampoline.h>
> +#include <asm/setup.h>
>
> #include <public/arch-x86/hvm/start_info.h>
> +#include <asm/guest/pvh-boot.h>
>
> #ifdef CONFIG_VIDEO
> # include "video.h"
> @@ -347,27 +349,41 @@ static multiboot_info_t *mbi2_reloc(uint32_t mbi_in, memctx *ctx)
> }
>
> /* SAF-1-safe */
> -void *reloc(uint32_t magic, uint32_t in)
> +void reloc(uint32_t magic, uint32_t in)
> {
> memctx ctx = { trampoline_phys + TRAMPOLINE_HEAP_END };
>
> + void *res;
> +
Nit: Please avoid blank lines between declarations unless the set of locals
is huge, or some really need to stand out.
> switch ( magic )
> {
> case MULTIBOOT_BOOTLOADER_MAGIC:
> - return mbi_reloc(in, &ctx);
> + res = mbi_reloc(in, &ctx);
> + break;
>
> case MULTIBOOT2_BOOTLOADER_MAGIC:
> - return mbi2_reloc(in, &ctx);
> + res = mbi2_reloc(in, &ctx);
> + break;
>
> case XEN_HVM_START_MAGIC_VALUE:
> if ( IS_ENABLED(CONFIG_PVH_GUEST) )
> - return pvh_info_reloc(in, &ctx);
> + {
> + res = pvh_info_reloc(in, &ctx);
> + break;
> + }
> /* Fallthrough */
>
> default:
> /* Nothing we can do */
> - return NULL;
> + res = NULL;
Simply keep returning here? No need to write the NULL when the variables
start out zeroed?
> }
> +
> +#ifdef CONFIG_PVH_GUEST
> + if ( pvh_boot )
> + pvh_start_info_pa = (unsigned long)res;
> +#endif
> +
> + multiboot_ptr = (unsigned long)res;
In the assembly original this is an "else" to the if().
Jan
On Tue, Dec 10, 2024 at 10:38 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 22.11.2024 10:33, Frediano Ziglio wrote:
> > Initialise multiboot_ptr and pvh_start_info_pa from C code.
> >
> > Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
> > ---
> > xen/arch/x86/boot/build32.lds.S | 3 +++
> > xen/arch/x86/boot/head.S | 10 --------
> > xen/arch/x86/boot/reloc.c | 28 ++++++++++++++++++-----
> > xen/arch/x86/include/asm/guest/pvh-boot.h | 1 +
> > 4 files changed, 26 insertions(+), 16 deletions(-)
>
> From the diffstat alone - is this really a win?
>
Yes, C can be longer then assembly, consider calling a function, assembly:
foo:
call foo
C:
void foo(int x); // declaration (maybe in a separate header)
void foo(int x) {
...
}
foo(123);
yes, much longer. Actually we could avoid the declaration, but usually
we explicitly force the compiler to complains about a missing
declaration. The reason is that usually programmers prefer the
compiler to avoid crashes and check for the passed parameters. This
requires more code but pay back the time not having to debug crashes.
If you look more at wide range (so, not only at this patch) the code
for a bit increases adding new files and symbols but after a while the
code starts to reduce (once added headers and preparation).
> > --- a/xen/arch/x86/boot/head.S
> > +++ b/xen/arch/x86/boot/head.S
> > @@ -517,16 +517,6 @@ trampoline_setup:
> > /* reloc(magic/eax, info/edx) using fastcall. */
> > call reloc
> >
> > -#ifdef CONFIG_PVH_GUEST
> > - cmpb $0, sym_esi(pvh_boot)
> > - je 1f
> > - mov %eax, sym_esi(pvh_start_info_pa)
> > - jmp 2f
> > -#endif
> > -1:
> > - mov %eax, sym_esi(multiboot_ptr)
> > -2:
> > -
> > /* Interrogate CPU extended features via CPUID. */
> > mov $1, %eax
> > cpuid
> > --- a/xen/arch/x86/boot/reloc.c
> > +++ b/xen/arch/x86/boot/reloc.c
> > @@ -17,13 +17,15 @@
> > #include <xen/types.h>
> >
> > #include <xen/kconfig.h>
> > -#include <xen/multiboot.h>
> > #include <xen/multiboot2.h>
> > #include <xen/page-size.h>
> > +#include <xen/bug.h>
> >
> > #include <asm/trampoline.h>
> > +#include <asm/setup.h>
> >
> > #include <public/arch-x86/hvm/start_info.h>
> > +#include <asm/guest/pvh-boot.h>
> >
> > #ifdef CONFIG_VIDEO
> > # include "video.h"
> > @@ -347,27 +349,41 @@ static multiboot_info_t *mbi2_reloc(uint32_t mbi_in, memctx *ctx)
> > }
> >
> > /* SAF-1-safe */
> > -void *reloc(uint32_t magic, uint32_t in)
> > +void reloc(uint32_t magic, uint32_t in)
> > {
> > memctx ctx = { trampoline_phys + TRAMPOLINE_HEAP_END };
> >
> > + void *res;
> > +
>
> Nit: Please avoid blank lines between declarations unless the set of locals
> is huge, or some really need to stand out.
>
Noted.
> > switch ( magic )
> > {
> > case MULTIBOOT_BOOTLOADER_MAGIC:
> > - return mbi_reloc(in, &ctx);
> > + res = mbi_reloc(in, &ctx);
> > + break;
> >
> > case MULTIBOOT2_BOOTLOADER_MAGIC:
> > - return mbi2_reloc(in, &ctx);
> > + res = mbi2_reloc(in, &ctx);
> > + break;
> >
> > case XEN_HVM_START_MAGIC_VALUE:
> > if ( IS_ENABLED(CONFIG_PVH_GUEST) )
> > - return pvh_info_reloc(in, &ctx);
> > + {
> > + res = pvh_info_reloc(in, &ctx);
> > + break;
> > + }
> > /* Fallthrough */
> >
> > default:
> > /* Nothing we can do */
> > - return NULL;
> > + res = NULL;
>
> Simply keep returning here? No need to write the NULL when the variables
> start out zeroed?
>
Yes, considering pvh_start_info_pa and multiboot_ptr should be already
NULL it makes sense
> > }
> > +
> > +#ifdef CONFIG_PVH_GUEST
> > + if ( pvh_boot )
> > + pvh_start_info_pa = (unsigned long)res;
> > +#endif
> > +
> > + multiboot_ptr = (unsigned long)res;
>
> In the assembly original this is an "else" to the if().
>
I suppose the return change above would solve also this.
> Jan
Frediano
On 10.12.2024 15:45, Frediano Ziglio wrote:
> On Tue, Dec 10, 2024 at 10:38 AM Jan Beulich <jbeulich@suse.com> wrote:
>> On 22.11.2024 10:33, Frediano Ziglio wrote:
>>> switch ( magic )
>>> {
>>> case MULTIBOOT_BOOTLOADER_MAGIC:
>>> - return mbi_reloc(in, &ctx);
>>> + res = mbi_reloc(in, &ctx);
>>> + break;
>>>
>>> case MULTIBOOT2_BOOTLOADER_MAGIC:
>>> - return mbi2_reloc(in, &ctx);
>>> + res = mbi2_reloc(in, &ctx);
>>> + break;
>>>
>>> case XEN_HVM_START_MAGIC_VALUE:
>>> if ( IS_ENABLED(CONFIG_PVH_GUEST) )
>>> - return pvh_info_reloc(in, &ctx);
>>> + {
>>> + res = pvh_info_reloc(in, &ctx);
>>> + break;
>>> + }
>>> /* Fallthrough */
>>>
>>> default:
>>> /* Nothing we can do */
>>> - return NULL;
>>> + res = NULL;
>>
>> Simply keep returning here? No need to write the NULL when the variables
>> start out zeroed?
>>
>
> Yes, considering pvh_start_info_pa and multiboot_ptr should be already
> NULL it makes sense
>
>>> }
>>> +
>>> +#ifdef CONFIG_PVH_GUEST
>>> + if ( pvh_boot )
>>> + pvh_start_info_pa = (unsigned long)res;
>>> +#endif
>>> +
>>> + multiboot_ptr = (unsigned long)res;
>>
>> In the assembly original this is an "else" to the if().
>>
>
> I suppose the return change above would solve also this.
I'm not convinced of this.
Jan
© 2016 - 2025 Red Hat, Inc.