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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

> Currently, the code proceeds with the dom0 kernel being corrupted.

And we agree that this wants fixing.

Jan