RE: [PATCH 2/4] VT-d / x86: re-arrange cache syncing

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Thursday, December 2, 2021 5:19 PM
> 
> On 01.12.2021 15:39, Andrew Cooper wrote:
> > On 01/12/2021 09:40, Jan Beulich wrote:
> >> The actual function should always have lived in core x86 code; move it
> >> there, replacing get_cache_line_size() by readily available (except very
> >> early during boot; see the code comment) data.
> >>
> >> Drop the respective IOMMU hook, (re)introducing a respective boolean
> >> instead. Replace a true and an almost open-coding instance of
> >> iommu_sync_cache().
> >
> > Coherency (or not) is a per-IOMMU property, not a global platform
> > property.  iGPU IOMMUs are very different to those the uncore, and
> > there's no reason to presume that IOMMUs in the PCH would have the
> same
> > properties as those in the uncore.
> >
> > Given how expensive sync_cache() is, we cannot afford to be using it for
> > coherent IOMMUs in a mixed system.
> >
> > This wants to be a boolean in arch_iommu.
> 
> That's a valid consideration, but may not be as easy as it may seem on
> the surface. Certainly not something I could promise to find time for
> soon. And definitely separate from the specific change here.

I'm fine with this patch if you prefer to a staging approach to improve it.
By any means this patch doesn't make things worse.

> 
> >> ---
> >> Placing the function next to flush_area_local() exposes a curious
> >> asymmetry between the SFENCE placements: sync_cache() has it after the
> >> flush, while flush_area_local() has it before it. I think the latter one
> >> is misplaced.
> >
> > Wow this is a mess.
> >
> > First and foremost, AMD state that on pre-CLFLUSHOPT parts, CLFLUSH is
> > unordered with ~anything (including SFENCE), and need bounding with
> > MFENCE on both sides.  We definitely aren't doing this correctly right now.
> >
> >
> > AMD explicitly states that SFENCE drains the store and WC buffers (i.e.
> > make stuff instantaneously globally visible).  Intel does not, and
> > merely guarantees ordering.
> >
> > A leading SFENCE would only make sense if there were WC concerns, but
> > both manuals say that the memory type doesn't matter, so I can't see a
> > justification for it.
> >
> > What does matter, from the IOMMU's point of view, is that the line has
> > been written back (or evicted on pre-CLWB parts) before the IOTLB
> > invalidation occurs.  The invalidation will be a write to a different
> > address, which is why the trailing SFENCE is necessary, as CLFLUSHOPT
> > isn't ordered with respect to unaliasing writes.
> 
> IOW for the purposes of this change all is correct, and everything else
> will require taking care of separately.
> 

Same for this part. btw Linux does mfence both before and after clflush.

Thanks
Kevin