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

[Andrew Cooper <amc96@xxxxxxxx>]

On 07/12/2021 11:37, Jan Beulich wrote:
> 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.

I don't consider that commit relevant.

There is no circumstance ever where you can relocate Xen with
actually-overlapping ranges, because one way or another, you'd end up
copying non-pagetable data over the live pagetables.

That particular commit was part of trying to make Xen's entry code
relocatable, so the MB2 path could load Xen at somewhere which wasn't 0,
but to this day we still skip any physical relocation if Xen isn't
started at 0.


To the comment specifically, it's actively wrong.

Back when XEN_IMG_OFFSET was 1M, and we had 16M worth of unconditional
mappings, then we could get PTE overlap as described, in the corner case
where we were moving Xen from 0 to anywhere between 4 and 16M physical
(2M physical was in principle a problem, but not an eligible position to
relocate to, given Xen's compile size).

And in that corner case, the logic would "corrupt" various PTEs by not
relocating them.  The PTE coving _start at 1M was special cased ahead of
the 2nd loop (finally fixed last week) and the PTEs mapping beyond _end
were unused which is why nothing actually went wrong.

~Andrew