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

From: Jan Beulich <jbeulich@suse.com>
To: Jason Andryuk <jason.andryuk@amd.com>
Cc: "Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	"George Dunlap" <george.dunlap@citrix.com>,
	"Julien Grall" <julien@xen.org>,
	"Stefano Stabellini" <sstabellini@kernel.org>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH v2 3/3] x86/PVH: Support relocatable dom0 kernels
Date: Fri, 15 Mar 2024 10:48:05 +0100	[thread overview]
Message-ID: <88bd8577-42e6-4087-9888-00cd73e7f0bc@suse.com> (raw)
In-Reply-To: <7506abe0-e3d4-44f7-b54d-592ae2e3fd3e@amd.com>

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.

>>> +/* 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?

Jan