Re: [PATCH v2 15/18] IOMMU/x86: prefill newly allocate page tables

["Roger Pau Monné" <roger.pau@citrix.com>]

On Tue, Dec 14, 2021 at 04:05:27PM +0100, Jan Beulich wrote:
> On 14.12.2021 15:50, Roger Pau Monné wrote:
> > On Fri, Sep 24, 2021 at 11:54:58AM +0200, Jan Beulich wrote:
> >> --- a/xen/drivers/passthrough/amd/iommu-defs.h
> >> +++ b/xen/drivers/passthrough/amd/iommu-defs.h
> >> @@ -445,6 +445,8 @@ union amd_iommu_x2apic_control {
> >>  #define IOMMU_PAGE_TABLE_U32_PER_ENTRY	(IOMMU_PAGE_TABLE_ENTRY_SIZE / 4)
> >>  #define IOMMU_PAGE_TABLE_ALIGNMENT	4096
> >>  
> >> +#define IOMMU_PTE_CONTIG_MASK           0x1e /* The ign0 field below. */
> > 
> > Should you rename ign0 to contig_mask or some such now?
> 
> Not sure. I guess I should attach a comment linking here.
> 
> > Same would apply to the comment next to dma_pte for VT-d, where bits
> > 52:62 are ignored (the comments seems to be missing this already) and
> > we will be using bits 52:55 to store the contiguous mask for the
> > entry.
> 
> Same here then.

I would prefer that.

> >> --- a/xen/drivers/passthrough/amd/iommu_map.c
> >> +++ b/xen/drivers/passthrough/amd/iommu_map.c
> >> @@ -116,7 +116,19 @@ static void set_iommu_ptes_present(unsig
> >>  
> >>      while ( nr_ptes-- )
> >>      {
> >> -        set_iommu_pde_present(pde, next_mfn, 0, iw, ir);
> >> +        ASSERT(!pde->next_level);
> >> +        ASSERT(!pde->u);
> >> +
> >> +        if ( pde > table )
> >> +            ASSERT(pde->ign0 == find_first_set_bit(pde - table));
> >> +        else
> >> +            ASSERT(pde->ign0 == PAGE_SHIFT - 3);
> > 
> > You could even special case (pde - table) % 2 != 0, but this is debug
> > only code, and it's possible a mod is more costly than
> > find_first_set_bit.
> 
> Not sure why I would want to special case anything that doesn't need
> special casing. The pde == table case needs special care because there
> find_first_set_bit() cannot be called.

Well in iommu_alloc_pgtable you already special case odd entries by
explicitly setting the mask to 0 instead of using find_first_set_bit.

> >> @@ -450,7 +450,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;
> >> +        }
> > 
> > You could likely do:
> > 
> > for ( i = 0; i < PAGE_SIZE / 8; i += 4 )
> > {
> >     p[i + 0] = i ? ((find_first_set_bit(i) + 0ull) << shift)
> >                  : ((PAGE_SHIFT - 3ull) << shift);
> >     p[i + 1] = 0;
> >     p[i + 2] = 1ull << shift;
> >     p[i + 3] = 0;
> > }
> > 
> > To avoid having to open code the first loop iteration.
> 
> I could, but I explicitly didn't want to. I consider conditionals
> inside a loop which special case just the first (or last) iteration
> quite odd (unless they really save a lot of duplication).
> 
> > The ternary
> > operator could also be nested before the shift, but I find that
> > harder to read.
> 
> If I was to make the change, then that alternative way, as it would
> allow to avoid the addition of 0ull:
> 
>     p[i + 0] = (i ? find_first_set_bit(i)
>                   : PAGE_SHIFT - 3ull) << shift;
> 
> I could be talked into going that route (but not the intermediate
> one you've suggested).

If you prefer to open code the first iteration I'm also fine with
that.

Thanks, Roger.