Re: [PATCH] x86/hvm/dom0: fix PVH initrd and metadata placement

[Xenia Ragiadakou <xenia.ragiadakou@xxxxxxx>]

On 26/10/23 14:51, Jan Beulich wrote:
> On 26.10.2023 11:46, Xenia Ragiadakou wrote:
> > On 26/10/23 11:45, Jan Beulich wrote:
> > > On 26.10.2023 10:34, Xenia Ragiadakou wrote:
> > > > On 26/10/23 10:35, Jan Beulich wrote:
> > > > > On 26.10.2023 08:45, Xenia Ragiadakou wrote:
> > > > > > --- a/xen/arch/x86/hvm/dom0_build.c
> > > > > > +++ b/xen/arch/x86/hvm/dom0_build.c
> > > > > > @@ -518,7 +518,7 @@ static paddr_t __init find_memory(
> > > > > >             if ( end <= kernel_start || start >= kernel_end )
> > > > > >                 ; /* No overlap, nothing to do. */
> > > > > >             /* Deal with the kernel already being loaded in the region. */
> > > > > > -        else if ( kernel_start - start > end - kernel_end )
> > > > > > +        else if ( kernel_start + kernel_end > start + end )
> > > > > What meaning has the sum of the start and end of either range? I can't
> > > > > figure how comparing those two values will be generally correct / useful.
> > > > > If the partial-overlap case needs handling in the first place, I think
> > > > > new conditionals need adding (and the existing one needs constraining to
> > > > > "kernel range fully contained") to use
> > > > > - as before, the larger of the non-overlapping ranges at start and end
> > > > >      if the kernel range is fully contained,
> > > > > - the tail of the range when the overlap is at the start,
> > > > > - the head of the range when the overlap is at the end.
> > > > Yes it is not quite straight forward to understand and is based on the
> > > > assumption that end > kernel_start and start < kernel_end, due to
> > > > the first condition failing.
> > > >
> > > > Both cases:
> > > > (start < kernel_start && end < kernel_end) and
> > > > (kernel_start - start > end - kernel_end)
> > > > fall into the condition ( kernel_start + kernel_end > start + end )
> > > >
> > > > And both the cases:
> > > > (start > kernel_start && end > kernel_end) and
> > > > (end - kernel_end > kernel_start - start)
> > > > fall into the condition ( kernel_start + kernel_end < start + end )
> > > >
> > > > ... unless of course I miss a case
> > > Well, mathematically (i.e. ignoring the potential for overflow) the
> > > original expression and your replacement are identical anyway. But
> > > overflow needs to be taken into consideration, and hence there is a
> > > (theoretical only at this point) risk with the replacement expression
> > > as well. As a result I still think that ...
> > >
> > > > > That said, in the "kernel range fully contained" case it may want
> > > > > considering to use the tail range if it is large enough, rather than
> > > > > the larger of the two ranges. In fact when switching to that model, we
> > > > > ought to be able to get away with one less conditional, as then the
> > > > > "kernel range fully contained" case doesn't need treating specially.
> > > ... this alternative approach may want considering (provided we need
> > > to make a change in the first place, which I continue to be
> > > unconvinced of).
> > Hmm, I see your point regarding the overflow.
> > Given that start < kernel_end and end > kernel_start, this could
> > be resolved by changing the above condition into:
> > if ( kernel_end - start > end - kernel_start )
> >
> > Would that work for you?
> That would look quite a bit more natural, yes. But I don't think it covers
> all cases: What if the E820 range is a proper sub-range of the kernel one?
> If we consider kernel range crossing E820 region boundaries, we also need
> to take that possibility into account, I think.
You are right, this case is not handled and can lead to either of the 
issues mentioned in commit message.
Maybe we should check whether end > start before proceeding with 
checking the size.
> Jan