On Tue, Dec 05, 2023 at 01:51:27PM -0400, Jason Gunthorpe wrote: > On Tue, Dec 05, 2023 at 05:21:27PM +0000, Catalin Marinas wrote: > > On Mon, Dec 04, 2023 at 02:23:30PM -0400, Jason Gunthorpe wrote: > > > On Mon, Dec 04, 2023 at 05:31:47PM +0000, Catalin Marinas wrote: > > > > Personally I'd optimise the mempcy_toio() arm64 implementation to do > > > > STPs if the alignment is right (like we do for classic memcpy()). > > > > There's a slight overhead for alignment checking but I suspect it would > > > > be lost as long as you can get the write-combining. Not sure whether the > > > > interspersed reads in memcpy_toio() would somehow prevent the > > > > write-combining. > > > > > > I understand on these new CPUs anything other than a block of > > > contiguous STPs is risky to break the WC. I was told we should not > > > have any loads between them. > > > > Classic memcpy does similar tricks with four LDPs in a row before > > starting to issue the STPs (though there are new LDPs for the next > > data in-between). But that was tuned for cacheable memory, not sure > > if something similar would behave well on Normal-NC memory. > > Can we conclude a direction here? > > 1) I don't want to mess with x86 so we keep a dedicated API > Are we agreed to call it __iowrite512_copy() and note its special > alignment limitation? Sounds fine to me. > 2) You want to #define __iowrite512_copy() to memcpy_toio() on ARM and > implement some quad STP optimization for this case? We can have the generic __iowrite512_copy() do memcpy_toio() and have the arm64 implement an optimised version. What I'm not entirely sure of is the DGH (whatever the io_* barrier name is). I'd put it in the same __iowrite512_copy() function and remove it from the driver code. Otherwise when ST64B is added, we have an unnecessary DGH in the driver. If this does not match the other __iowrite*_copy() semantics, we can come up with another name. But start with this for now and document the function. > 3) A future ST64B and the x86 version would be put under > __iowrite512_copy()? Yes, arch-specific override. > 4) A future ST64B would come with some kind of 'must do 64b copy or > oops' to support the future HW that must have this instruction? eg > we already see on Intel that HW must use ENQCMD and nothing else. I don't agree with the oops part. We can't guarantee it on arm64, ST64B I think is optional in the architecture. If you do need such guarantees, we'd need the driver to probe for the feature (e.g. arch_has_...()) and invoke a new macro. You can't have the __iowrite512_copy() that worked fine suddenly giving an error just because some driver wants a guaranteed atomic 64 byte write. -- Catalin
