Re: [Xen-devel] [PATCH] x86/setup: do not relocate below the end of current Xen image placement

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.11.17 at 13:41, <daniel.kiper@xxxxxxxxxx> wrote:
> On Mon, Nov 27, 2017 at 09:51:56AM -0700, Jan Beulich wrote:
>> >>> On 27.11.17 at 16:41, <daniel.kiper@xxxxxxxxxx> wrote:
>> > If it is possible we would like to have the Xen image higher than the
>> > booloader put it and certainly do not overwrite the Xen code and data
>> > during copy/relocation. Otherwise the Xen may crash silently at boot.
>>
>> Is this something that you've actually observed happening? I'd be
>> curious about the particular numbers if so.
> 
> We were hit by the issue in OVS Xen 4.4 with my earlier version of
> EFI/Multiboot2 patches. Initially its implementation allowed relocation
> of Xen even if it was relocated by the bootloader. This led to the
> crashes on some new Oracle machines because copy destination partially
> overlapped with the end of current/initial Xen image placement.
> 
> After some discussion here we decided to disable Xen relocation in my
> EFI/Multiboot2 upstream patches if the booloader did the work for us.
> Though one case is still not covered. If Xen is not relocated by the
> booloader then it tries to do that by itself. If all RAM regions above
> currently occupied one are unsuitable for relocation then Xen tries move
> itself higher in it. And if (end - reloc_size + XEN_IMG_OFFSET) goes
> below _end then copy/relocation destination overlaps, at least partially,
> with its source.
> 
> I can agree that this should not happen on todays machines very often.
> If at all. It is rather unusual to not have usable RAM regions above
> ~5 MiB nowadays. Though I think that we should at least consider putting
> such safety measure here. Otherwise Xen may crash mysteriously without
> any stack trace. So, as you may imagine it is very confusing and impairs
> further debugging. However, due to rarity of the issue I am not going to
> insist very strongly here.

I don't mind taking care of this case, as long as everything is
being properly described.

>> > Signed-off-by: Daniel Kiper <daniel.kiper@xxxxxxxxxx>
>> > ---
>> >  xen/arch/x86/setup.c |    8 +++++++-
>> >  1 file changed, 7 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>> > index 32bb02e..be91d34 100644
>> > --- a/xen/arch/x86/setup.c
>> > +++ b/xen/arch/x86/setup.c
>> > @@ -960,7 +960,13 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>> >          }
>> >          else
>> >              end = 0;
>> > -        if ( end > s )
>> > +
>> > +        /*
>> > +         * Is the region size greater than zero?
>>
>> Why "greater than zero"?
> 
> end > s?

Oh, I had wrongly assumed the comment was meant to cover only
the addition you're making to the condition.

>> > +         * Does the region begins above or at the
>> > +         * end of current Xen image placement?
>> > +         */
>> > +        if ( end > s && (end - reloc_size >= _end - _start) )
>>
>> And how does this added condition effect Xen only being moved
>> upwards? _end - _start after all is only the Xen image size, with
>> no consideration of its position. Plus I think you really mean
>> __2M_rwdata_end instead of _end.
> 
> OK, we have below:
> 
> [...]
> 
> e = end - reloc_size;
> 
> [...]
> 
> move_memory(e + XEN_IMG_OFFSET, XEN_IMG_OFFSET, _end - _start, 1);
> 
> So, we have: e + XEN_IMG_OFFSET >= XEN_IMG_OFFSET + _end - _start
> 
> We can drop XEN_IMG_OFFSET, so we have: e >= _end - _start
> 
> However, we cannot use "e" in the condition, so we have:
>   end - reloc_size >= _end - _start

Okay, so this implies an original base address of zero. In that case,
however, we can't possibly move Xen downwards; looks like I've
misunderstood the title/description: You're really worried about an
overlap of the regions. From description and comment I was getting
the impression that the goal would be to cover more cases (including
ones where Xen wasn't loaded at the lowest possible address).
Could you try to make this more clear?

Furthermore I'm not convinced the condition needs to be as strict
as you make it now: As long as move_memory() can deal with
overlapping areas, a partial overlap looks to be okay. That would
allow for more cases where Xen does actually get moved even
with very little memory.

Another option is to consider not moving Xen based on other
criteria: The main goal here is to free up memory below 16Mb. If
the machine has no memory above 16Mb, moving Xen around
won't do any good in this regard.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel