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

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

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.

> > 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?

> > +         * 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

> Also as a more general remark: While we dislike too long lines,
> too short ones aren't very nice either. Without trying it out I
> can't even be sure the whole comment wouldn't perhaps fit on
> two lines instead of the three it uses right now.

Wilco!

> Finally - please use parentheses in expressions consistently.

Wilco!

Daniel

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