Re: [PATCH v2 3/4] x86/boot: Move some settings to C

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxxx>
> ---
>  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