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

[Jason Gunthorpe <jgg@nvidia.com>]

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.

Jason