Re: [PATCH v4 5/5] x86/PVH: Support relocatable dom0 kernels

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.03.2024 21:45, Jason Andryuk wrote:
> +/* Find an e820 RAM region that fits the kernel at a suitable alignment. */
> +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 i;
> +
> +    for ( i = 0; i < d->arch.nr_e820; i++ )
> +    {
> +        paddr_t start = d->arch.e820[i].addr;
> +        paddr_t end = start + d->arch.e820[i].size;
> +        paddr_t kstart, kend;
> +
> +        if ( d->arch.e820[i].type != E820_RAM ||
> +             d->arch.e820[i].size < kernel_size )
> +            continue;
> +
> +        kstart = ROUNDUP(start, parms->phys_align);
> +        kstart = max_t(paddr_t, kstart, parms->phys_min);
> +        kend = kstart + kernel_size;
> +
> +        if ( kend - 1 > parms->phys_max )
> +            return 0;
> +
> +        if ( kend <= end )
> +            return kstart;

IOW within a suitable region the lowest suitable part is selected. Often
low memory is deemed more precious than higher one, so if this choice is
indeed intentional, I'd like to ask for a brief comment towards the
reasons.

> --- a/xen/common/libelf/libelf-dominfo.c
> +++ b/xen/common/libelf/libelf-dominfo.c
> @@ -17,6 +17,16 @@
>  
>  #include "libelf-private.h"
>  
> +#if defined(__i386__) || defined(__x86_64__)
> +#define ARCH_PHYS_MIN_DEFAULT   0;
> +#define ARCH_PHYS_MAX_DEFAULT   (GB(4) - 1);
> +#define ARCH_PHYS_ALIGN_DEFAULT MB(2);
> +#else
> +#define ARCH_PHYS_MIN_DEFAULT   0;
> +#define ARCH_PHYS_MAX_DEFAULT   0;
> +#define ARCH_PHYS_ALIGN_DEFAULT 0;
> +#endif

None of the semicolons should really be here.

> @@ -227,6 +239,27 @@ elf_errorstatus elf_xen_parse_note(struct elf_binary 
> *elf,
>      case XEN_ELFNOTE_PHYS32_ENTRY:
>          parms->phys_entry = val;
>          break;
> +
> +    case XEN_ELFNOTE_PHYS32_RELOC:
> +        parms->phys_reloc = true;
> +
> +        if ( descsz >= 4 )
> +        {
> +            parms->phys_max = elf_note_numeric_array(elf, note, 4, 0);
> +            elf_msg(elf, " = max: %#"PRIx32, parms->phys_max);

As indicated before, I consider the = here a little odd.

> +        }
> +        if ( descsz >= 8 )
> +        {
> +            parms->phys_min = elf_note_numeric_array(elf, note, 4, 1);
> +            elf_msg(elf, " min: %#"PRIx32, parms->phys_min);
> +        }
> +        if ( descsz >= 12 )
> +        {
> +            parms->phys_align = elf_note_numeric_array(elf, note, 4, 2);
> +            elf_msg(elf, " align: %#"PRIx32, parms->phys_align);
> +        }

I'd like us to reconsider this ordering: I'm inclined to say that MAX isn't
the most likely one a guest may find a need to use. Instead I'd expect both
MIN and ALIGN wanting to be given higher priority; what I'm less certain
about is the ordering between the two. To keep MIN and MAX adjacent, how
about ALIGN, MIN, MAX?

> --- a/xen/include/public/elfnote.h
> +++ b/xen/include/public/elfnote.h
> @@ -194,10 +194,27 @@
>   */
>  #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 maximum address for the entire image to be loaded below (default
> + *      0xffffffff)

"below" isn't exactly true anymore with this now being an inclusive value.
Perhaps "up to", or perhaps more of a re-wording.

I also think the wrapped line's indentation is too deep (by 2 blanks).

> + *  - a minimum address for the start of the image (default 0)
> + *  - a required start alignment (default 0x200000)
> + *
> + *  This note is only valid for x86 binaries.

Maybe s/valid/recognized/ (or honored or some such)?

Jan