Re: [PATCH 1/3] x86/boot: Drop pte_update_limit from physical relocation logic

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.12.2021 11:53, Andrew Cooper wrote:
> This check has existed in one form or another since c/s 369bafdb1c1 "xen: Big
> changes to x86 start-of-day" in 2007.  Even then, AFAICT, it wasn't necessary
> because there was a clean split between the statically created entries (L3 and
> higher) and the dynamically created ones (L2 and lower).
> 
> Without dissecting the myriad changes over the past 14 years, I'm pretty
> certain Xen only booted by accident when l2_xenmap[0] was handled specially
> and skipped the pte_update_limit check which would have left it corrupt.
> 
> Nevertheless, as of right now, all non-leaf PTEs (the first loop), and all 2M
> xenmap leaf mappings (the second loop) need relocating.  They are no unused
> mappings in the range, no mappings which will be encountered multiple times,
> and it is unlikely that such mappings would be introduced.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

However, as to the description and ...

> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1230,7 +1230,6 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>              l3_pgentry_t *pl3e;
>              l2_pgentry_t *pl2e;
>              int i, j, k;
> -            unsigned long pte_update_limit;
>  
>              /* Select relocation address. */
>              xen_phys_start = end - reloc_size;
> @@ -1238,14 +1237,6 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>              bootsym(trampoline_xen_phys_start) = xen_phys_start;
>  
>              /*
> -             * No PTEs pointing above this address are 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);

... considering the comment here: Isn't it 0d31d1680868 ("x86/setup: do
not relocate Xen over current Xen image placement") where need for this
disappeared? Afaict the non-overlap of source and destination is the
crucial factor here, yet your description doesn't mention this aspect at
all. I'd therefore like to ask for an adjustment there.

Jan