linux-kernel - Re: [PATCH RFC 01/14] x86/asm: add iosubmit_cmds512() based on movdir64b CPU instruction

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 20 Nov 2019 15:19:23 -0800
From:   "Luck, Tony" <tony.luck@...el.com>
To:     Borislav Petkov <bp@...en8.de>
Cc:     Dave Jiang <dave.jiang@...el.com>, dmaengine@...r.kernel.org,
        linux-kernel@...r.kernel.org, vkoul@...nel.org,
        dan.j.williams@...el.com, jing.lin@...el.com, ashok.raj@...el.com,
        sanjay.k.kumar@...el.com, megha.dey@...el.com,
        jacob.jun.pan@...el.com, yi.l.liu@...el.com, axboe@...nel.dk,
        akpm@...ux-foundation.org, tglx@...utronix.de, mingo@...hat.com,
        fenghua.yu@...el.com, hpa@...or.com
Subject: Re: [PATCH RFC 01/14] x86/asm: add iosubmit_cmds512() based on
 movdir64b CPU instruction

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).

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.

-Tony