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

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

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@xxxxxxxxxx> 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.

> > Additionally, remapping will execute a bit faster due to this change.
>
> Besides it hardly mattering - how come?

The continue triggered by "l3e_get_pfn(*pl3e) > ..." check will fire
after going above xen_remap_end_pfn instead of PFN_DOWN(xen_phys_start).
And xen_remap_end_pfn is often much less than PFN_DOWN(xen_phys_start).
However, I agree that it hardly matters here. It is just side effect of
this patch. This is not main goal of it.

> > --- a/xen/arch/x86/setup.c
> > +++ b/xen/arch/x86/setup.c
> > @@ -973,6 +973,11 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> >              l3_pgentry_t *pl3e;
> >              l2_pgentry_t *pl2e;
> >              int i, j, k;
> > +            /*
> > +             * We have to calculate xen_remap_end_pfn before
> > +             * xen_phys_start change.
> > +             */
> > +            unsigned long xen_remap_end_pfn = PFN_DOWN(__pa(_end));
>
> In case you can provide a convincing reason for the patch to be
> needed (or at least wanted) - the xen_ prefix is pointless here,
> and you might help the compiler (and maybe also the reader) a
> little by declaring the whole thing const.

OK.

Daniel

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