Re: [PATCH v7 2/2] x86/PVH: Support relocatable dom0 kernels

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.04.2024 18:56, Jason Andryuk wrote:
> On 2024-04-08 03:00, Jan Beulich wrote:
>> On 04.04.2024 23:25, Jason Andryuk wrote:
>>> --- a/xen/arch/x86/hvm/dom0_build.c
>>> +++ b/xen/arch/x86/hvm/dom0_build.c
>>> @@ -537,6 +537,111 @@ 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 = (uintptr_t)elf->dest_base;
>>> +    paddr_t kernel_end = kernel_start + elf->dest_size;
>>> +    unsigned int i;
>>
>> While properly typed here, ...
>>
>>> +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 align;
>>> +    int i;
>>
>> ... I must have missed when this was changed to plain int. It should have
>> been unsigned int here, too, ...
>>
>>> +    if ( parms->phys_align != UNSET_ADDR32 )
>>> +        align = parms->phys_align;
>>> +    else if ( elf->palign >= PAGE_SIZE )
>>> +        align = elf->palign;
>>> +    else
>>> +        align = MB(2);
>>> +
>>> +    /* Search backwards to find the highest address. */
>>> +    for ( i = d->arch.nr_e820 - 1; i >= 0 ; i-- )
>>
>> ... with this suitably adjusted. However, I'm not going to change this while
>> committing, to avoid screwing up.
> 
> I intentionally changed this.  Looping downwards, a signed int allows 
> writing the check naturally with i >= 0.  I think it's clearer when 
> written this way.

Just to clarify: Is

    for ( i = d->arch.nr_e820; i--; )

any less clear? (While replying I also notice there's a stray blank
in the for() you have, ahead of the 2nd semicolon.)

Jan