Re: [PATCH v4 16/21] VT-d: free all-empty page tables

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.05.2022 09:40, Jan Beulich wrote:
> On 20.05.2022 13:13, Roger Pau Monné wrote:
>> On Wed, May 18, 2022 at 12:26:03PM +0200, Jan Beulich wrote:
>>> On 10.05.2022 16:30, Roger Pau Monné wrote:
>>>> On Mon, Apr 25, 2022 at 10:42:50AM +0200, Jan Beulich wrote:
>>>>> @@ -837,9 +843,31 @@ static int dma_pte_clear_one(struct doma
>>>>>  
>>>>>      old = *pte;
>>>>>      dma_clear_pte(*pte);
>>>>> +    iommu_sync_cache(pte, sizeof(*pte));
>>>>> +
>>>>> +    while ( pt_update_contig_markers(&page->val,
>>>>> +                                     address_level_offset(addr, level),
>>>>> +                                     level, PTE_kind_null) &&
>>>>> +            ++level < min_pt_levels )
>>>>> +    {
>>>>> +        struct page_info *pg = maddr_to_page(pg_maddr);
>>>>> +
>>>>> +        unmap_vtd_domain_page(page);
>>>>> +
>>>>> +        pg_maddr = addr_to_dma_page_maddr(domain, addr, level, 
>>>>> flush_flags,
>>>>> +                                          false);
>>>>> +        BUG_ON(pg_maddr < PAGE_SIZE);
>>>>> +
>>>>> +        page = map_vtd_domain_page(pg_maddr);
>>>>> +        pte = &page[address_level_offset(addr, level)];
>>>>> +        dma_clear_pte(*pte);
>>>>> +        iommu_sync_cache(pte, sizeof(*pte));
>>>>> +
>>>>> +        *flush_flags |= IOMMU_FLUSHF_all;
>>>>> +        iommu_queue_free_pgtable(hd, pg);
>>>>> +    }
>>>>
>>>> I think I'm setting myself for trouble, but do we need to sync cache
>>>> the lower lever entries if higher level ones are to be changed.
>>>>
>>>> IOW, would it be fine to just flush the highest level modified PTE?
>>>> As the lower lever ones won't be reachable anyway.
>>>
>>> I definitely want to err on the safe side here. If later we can
>>> prove that some cache flush is unneeded, I'd be happy to see it
>>> go away.
>>
>> Hm, so it's not only about adding more cache flushes, but moving them
>> inside of the locked region: previously the only cache flush was done
>> outside of the locked region.
>>
>> I guess I can't convince myself why we would need to flush cache of
>> entries that are to be removed, and that also point to pages scheduled
>> to be freed.
> 
> As previously said - with a series like this I wanted to strictly be
> on the safe side, maintaining the pre-existing pattern of all
> modifications of live tables being accompanied by a flush (if flushes
> are needed in the first place, of course). As to moving flushes into
> the locked region, I don't view this as a problem, seeing in
> particular that elsewhere we already have flushes with the lock held
> (at the very least the _full page_ one in addr_to_dma_page_maddr(),
> but also e.g. in intel_iommu_map_page(), where it could be easily
> moved past the unlock).
> 
> If you (continue to) think that breaking the present pattern isn't
> going to misguide future changes, I can certainly drop these not
> really necessary flushes. Otoh I was actually considering to,
> subsequently, integrate the flushes into e.g. dma_clear_pte() to
> make it virtually impossible to break that pattern. This would
> imply that all page table related flushes would then occur with the
> lock held.
> 
> (I won't separately reply to the similar topic on patch 18.)

Oh, one more (formal / minor) aspect: Changing when to (not) flush
would also invalidate Kevin's R-b which I've got already for both
this and the later, similarly affected patch.

Jan