Re: [Xen-devel] [PATCH v6] x86/setup: properly update PTEs if src/dst overlaps when relocating Xen image

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Thu, Apr 19, 2018 at 09:05:35AM -0600, Jan Beulich wrote:
> >>> On 19.04.18 at 16:28, <daniel.kiper@xxxxxxxxxx> wrote:
> > Commit 0d31d16 (x86/setup: do not relocate Xen over current Xen image
> > placement) disallowed src/dst images overlaps when relocating Xen image.
> > Though it deliberately allowed destination region between __image_base__
> > and (__image_base__ + XEN_IMG_OFFSET) overlaps with the end of source
> > image. And here is the problem. If anything between __page_tables_start
> > and __page_tables_end in source image lands in the overlap then some or
> > even all page table entries may not be updated. This usually means boom
> > in early boot which will be difficult to the investigate. So, I think
> > that 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 and replace ">" with ">=",
> >   - change PFN_DOWN(xen_phys_start) to PFN_DOWN(xen_remap_end_pfn)
> >     proposed in earlier version of this patch.
> >
> > This patch implements the second option. This way we still allow source
> > and destination partial overlap as described above but PTEs are properly
> > updated now.
> >
> > Signed-off-by: Daniel Kiper <daniel.kiper@xxxxxxxxxx>
>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> and queuing for after 4.11.

Thank you.

> One further small suggestion though:
>
> > @@ -1019,6 +1020,15 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> >              bootsym(trampoline_xen_phys_start) = e;
> >
> >              /*
> > +             * All PTEs pointing above that address are not candidates for
> > +             * relocation. Due to possibility of partial overlap of the end
> > +             * of source image and the beginning of region for destination
> > +             * image some PTEs may point to addresses in
> > +             * range [e, e + XEN_IMG_OFFSET).
> > +             */
> > +            pte_update_limit = PFN_DOWN(e + XEN_IMG_OFFSET);
>
> Perhaps better start with "No PTEs pointing above this address are
> candidates ..."?

I am OK with that change. Should I post v7 or are you going to update
the comment before committing? Both works for me.

Daniel

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