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.
