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

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

[Jan Beulich]

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@suse.com>
---
v2: Refine an expression. Add comment.
---
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) and an apparently wrong comment stating
that not only v_end but also v_start would be superpage aligned (in fact
v_end is 4MiB aligned, which is the superpage size only on long
abandoned [by us] non-PAE x86-32).

--- a/xen/arch/x86/pv/dom0_build.c
+++ b/xen/arch/x86/pv/dom0_build.c
@@ -59,6 +59,16 @@ static __init void mark_pv_pt_pages_rdon
         l1e_remove_flags(*pl1e, _PAGE_RW);
         page = mfn_to_page(l1e_get_mfn(*pl1e));
 
+        /*
+         * Verify that
+         * - all pages have a valid L1...Ln page table type and
+         * - no other bits are set, in particular the type refcount is still
+         *   zero.
+         */
+        ASSERT((page->u.inuse.type_info & PGT_type_mask) >= PGT_l1_page_table);
+        ASSERT((page->u.inuse.type_info & PGT_type_mask) <= PGT_root_page_table);
+        ASSERT(!(page->u.inuse.type_info & ~PGT_type_mask));
+
         /* Read-only mapping + PGC_allocated + page-table page. */
         page->count_info         = PGC_allocated | 3;
         page->u.inuse.type_info |= PGT_validated | 1;

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

[Andrew Cooper]

On 17/08/2021 15: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@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citirx.com>, thanks.

Are you planning further cleanups here imminently?  If not, I can
probably shuffle some of the easy ROUNDUP() refactoring in the direction
of an intern or grad.

~Andrew

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

[Jan Beulich]

On 17.08.2021 19:25, Andrew Cooper wrote:
> On 17/08/2021 15: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@suse.com>
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@citirx.com>, thanks.
> 
> Are you planning further cleanups here imminently?  If not, I can
> probably shuffle some of the easy ROUNDUP() refactoring in the direction
> of an intern or grad.

Not any cleanup, I don't think, but quite a few further changes to make
Dom0 use large IOMMU page mappings where possible (without introducing
logic yet to un-shatter split pages, not the least because relying on
just that would then undermine the secondary effect of improving boot
time). The two changes posted so far were really just fallout from that
work, with it seeming reasonable to post up front to reduce the size of
the actual series, which has grown quite a bit longer than I though it
would.

Jan