Re: [PATCH] cxl/region: Move coherence tracking into cxl_region_attach()

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Thu, 30 Mar 2023 11:16:42 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Jonathan Cameron wrote:
> > On Tue, 28 Mar 2023 11:36:28 -0700
> > Dan Williams <dan.j.williams@intel.com> wrote:
> >   
> > > Each time the contents of a given HPA are potentially changed in a cache
> > > incoherent manner the CXL core sets CXL_REGION_F_INCOHERENT to
> > > invalidate CPU caches before the region is used.
> > > 
> > > Successful invocation of attach_target() indicates that DPA has been
> > > newly assigned to a given HPA in the dynamic region creation flow.
> > > However, attach_target() is also reused in the autodiscovery flow where
> > > the region was activated by platform firmware. In that case there is no
> > > need to invalidate caches because that region is already in active use
> > > and nothing about the autodiscovery flow modifies the HPA-to-DPA
> > > relationship.
> > > 
> > > Fixes: a32320b71f08 ("cxl/region: Add region autodiscovery")
> > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>  
> > 
> > Looks correct to me.  Might be worth making it clear in the patch description
> > that for auto discovered regions, we have exited cxl_region_attach()
> > before the point where you set the flag.  
> 
> Sure, added:
> 
> "In the autodiscovery case cxl_region_attach() exits early after
> determining the endpoint decoder is already correctly attached to the
> region."
Thanks.
> 
> 
> > I'm also fairly sure that in previous flow the cache flush was happening on
> > each target being added rather than just on the final one being added which
> > would be a significant overhead also avoided via this patch.  
> 
> No, it did set the bit multiple times, but the actual flush itself does
> not occur until the region is activated in cxl_region_probe() (the only
> caller of cxl_region_invalidate_memregion()).

ah. That makes sense.  Bit isn't magic :)