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

[Jason Andryuk <jason.andryuk@xxxxxxx>]

On 2024-03-15 05:48, Jan Beulich wrote:
> On 14.03.2024 20:19, Jason Andryuk wrote:
> > On 2024-03-14 09:31, Jan Beulich wrote:
> > > On 13.03.2024 20:30, Jason Andryuk wrote:
> > > > --- a/xen/arch/x86/hvm/dom0_build.c
> > > > +++ b/xen/arch/x86/hvm/dom0_build.c
> > > > @@ -537,6 +537,108 @@ 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 & PAGE_MASK;
> > > > +    paddr_t kernel_end = PAGE_ALIGN((paddr_t)elf->dest_base + elf->dest_size);
> > > Both casts act on a pointer value. Such cannot legitimately be converted
> > > to paddr_t; it only so happens that paddr_t is effectively the same as
> > > uintptr_t right now. (Same issue again further down.) That said, I notice
> > > we have pre-existing examples of this ...
> > Yes, I followed existing code.  Do you want dest_base to be switched to
> > a uintptr_t?
> I think it was deliberately switched to being a pointer at some point,
> maybe even in a security fix.
commit 65808a8ed41cc7c044f588bd6cab5af0fdc0e029 "libelf: check all 
pointer accesses", part of XSA-55, switched from a single dest pointer 
to dest_base & dest_size.
For libxenguest, it's a pointer to guest-mapped memory to copy in the 
kernel.  For PV dom0 creation, it's a pointer - Xen switches to the dom0 
page tables and performs the copy with it as-is.  For PVH dom0, it's a 
guest physical address.
Are you looking for this to be addressed in this series?

> > > > +/* 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_align == UNSET_ADDR )
> > > > +    {
> > > > +        printk("Address conflict and %pd kernel is not relocatable\n", d);
> > > > +        return false;
> > > > +    }
> > > > +
> > > > +    reloc_base = find_kernel_memory(d, elf, parms);
> > > > +    if ( reloc_base == 0 )
> > > > +    {
> > > > +        printk("Failed find a load address for the kernel\n");
> > > > +        return false;
> > > > +    }
> > > > +
> > > > +    if ( opt_dom0_verbose )
> > > > +        printk("Relocating kernel from [%lx, %lx] -> [%lx, %lx]\n",
> > > > +               (paddr_t)elf->dest_base,
> > > > +               (paddr_t)elf->dest_base + elf->dest_size,
> > > By using %p you clearly can avoid the casts here.
> > Ok.
> >
> > > > +               reloc_base, reloc_base + elf->dest_size);
> > > I'm not convinced %lx is really appropriate for paddr_t.
> > PRIpaddr exists.  It's "016lx" for x86.  Using that and %p add lots of 0s:
> > (XEN) Relocating kernel from [0000000001000000, 000000000202ffff] ->
> > [0000000002200000, 000000000322ffff]
> That's to be accepted, I guess.
>
> Looking at it again, is "Relocating" in the log message perhaps misleading?
> We don't relocate it, that's something the kernel itself has to do. We only
> put it at other than the default position. Maybe "Moving" instead?
Yes, "Moving" sounds better.  I guess I'll drop the "from" and insert a 
%pd for:
(XEN) Moving d0 kernel [0000000001000000, 000000000202ffff] -> 
[0000000002200000, 000000000322ffff]
Regards,
Jason