RE: [PATCH v3 17/23] IOMMU/x86: prefill newly allocate page tables

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Friday, February 18, 2022 4:25 PM
> 
> On 18.02.2022 06:01, Tian, Kevin wrote:
> >> From: Jan Beulich <jbeulich@xxxxxxxx>
> >> Sent: Tuesday, January 11, 2022 12:35 AM
> >>
> >> Page tables are used for two purposes after allocation: They either
> >> start out all empty, or they get filled to replace a superpage.
> >> Subsequently, to replace all empty or fully contiguous page tables,
> >> contiguous sub-regions will be recorded within individual page tables.
> >> Install the initial set of markers immediately after allocation. Make
> >> sure to retain these markers when further populating a page table in
> >> preparation for it to replace a superpage.
> >>
> >> The markers are simply 4-bit fields holding the order value of
> >> contiguous entries. To demonstrate this, if a page table had just 16
> >> entries, this would be the initial (fully contiguous) set of markers:
> >>
> >> index  0 1 2 3 4 5 6 7 8 9 A B C D E F
> >> marker 4 0 1 0 2 0 1 0 3 0 1 0 2 0 1 0
> >>
> >> "Contiguous" here means not only present entries with successively
> >> increasing MFNs, each one suitably aligned for its slot, but also a
> >> respective number of all non-present entries.
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> >
> > Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>, with a nit:
> 
> Thanks.
> 
> >> @@ -478,7 +478,28 @@ struct page_info *iommu_alloc_pgtable(st
> >>          return NULL;
> >>
> >>      p = __map_domain_page(pg);
> >> -    clear_page(p);
> >> +
> >> +    if ( contig_mask )
> >> +    {
> >> +        unsigned int i, shift = find_first_set_bit(contig_mask);
> >> +
> >> +        ASSERT(((PAGE_SHIFT - 3) & (contig_mask >> shift)) == PAGE_SHIFT -
> 3);
> >> +
> >> +        p[0] = (PAGE_SHIFT - 3ull) << shift;
> >> +        p[1] = 0;
> >> +        p[2] = 1ull << shift;
> >> +        p[3] = 0;
> >> +
> >> +        for ( i = 4; i < PAGE_SIZE / 8; i += 4 )
> >> +        {
> >> +            p[i + 0] = (find_first_set_bit(i) + 0ull) << shift;
> >> +            p[i + 1] = 0;
> >> +            p[i + 2] = 1ull << shift;
> >> +            p[i + 3] = 0;
> >> +        }
> >
> > some comment similar to what commit msg describes can improve
> > the readability here.
> 
> I wouldn't want to replicate what pt-contig-markers.h describes, so
> maybe a comment referring there would do?
> 

sounds good.