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

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

On Mon, May 23, 2022 at 08:49:27AM +0200, Jan Beulich wrote:
> On 20.05.2022 16:38, Roger Pau Monné wrote:
> > 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.
> 
> Oh, good that I looked here before replying to the earlier mail: I'm
> afraid I view PAGE_TABLE_ORDER_4K as not very useful. From an
> abstract POV, what is the base unit meant to be that the order is
> is based upon? PAGE_SHIFT? Or PAGE_SHIFT_4K? I think such an
> ambiguity is going to remain even if we very clearly spelled out what
> we mean things to be, as one would always need to go back to that
> comment to check which of the two possible ways it is.
> 
> Furthermore I'm not convinced PAGETABLE_ORDER is really meant to be
> associated with a particular page size anyway: PAGE_TABLE_ORDER_2M
> imo makes no sense at all. And page-defs.h is not supposed to
> express any platform properties anyway, it's merely an accumulation
> of (believed) useful constants.
> 
> Hence the only thing which I might see as a (remote) option is
> IOMMU_PAGE_TABLE_ORDER (for platforms where all IOMMU variants have
> all page table levels using identical sizes, which isn't a given, but
> which would hold for x86 and hence for the purpose here).

Since you already define a page table order in pt-contig-markers.h
(CONTIG_NR) it might be possible to export and use that?  In fact the
check done here would be even more accurate if it was done using the
same constant that's used in pt_update_contig_markers(), because the
purpose here is to check that the vendor specific code to init the
page tables has used the correct value.

Thanks, Roger.