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

[Frediano Ziglio <frediano.ziglio@xxxxxxxxx>]

On Tue, Dec 10, 2024 at 10:38 AM Jan Beulich <jbeulich@xxxxxxxx> 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@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?
>

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