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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.04.2024 16:34, Jason Andryuk wrote:
> On 2024-04-17 09:24, Jan Beulich wrote:
>> 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?
> 
> It's not something I normally write, so I had to think about it more. If 
> you are already familiar with such a construct, then that isn't an issue 
> for you.
> 
> Your way is more subtle in my opinion because it relies on the post 
> decrement to ensure correct bounds within the loop body.  I prefer i >= 
> 0 because it clearly states the valid index values.
> 
> Is your main concern that you only want unsigned values as array indices?

Yes. Besides eliminating any concerns towards possible underruns, that also
often allows the compiler to produce better code.

Jan