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

[Jan Beulich <jbeulich@xxxxxxxx>]

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:

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

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

Jan