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

[Xenia Ragiadakou <xenia.ragiadakou@xxxxxxx>]

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:
> > > > Given that start < kernel_end and end > kernel_start, the logic that
> > > > determines the best placement for dom0 initrd and metadata, does not
> > > > take into account the two cases below:
> > > > (1) start > kernel_start && end > kernel_end
> > > > (2) start < kernel_start && end < kernel_end
> > > >
> > > > In case (1), the evaluation will result in end = kernel_start
> > > > i.e. end < start, and will load initrd in the middle of the kernel.
> > > > In case (2), the evaluation will result in start = kernel_end
> > > > i.e. end < start, and will load initrd at kernel_end, that is out
> > > > of the memory region under evaluation.
> > > >           
> > > I agree there is a problem if the kernel range overlaps but is not fully
> > > contained in the E820 range under inspection. I'd like to ask though
> > > under what conditions that can happen, as it seems suspicious for the
> > > kernel range to span multiple E820 ranges.
> > >         
> > We tried to boot Zephyr as pvh dom0 and its load address was under 1MB.
> >
> > I know ... that maybe shouldn't have been permitted at all, but 
> > nevertheless we hit this issue.
> >       
> How can they expect to have a contiguous, large enough range there?
>     
    Me too I wonder. I don't know, maybe somebody else could shed
    some light on this.


> > > > This patch rephrases the if condition to include the above two cases
> > > > without affecting the behaviour for the case where
> > > > start < kernel_start && end > kernel_end
> > > >
> > > > Fixes: 73b47eea2104 ('x86/dom0: improve PVH initrd and metadata placement')
> > > > Signed-off-by: Xenia Ragiadakou<xenia.ragiadakou@xxxxxxx>
> > > > ---
> > > >   xen/arch/x86/hvm/dom0_build.c | 2 +-
> > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
> > > > index c7d47d0d4c..5fc2c12f3a 100644
> > > > --- 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?

> Jan
>