> From: Jan Beulich <jbeul...@suse.com> > 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
