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

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

On Fri, May 20, 2022 at 04:28:14PM +0200, Roger Pau Monné wrote:
> On Fri, May 20, 2022 at 02:36:02PM +0200, Jan Beulich wrote:
> > On 20.05.2022 14:22, Roger Pau Monné wrote:
> > > On Fri, May 20, 2022 at 01:13:28PM +0200, Jan Beulich wrote:
> > >> On 20.05.2022 13:11, Jan Beulich wrote:
> > >>> On 20.05.2022 12:47, Roger Pau Monné wrote:
> > >>>> 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:
> > >>>>>>> --- 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.
> > >>>
> > >>> Hmm, yes and no. But for consistency with other IOMMU code I may want
> > >>> to switch to PAGE_SHIFT_4K.
> > >>
> > >> Except that, with the plan to re-use pt_update_contig_markers() for CPU-
> > >> side re-coalescing, there I'd prefer to stick to PAGE_SHIFT.
> > > 
> > > Then can PAGETABLE_ORDER be used instead of PAGE_SHIFT - 3?
> > 
> > pt_update_contig_markers() isn't IOMMU code; since I've said I'd switch
> > to PAGE_SHIFT_4K in IOMMU code I'm having a hard time seeing how I could
> > at the same time start using PAGETABLE_ORDER there.
> 
> I've got confused by the double reply and read it as if you where
> going to stick to using PAGE_SHIFT everywhere as proposed originally.
> 
> > What I maybe could do is use PTE_PER_TABLE_SHIFT in AMD code and
> > LEVEL_STRIDE in VT-d one. Yet I'm not sure that would be fully correct/
> > consistent, ...
> > 
> > > IMO it makes the code quite easier to understand.
> > 
> > ... or in fact helping readability.
> 
> Looking at pt_update_contig_markers() we hardcode CONTIG_LEVEL_SHIFT
> to 9 there, which means all users must have a page table order of 9.
> 
> It seems to me we are just making things more complicated than
> necessary by trying to avoid dependencies between CPU and IOMMU
> page-table sizes and definitions, when the underlying mechanism to set
> ->ign0 has those assumptions baked in.
> 
> Would it help if you introduced a PAGE_TABLE_ORDER in page-defs.h?

Sorry, should be PAGE_TABLE_ORDER_4K.

Roger.