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.