Re: [PATCH v2 04/10] iommupt: Flush the CPU cache after any writes to the page table

[Baolu Lu <baolu.lu@linux.intel.com>]

On 9/23/25 22:13, Jason Gunthorpe wrote:
> On Tue, Sep 23, 2025 at 10:29:21AM +0800, Baolu Lu wrote:
>> On 9/22/25 22:44, Jason Gunthorpe wrote:
>>> On Mon, Sep 22, 2025 at 10:31:49AM +0800, Baolu Lu wrote:
>>>> On 8/27/25 01:26, Jason Gunthorpe wrote:
>>>>> @@ -585,6 +635,7 @@ static __always_inline int __do_map_single_page(struct pt_range *range,
>>>>>     			return -EADDRINUSE;
>>>>>     		pt_install_leaf_entry(&pts, map->oa, PAGE_SHIFT,
>>>>>     				      &map->attrs);
>>>>> +		/* No flush, not used when incoherent */
>>>>>     		map->oa += PAGE_SIZE;
>>>>>     		return 0;
>>>>>     	}
>>>>> @@ -811,7 +862,8 @@ int DOMAIN_NS(map_pages)(struct iommu_domain *domain, unsigned long iova,
>>>>>     	PT_WARN_ON(map.leaf_level > range.top_level);
>>>>>     	do {
>>>>> -		if (single_page) {
>>>>> +		if (single_page &&
>>>>> +		    !pt_feature(common, PT_FEAT_DMA_INCOHERENT)) {
>>>>>     			ret = pt_walk_range(&range, __map_single_page, &map);
>>>>>     			if (ret != -EAGAIN)
>>>>>     				break;
>>>> I don't follow the single_page logic here. Why is single_page exclusive
>>>> with PT_FEAT_DMA_INCOHERENT? To my understanding, PT_FEAT_DMA_INCOHERENT
>>>> has no relationship with how the page table is organized. Could you
>>>> elaborate a bit?
>>> It is this comment above:
>>>
>>>                   /* No flush, not used when incoherent */
>>>
>>> __do_map_single_page() doesn't implement the coherency logic. As an
>>> agressive inline I didn't want to bloat it.
>> But, is that functionally correct, though?
> It is functionally correct because it is never called on incoherent
> configurations 🙂
> 
>> In the incoherent case, even
>> if a leaf page entry is changed, the cache should be flushed so that the
>> hardware can see the change.
> Yes, and since the code doesn't do this it isn't called in that case
> otherwise it is a functional problem.
> 
>> Basically, I don't understand why
>> __do_map_single_page() is "not used when incoherent". I must have
>> overlooked something. 🙂
> Basically it is an optimization and we disable the optimization on
> incoherent. It is disabled because implementing the flushing would
> bloat the code undesirably.

Oh, I see! __do_map_single_page() is actually a fast path. We disabled
this fast path for the incoherent case due to the cache flushing
behavior. Thanks for the explanation.

Thanks,
baolu