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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Oct 26, 2023 at 04:55:36PM +0200, Jan Beulich wrote:
> On 26.10.2023 15:58, Xenia Ragiadakou wrote:
> > 
> > On 26/10/23 15:37, Jan Beulich wrote:
> >> On 26.10.2023 14:35, Xenia Ragiadakou wrote:
> >>>
> >>>
> >>> 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.
> >>
> >> It looks like it all boils down to the alternative I did sketch out.
> > 
> > I 'm not sure I fully understood the alternative.
> > Do you mean sth in the lines below?
> > 
> >           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 ( start < kernel_start && end > kernel_end ) {
> > +            if ( kernel_start - start > end - kernel_end )
> > +                end = kernel_start;
> > +            else
> > +                start = kernel_end;
> > +        }
> > +        else if ( start < kernel_start )
> >               end = kernel_start;
> > -        else
> > +        else if ( end > kernel_end )
> >               start = kernel_end;
> > +        else
> > +            continue;
> > 
> >           if ( end - start >= size )
> >               return start;
> 
> Not exactly, no, because this still takes the size into account only
> in this final if().
> 
> > You wouldn't like to consider this approach?
> 
> I'm happy to consider any other approach. Just that ...
> 
> >           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_end - start > end - kernel_start )
> >               end = kernel_start;
> >           else
> >               start = kernel_end;
> > 
> > -        if ( end - start >= size )
> > +        if ( end > start && end - start >= size )
> >               return start;
> >       }
> 
> ... I'm afraid this doesn't deal well with the specific case I was
> mentioning: If the E820 region is fully contained in the kernel range,
> it looks to me as if this approach would ignore the E820 altogether,
> since you either move end ahead of start or start past end then. Both
> head and tail regions may be large enough in this case, and if this
> was the only region above 1M, there'd be no other space to fall back
> to.

I think the only sane option and more robust if we have to start
supporting kernels with a set of scattered program headers physical
addresses is to populate a rangeset with all the RAM ranges,
subtract all memory used by the kernel and then find a suitable
region for the metadata.

Roger.