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

[Jason Andryuk <jason.andryuk@amd.com>]

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