On Wed, Nov 20, 2019 at 3:19 PM Luck, Tony <tony.luck@xxxxxxxxx> wrote: > > On Wed, Nov 20, 2019 at 10:53:39PM +0100, Borislav Petkov wrote: > > On Wed, Nov 20, 2019 at 02:23:49PM -0700, Dave Jiang wrote: > > > +static inline void iosubmit_cmds512(void __iomem *dst, const void *src, > > > + size_t count) > > > > An iosubmit function which returns void and doesn't tell its callers > > whether it succeeded or not? That looks non-optimal to say the least. > > That's the underlying functionality of the MOVDIR64B instruction. A > posted write so no way to know if it succeeded. When using dedicated > queues the caller must keep count of how many operations are in flight > and not send more than the depth of the queue. > > > Why isn't there a fallback function which to call when the CPU doesn't > > support movdir64b? > > This particular driver has no option for fallback. Descriptors can > only be submitted with MOVDIR64B (to dedicated queues ... in later > patch series support for shared queues will be added, but those require > ENQCMD or ENQCMDS to submit). I think > > The driver bails out at the beginning of the probe routine if the > necessary instructions are not supported: > > + /* > + * If the CPU does not support write512, there's no point in > + * enumerating the device. We can not utilize it. > + */ > + if (!cpu_has_write512()) > + return -ENXIO; > > Though we should always get past that as this PCI device ID shouldn't > every appear on a system that doesn't have the support. Device is on > the die, not a plug-in card. > > > Because then you can use alternative_call() and have the thing work > > regardless of hardware support for MOVDIR*. > > > > > +{ > > > + const u8 *from = src; > > > + const u8 *end = from + count * 64; > > > + > > > + if (!cpu_has_write512()) > > > > If anything, that thing needs to go and you should use > > > > static_cpu_has(X86_FEATURE_MOVDIR64B) > > > > as it looks to me like you would care about speed on this fast path? > > Yes, no? > > That might be a better. It's meant to be identical. The expectation was that cpu_has_write512() could be used generically in drivers like the pmem driver that have a use for movdir64b outside of DSA command submission use case. On x86 it would just be #define'd to static_cpu_has(X86_FEATURE_MOVDIR64B), on other archs something else in the future. For pmem if cpu_has_write512() is false it falls back to talking to platform firmware for error management. Case1 from the changelog.