Re: [PATCH v7 2/2] x86/PVH: Support relocatable dom0 kernels

[Jason Andryuk <jason.andryuk@xxxxxxx>]

On 2024-04-08 03:00, Jan Beulich wrote:
> On 04.04.2024 23:25, Jason Andryuk wrote:
> > Xen tries to load a PVH dom0 kernel at the fixed guest physical address
> > from the elf headers.  For Linux, this defaults to 0x1000000 (16MB), but
> > it can be configured.
> >
> > Unfortunately there exist firmwares that have reserved regions at this
> > address, so Xen fails to load the dom0 kernel since it's not RAM.
> >
> > The PVH entry code is not relocatable - it loads from absolute
> > addresses, which fail when the kernel is loaded at a different address.
> > With a suitably modified kernel, a reloctable entry point is possible.
> >
> > Add XEN_ELFNOTE_PHYS32_RELOC which specifies optional alignment,
> > minimum, and maximum addresses needed for the kernel.  The presence of
> > the NOTE indicates the kernel supports a relocatable entry path.
> >
> > Change the loading to check for an acceptable load address.  If the
> > kernel is relocatable, support finding an alternate load address.
> >
> > The primary motivation for an explicit align field is that Linux has a
> > configurable CONFIG_PHYSICAL_ALIGN field.  This value is present in the
> > bzImage setup header, but not the ELF program headers p_align, which
> > report 2MB even when CONFIG_PHYSICAL_ALIGN is greater.  Since a kernel
> > is only considered relocatable if the PHYS32_RELOC elf note is present,
> > the alignment contraints can just be specified within the note instead
> > of searching for an alignment value via a heuristic.
> >
> > Load alignment uses the PHYS32_RELOC note value if specified.
> > Otherwise, the maxmum ELF PHDR p_align value is selected if greater than
> > or equal to PAGE_SIZE.  Finally, the fallback default is 2MB.
> >
> > libelf-private.h includes common-macros.h to satisfy the fuzzer build.
> >
> > Link: https://gitlab.com/xen-project/xen/-/issues/180
> > Signed-off-by: Jason Andryuk <jason.andryuk@xxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> yet still with two remarks:
Thanks.

> > --- a/xen/arch/x86/hvm/dom0_build.c
> > +++ b/xen/arch/x86/hvm/dom0_build.c
> > @@ -537,6 +537,111 @@ static paddr_t __init find_memory(
> >       return INVALID_PADDR;
> >   }
> >   
> > +static bool __init check_load_address(
> > +    const struct domain *d, const struct elf_binary *elf)
> > +{
> > +    paddr_t kernel_start = (uintptr_t)elf->dest_base;
> > +    paddr_t kernel_end = kernel_start + elf->dest_size;
> > +    unsigned int i;
> While properly typed here, ...
>
> > +static paddr_t __init find_kernel_memory(
> > +    const struct domain *d, struct elf_binary *elf,
> > +    const struct elf_dom_parms *parms)
> > +{
> > +    paddr_t kernel_size = elf->dest_size;
> > +    unsigned int align;
> > +    int i;
> ... I must have missed when this was changed to plain int. It should have
> been unsigned int here, too, ...
>
> > +    if ( parms->phys_align != UNSET_ADDR32 )
> > +        align = parms->phys_align;
> > +    else if ( elf->palign >= PAGE_SIZE )
> > +        align = elf->palign;
> > +    else
> > +        align = MB(2);
> > +
> > +    /* Search backwards to find the highest address. */
> > +    for ( i = d->arch.nr_e820 - 1; i >= 0 ; i-- )
> ... with this suitably adjusted. However, I'm not going to change this while
> committing, to avoid screwing up.
I intentionally changed this.  Looping downwards, a signed int allows 
writing the check naturally with i >= 0.  I think it's clearer when 
written this way.
> > --- a/xen/include/public/elfnote.h
> > +++ b/xen/include/public/elfnote.h
> > @@ -196,10 +196,28 @@
> >    */
> >   #define XEN_ELFNOTE_PHYS32_ENTRY 18
> >   
> > +/*
> > + * Physical loading constraints for PVH kernels
> > + *
> > + * The presence of this note indicates the kernel supports relocating itself.
> > + *
> > + * The note may include up to three 32bit values to place constraints on the
> > + * guest physical loading addresses and alignment for a PVH kernel.  Values
> > + * are read in the following order:
> > + *  - a required start alignment (default 0x200000)
> "... (default 0x200000; see below)", i.e. ...
>
> > + *  - a minimum address for the start of the image (default 0)
> > + *  - a maximum address for the last byte of the image (default 0xffffffff)
> > + *
> > + *  When this note specifies an alignment value, it is used.  Otherwise the
> > + *  maximum p_align value from loadable ELF Program Headers is used, if it is
> > + *  greater than or equal to 4k (0x1000).  Otherwise, the default is used.
> ... referring to this (which, btw, has one too many padding blanks on the
> entire paragraph). I will take the liberty to make this adjustment while
> committing.
Thanks.  vim must have kept the indent from the list above, and I didn't 
notice.
Regards,
Jason