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.zig...@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
