Re: [PATCH] x86/PV: assert page state in mark_pv_pt_pages_rdonly()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.08.2021 21:25, Andrew Cooper wrote:
> On 16/08/2021 16:29, Jan Beulich wrote:
>> About every time I look at dom0_construct_pv()'s "calculation" of
>> nr_pt_pages I question (myself) whether the result is precise or merely
>> an upper bound. I think it is meant to be precise, but I think we would
>> be better off having some checking in place. Hence add ASSERT()s to
>> verify that
>> - all pages have a valid L1...Ln (currently L4) page table type and
>> - no other bits are set, in particular the type refcount is still zero.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> ---
>> There are (at least) two factors supporting my uncertainty about the
>> "calculation" being precise: The loop starting from 2 (which clearly is
>> too small for a possible result)
> 
> 2 was the correct absolute minimum for 2-level guests.

Which has been history for how many years? The minimum for the
current implementation is 4 afaict, and ...

> XTF kernels don't exceed the 2M boundary (at least, not currently), so
> they can be mapped with only 3 or 4 pagetables, except:
> 
> * 3-level guests are created with 4 L2's for no obvious reason.  This is
> nothing to do with legacy PAE paging, nor with how a typical Linux/BSD
> kernel works.  The requirement to make 3-level guests work (and even
> then, only under 32bit Xen) is to create a PGT_pae_xen_l2 if not already
> covered by the other mappings.  Any non-toy kernel discards these
> pagetables in favour of its own idea of pagetables.

... could be 3 for 32-bit Dom0.

> * v_end is rounded up to 4MB.
> 
> Most XTF guests will operate entirely happily in a few hundred kb of
> space, and the same will be true of other microservices.  The rounding
> up of memory might be helpful for the traditional big VMs case, but it
> isn't correct or useful for other usecases.
> 
>> and an apparently wrong comment stating
>> that not only v_end but also v_start would be superpage aligned
> 
> Which comment?  The only one I see about 4M has nothing to do with
> superpages.

The one immediately ahead of the related variable declarations:

    /*
     * This fully describes the memory layout of the initial domain. All
     * *_start address are page-aligned, except v_start (and v_end) which are
     * superpage-aligned.
     */

I see nothing forcing v_start to be superpage-aligned, while I
do suspect that the "calculation" of the number of page tables
will be wrong when it isn't.

>>  (in fact
>> v_end is 4MiB aligned, which is the superpage size only on long
>> abandoned [by us] non-PAE x86-32).
> 
> Tangentially, that code needs some serious work to use ROUNDUP/DOWN
> macros for clarity.

Agreed.

>> --- a/xen/arch/x86/pv/dom0_build.c
>> +++ b/xen/arch/x86/pv/dom0_build.c
>> @@ -59,6 +59,10 @@ static __init void mark_pv_pt_pages_rdon
>>          l1e_remove_flags(*pl1e, _PAGE_RW);
>>          page = mfn_to_page(l1e_get_mfn(*pl1e));
>>  
>> +        ASSERT(page->u.inuse.type_info & PGT_type_mask);
>> +        ASSERT((page->u.inuse.type_info & PGT_type_mask) <= 
>> PGT_root_page_table);
> 
> This is an obfuscated
> 
> ASSERT((page->u.inuse.type_info & PGT_type_mask) >= PGT_l1_page_table &&
>        (page->u.inuse.type_info & PGT_type_mask) <= PGT_root_page_table);

I can certainly switch to this yet longer piece of code, and ...

> and
> 
>> +        ASSERT(!(page->u.inuse.type_info & ~PGT_type_mask));
> 
> this has no context.
> 
> At a bare minimum, you need a comment stating what properties we're
> looking for, so anyone suffering an assertion failure has some clue as
> to what may have gone wrong.

... I can certainly transform the respective parts of the
description into a code comment.

Jan