Re: [PATCH v3 3/3] x86/PVH: Support relocatable dom0 kernels

[Jason Andryuk <jason.andryuk@xxxxxxx>]

On 2024-03-20 10:39, Jan Beulich wrote:
> On 19.03.2024 21:58, Jason Andryuk wrote:
> > --- a/xen/arch/x86/hvm/dom0_build.c
> > +++ b/xen/arch/x86/hvm/dom0_build.c
> > @@ -537,6 +537,97 @@ 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 = (paddr_t)elf->dest_base;
> As before I think it is illegitimate to cast a pointer to paddr_t. The
> variable type wants to remain such, but the cast wants to be to
> unsigned long or uintptr_t (or else at least a comment wants adding).
Ok, thanks.  This explains it clearly.

> > +/* 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 = d->arch.e820[i].addr + 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);
> I'd generally try to avoid max_t(), but I cannot think of any good way
> of expressing this without using it.
>
> > +        kend = kstart + kernel_size;
> > +
> > +        if ( kend > parms->phys_max )
> So despite its default phys_max is exclusive aiui. Otherwise with
> kend being exclusive too, this would look to be off by one.
Yes, I'll fix the off-by-one.  Hmmm, phys_max being 32bit, we want it to 
be inclusive to represent the maximum range.
> > +            return 0;
> > +
> > +        if ( kend <= end )
> > +            return kstart;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +/* Check the kernel load address, and adjust if necessary and possible. */
> > +static bool __init check_and_adjust_load_address(
> > +    const struct domain *d, struct elf_binary *elf, struct elf_dom_parms 
> > *parms)
> > +{
> > +    paddr_t reloc_base;
> > +
> > +    if ( check_load_address(d, elf) )
> > +        return true;
> > +
> > +    if ( !parms->phys_reloc )
> > +    {
> > +        printk("%pd kernel: Address conflict and not relocatable\n", d);
> > +        return false;
> This better wouldn't result in -ENOMEM in the caller. Then again reasons
> are logged, so the specific error code perhaos doesn't matter that much.
Failure here is turned into -ENOMEM by pvh_load_kernel().   -ENOMEM is 
already returned for later failure with find_memory(), so I thought it 
was acceptable.  Without this code, elf_load_binary() would failed with 
-1 and that would be returned.  I'll change it to whatever you prefer.
> > +    }
> > +
> > +    reloc_base = find_kernel_memory(d, elf, parms);
> > +    if ( reloc_base == 0 )
> With ! used above please be consistent and do so here, too.
phys_reloc is a bool, and reloc_base is a paddr_t.

> > +    {
> > +        printk("%pd kernel: Failed find a load address\n", d);
> > +        return false;
> > +    }
> > +
> > +    if ( opt_dom0_verbose )
> > +        printk("%pd kernel: Moving [%p, %p] -> [%"PRIpaddr", %"PRIpaddr"]\n", 
> > d,
> > +               elf->dest_base, elf->dest_base + elf->dest_size - 1,
> > +               reloc_base, reloc_base + elf->dest_size - 1);
> > +
> > +    parms->phys_entry = reloc_base +
> > +                            (parms->phys_entry - (paddr_t)elf->dest_base);
> > +    elf->dest_base = (char *)reloc_base;
> Just as a remark, no request to change anything: We're not dealing with
> strings here. Hence char * isn't quite right, just that "dest_base" is
> of that type (for whatever reason).
I think the reason is just to be byte addressable.

> > @@ -225,6 +232,28 @@ 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);
> > +        }
> > +        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);
> > +        }
> > +
> > +        elf_msg(elf, "\n");
> Imo the newline wants adding outside of the switch(), for everything
> set to ELFNOTE_CUSTOM. In fact with that I think ELFNOTE_CUSTOM and
> ELFNOTE_NAME could be folded (with what's NAME right now simply
> printing nothing in the switch here). Which suggests that I may
> better not commit the (slightly adjusted) earlier patch, yet.
Yes, please hold off and I'll re-work both.

> > @@ -536,6 +565,10 @@ elf_errorstatus elf_xen_parse(struct elf_binary *elf,
> >       parms->p2m_base = UNSET_ADDR;
> >       parms->elf_paddr_offset = UNSET_ADDR;
> >       parms->phys_entry = UNSET_ADDR32;
> > +    parms->phys_min = 0;
> > +    parms->phys_max = 0xffffffff;
> > +    parms->phys_align = 0x200000;
> I think this wants to be MB(2) (requiring a pre-patch to centralize MB()
> in the tool stack to tools/include/xen-tools/common-macros.h). And I
> further think this needs to be an arch-specific constant, even if right
> now the note is expected to be present only for x86. Which then also
> needs saying ...
Ok, I'll look into this.

> > --- a/xen/include/public/elfnote.h
> > +++ b/xen/include/public/elfnote.h
> > @@ -194,10 +194,26 @@
> >    */
> >   #define XEN_ELFNOTE_PHYS32_ENTRY 18
> >   
> > +/*
> > + * Physical loading constraints for PVH kernels
> > + *
> > + * Used to place constraints on the guest physical loading addresses and
> > + * alignment for a PVH kernel.
> > + *
> > + * The presence of this note indicates the kernel supports relocating itself.
> > + *
> > + * The note may include up to three 32bit values in the following order:
> > + *  - a maximum address for the entire image to be loaded below (default
> > + *      0xffffffff)
> > + *  - a minimum address for the start of the image (default 0)
> > + *  - a required start alignment (default 0x200000)
> > + */
> > +#define XEN_ELFNOTE_PHYS32_RELOC 19
> ... in the comment here. The reasoning behind not (intermediately) falling
> back to the alignment in the ELF headers also wants adding to the patch
> description (or a code comment, albeit there's no really good place to put
> it, imo).
>
> Additionally I think the pieces of the comment want re-ordering. The primary
> purpose is indicating relocatability; when not relocating, the constraints
> aren't applied in any way.
Yes, this is a good point.

Thanks for the review.

-Jason