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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.05.2022 11:10, Roger Pau Monné wrote:
> 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.

Hmm, yes, let me do that. It'll be a little odd in the header itself
(as I'll need to exclude the bulk of it when CONTIG_MASK is not
defined), but apart from that it should indeed end up being better.

Jan