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]

Message-ID: <CAPcyv4g2uYjqSx3qe6g6PYzVJcRrn3HgsmksV-+vvGm-vLhY9A@mail.gmail.com>
Date:   Wed, 20 Nov 2019 16:21:05 -0800
From:   Dan Williams <dan.j.williams@...el.com>
To:     "Luck, Tony" <tony.luck@...el.com>
Cc:     Borislav Petkov <bp@...en8.de>, Dave Jiang <dave.jiang@...el.com>,
        dmaengine@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Vinod Koul <vkoul@...nel.org>, Jing Lin <jing.lin@...el.com>,
        "Raj, Ashok" <ashok.raj@...el.com>,
        "Kumar, Sanjay K" <sanjay.k.kumar@...el.com>,
        "Dey, Megha" <megha.dey@...el.com>,
        "Pan, Jacob jun" <jacob.jun.pan@...el.com>,
        "Liu, Yi L" <yi.l.liu@...el.com>, Jens Axboe <axboe@...nel.dk>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        Fenghua Yu <fenghua.yu@...el.com>,
        "H. Peter Anvin" <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 3:19 PM Luck, Tony <tony.luck@...el.com> 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.