Re: [PATCH v3 2/2] x86/setup: remap Xen image up to PFN_DOWN(__pa(_end))

[Daniel Kiper <daniel.kiper@oracle.com>]

On Tue, Feb 13, 2018 at 02:16:22AM -0700, Jan Beulich wrote:
> >>> On 08.02.18 at 14:46, <daniel.kiper@oracle.com> wrote:
> > Sorry for late reply but I was busy with other stuff.
> >
> > On Fri, Jan 19, 2018 at 08:27:46AM -0700, Jan Beulich wrote:
> >> >>> On 10.01.18 at 14:05, <daniel.kiper@oracle.com> wrote:
> >> > Current limit, PFN_DOWN(xen_phys_start), introduced by commit b280442
> >> > (x86: make Xen early boot code relocatable) is not reliable. Potentially
> >> > its value may fall below PFN_DOWN(__pa(_end)) and then part of Xen image
> >> > may not be mapped after relocation. This will not happen in current code
> >> > thanks to "x86/setup: do not relocate over current Xen image placement"
> >> > patch. Though this safety measure may save a lot of debugging time when
> >> > somebody decide to relax existing relocation restrictions one day.
> >>
> >> I've gone back through the v2 discussion, and I continue to fail to
> >> see what is being fixed here, even if just theoretically. It is bad
> >
> > OK, let's give an example. I assume that there is no patch 1 and Xen can
> > relocate itself even it was initially relocated by the bootloader. So,
> > let's assume that the bootloader loaded Xen image at 0x80200000
> > (xen_phys_start == 0x80000000) and its size is 0x700000 (7 MiB).
> > The RAM region ends at 0x80D00000 and there is no RAM above that
> > address. At some point Xen realizes that it can relocate itself
> > to 0x80600000 (xen_phys_start == 0x80400000). So, it does and then
> > remaps itself. And here is the problem. Currently existing code
> > will remap only Xen image up to 0x803fffff. Everything above will
> > no be remapped. So, that is why I suggested this patch.
> >
> >> enough that the description here isn't clarifying this and I need to
> >> go back to the earlier discussion, but it's even worse if even that
> >> earlier discussion didn't really help. My conclusion is that you're
> >
> > Sorry about that.
> >
> >> talking about a case where old and positions of Xen overlap, a
> >> case which I thought patch 1 eliminates.
> >
> > It does not eliminate the issue described above. It just hides it.
>
> Well, no, I disagree - it makes an overlap impossible afaict,
> which is more that just hiding the problem. Anyway - I'm not
> going to object to the change provided it comes with a clear
> description of what _existing_ issue (even if just a theoretical
> one) is being fixed _with the currently present code in mind_
> (i.e. in particular including your patch 1).

Well, it looks that I have misread the code and I was simply lying.
Sorry about that. However, I had strange feeling that still something
is wrong here. And it is wrong. Currently destination region between
__image_base__ and (__image_base__ + XEN_IMG_OFFSET) may overlap with
the end of source image. And here is the problem. If anything between
__page_tables_start and __page_tables_end lands in the overlap then
some or even all page table entries may not be updated. This means boom
in early boot which will be difficult to the investigate. So, I think
the we have three choices to fix the issue:
  - drop XEN_IMG_OFFSET from
    if ( (end > s) && (end - reloc_size + XEN_IMG_OFFSET >= __pa(_end)) )
  - add XEN_IMG_OFFSET to xen_phys_start in PFN_DOWN(xen_phys_start)
    used in loops as one of conditions,
  - change PFN_DOWN(xen_phys_start) to PFN_DOWN(xen_remap_end_pfn)
    proposed in this patch.

I think that we should choose first option. This way we will avoid
all kinds of overlaps which are always full can of worms.

Daniel

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