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. Jan
