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

[Jason Andryuk <jason.andryuk@xxxxxxx>]

On 2024-03-26 03:50, Jan Beulich wrote:
> 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.
It is not particularly intentional.  I'll look into locating at a higher 
address.
> > --- 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.
Yes, sorry.  I inadvertently retained them when reworking this.

> > @@ -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.
I retained = for consistency with other notes:
ELF: note: PHYS32_RELOC = max: 0x40000000 min: 0x1000000 align: 0x200000
ELF: note: PHYS32_ENTRY = 0x1000000
ELF: note: GUEST_OS = "linux"

I guess whitespace and labels makes it clear, so I'll drop the '='.

> > +        }
> > +        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?
ALIGN, MIN, MAX works for me.

On the Linux side, I'm expecting them all to be set:
ALIGN = CONFIG_PHYSICAL_ALIGN
MIN = LOAD_PHYSICAL_ADDR
MAX = KERNEL_IMAGE_SIZE

You need enough identity page tables to cover up to MAX. 
LOAD_PHYSICAL_ADDR is used as a minimum, so requesting placement above 
MIN makes sense to me.
> > --- 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.
Yes, good point.

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

> > + *  - 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)?
Would a comment at the top of the file saying Notes are only used with 
x86 be better instead of this one-off comment?  Roger already said that, 
and elf_xen_note_check() has a successful early exit with "ELF: Not 
bothering with notes on ARM\n"
Thanks,
Jason