linux-kernel - Re: [PATCH 2/2] Removes the x86 dependency on the QAT drivers

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: <YqcUXrFGO+QVMKBd@silpixa00400314>
Date:   Mon, 13 Jun 2022 11:41:34 +0100
From:   Giovanni Cabiddu <giovanni.cabiddu@...el.com>
To:     Andre Przywara <andre.przywara@....com>
Cc:     Rob Herring <robh@...nel.org>, Yoan Picchi <yoan.picchi@....com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        "David S . Miller" <davem@...emloft.net>, qat-linux@...el.com,
        linux-crypto@...r.kernel.org, linux-kernel@...r.kernel.org,
        Ard Biesheuvel <ardb@...nel.org>
Subject: Re: [PATCH 2/2] Removes the x86 dependency on the QAT drivers

On Fri, Jun 10, 2022 at 11:48:40AM +0100, Andre Przywara wrote:
> On Thu, 9 Jun 2022 15:36:52 -0600
> Rob Herring <robh@...nel.org> wrote:
> 
> Hi,
> 
> > On Tue, Jun 07, 2022 at 04:58:40PM +0000, Yoan Picchi wrote:
> > > This dependency looks outdated. After the previous patch, we have been able
> > > to use this driver to encrypt some data and to create working VF on arm64.
> > > We have not tested it yet on any big endian machine, hence the new dependency  
> > 
> > For the subject, use prefixes matching the subsystem (like you did on 
> > patch 1).
Just to add on this, patches to the qat driver should have the following
headline:
    crypto: qat -
not
    crypto: qat:

> > 
> > The only testing obligation you have is compiling for BE.
> 
> So I just compiled for arm64 BE, powerpc BE & LE, and riscv again:
> $ file qat_c62xvf.ko
> qat_c62xvf.ko: ELF 64-bit MSB relocatable, ARM aarch64, version 1 (SYSV),
> BuildID[sha1]=630cc0ee5586c7aeb6e0ab5567ce2f2f7cc46adf, with debug_info,
> not stripped
> qat_c62xvf.ko: ELF 64-bit MSB relocatable, 64-bit PowerPC or cisco 7500,
> version 1 (SYSV), BuildID[sha1]=4090ba181cf95f27108bf3ecde0776f12ef2b636,
> not stripped
> qat_c62xvf.ko: ELF 64-bit LSB relocatable, 64-bit PowerPC or cisco 7500,
> version 1 (SYSV), BuildID[sha1]=2cb0fd09d5bc36c8918fcd061c9f3dac1546cf0d,
> not stripped
> qat_c62xvf.ko: ELF 64-bit LSB relocatable, UCB RISC-V, version 1 (SYSV),
> BuildID[sha1]=bfaa53df7e9aad79d3ab4c05e75ca9169227f6b8, not stripped
> 
> All built without errors or warnings, for every of the enabled drivers.
> 
> > If kconfig was
> > supposed to capture what endianness drivers have been tested or not
> > tested with, then lots of drivers are missing the dependency. Kconfig
> > depends/select entries should generally be either to prevent compile
> > failures (you checked PPC, RiscV, etc.?) or to hide drivers *really*
> > specific to a platform. IMO, we should only have !CPU_BIG_ENDIAN if it 
> > is known not to work and not easily fixed.
> 
> Fair enough, I leave that decision to Giovanni. I have plans to test this
> with BE, but getting a BE setup on a server is not trivial, both for
> userland and actual booting, so this will take some time. We just didn't
> want to block this on some BE concerns.
Just inspecting the code I can see we are not handling BE in the logic
that builds FW descriptors.
My preference would be to keep !CPU_BIG_ENDIAN until the driver is fixed
and tested.

> > Also, with the dependency, no one can test the driver without modifying 
> > the kernel and if it does work as-is, then one has to upstream a change 
> > and then wait for it to show up in distro kernels. You could mitigate 
> > the first part with COMPILE_TEST.
> 
> Yeah, that's a good point, we were already bitten by this, the initial
> testing was done on a stable distro kernel (v5.4), and it worked fine
> already there.
> 
> Cheers,
> Andre

-- 
Giovanni