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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.10.2023 13:18, Xenia Ragiadakou wrote:
> 
> On 27/10/23 09:37, Jan Beulich wrote:
>> On 26.10.2023 18:55, Xenia Ragiadakou wrote:
>>>
>>>
>>> On 26/10/23 17:55, 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.
>>>
>>> Yes, in which case it will fail. This is legitimate.
>>
>> Not really, if there is space available (but just not properly used).
> 
> I said so because I noticed that, if, for instance, the loading address 
> conflicts with a reserved memory region, xen won't attempt to relocate 
> the kernel (assuming that it is relocatable). It will fail.

Hmm, if so, perhaps yet something else to deal with.

>>> Currently, the code proceeds with the dom0 kernel being corrupted.
>>
>> And we agree that this wants fixing.
> 
> Ok, and IIUC, using rangeset as per Roger's suggestion, right?

Going that route would be optimal of course, but I for one wouldn't
insist.

Jan