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

[Jason Andryuk <jason.andryuk@xxxxxxx>]

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?

> (While replying I also notice there's a stray blank
> in the for() you have, ahead of the 2nd semicolon.)
Sorry about that.

Regards,
Jason