Re: [PATCH v4 13/21] IOMMU/x86: prefill newly allocate page tables

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, May 19, 2022 at 02:12:04PM +0200, Jan Beulich wrote:
> On 06.05.2022 13:16, Roger Pau Monné wrote:
> > On Mon, Apr 25, 2022 at 10:40:55AM +0200, Jan Beulich wrote:
> >> ---
> >> An alternative to the ASSERT()s added to set_iommu_ptes_present() would
> >> be to make the function less general-purpose; it's used in a single
> >> place only after all (i.e. it might as well be folded into its only
> >> caller).
> > 
> > I would think adding a comment that the function requires the PDE to
> > be empty would be good.
> 
> But that's not the case - what the function expects to be clear is
> what is being ASSERT()ed.
> 
> >  Also given the current usage we could drop
> > the nr_ptes parameter and just name the function fill_pde() or
> > similar.
> 
> Right, but that would want to be a separate change.
> 
> >> --- a/xen/drivers/passthrough/amd/iommu_map.c
> >> +++ b/xen/drivers/passthrough/amd/iommu_map.c
> >> @@ -115,7 +115,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);
> > 
> > I think PAGETABLE_ORDER would be clearer here.
> 
> I disagree - PAGETABLE_ORDER is a CPU-side concept. It's not used anywhere
> in IOMMU code afaics.

Isn't PAGE_SHIFT also a CPU-side concept in the same way?  I'm not
sure what's the rule for declaring that PAGE_SHIFT is fine to use in
IOMMU code  but not PAGETABLE_ORDER.

Thanks, Roger.