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
Signed-off-by: Yoan Picchi <[email protected]>
---
drivers/crypto/qat/Kconfig | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/crypto/qat/Kconfig b/drivers/crypto/qat/Kconfig
index 4b90c0f22b03..afc28059274e 100644
--- a/drivers/crypto/qat/Kconfig
+++ b/drivers/crypto/qat/Kconfig
@@ -17,7 +17,7 @@ config CRYPTO_DEV_QAT
config CRYPTO_DEV_QAT_DH895xCC
tristate "Support for Intel(R) DH895xCC"
- depends on X86 && PCI
+ depends on PCI && !CPU_BIG_ENDIAN
select CRYPTO_DEV_QAT
help
Support for Intel(R) DH895xcc with Intel(R) QuickAssist Technology
@@ -28,7 +28,7 @@ config CRYPTO_DEV_QAT_DH895xCC
config CRYPTO_DEV_QAT_C3XXX
tristate "Support for Intel(R) C3XXX"
- depends on X86 && PCI
+ depends on PCI && !CPU_BIG_ENDIAN
select CRYPTO_DEV_QAT
help
Support for Intel(R) C3xxx with Intel(R) QuickAssist Technology
@@ -39,7 +39,7 @@ config CRYPTO_DEV_QAT_C3XXX
config CRYPTO_DEV_QAT_C62X
tristate "Support for Intel(R) C62X"
- depends on X86 && PCI
+ depends on PCI && !CPU_BIG_ENDIAN
select CRYPTO_DEV_QAT
help
Support for Intel(R) C62x with Intel(R) QuickAssist Technology
@@ -50,7 +50,7 @@ config CRYPTO_DEV_QAT_C62X
config CRYPTO_DEV_QAT_4XXX
tristate "Support for Intel(R) QAT_4XXX"
- depends on X86 && PCI
+ depends on PCI && !CPU_BIG_ENDIAN
select CRYPTO_DEV_QAT
help
Support for Intel(R) QuickAssist Technology QAT_4xxx
@@ -61,7 +61,7 @@ config CRYPTO_DEV_QAT_4XXX
config CRYPTO_DEV_QAT_DH895xCCVF
tristate "Support for Intel(R) DH895xCC Virtual Function"
- depends on X86 && PCI
+ depends on PCI && !CPU_BIG_ENDIAN
select PCI_IOV
select CRYPTO_DEV_QAT
@@ -74,7 +74,7 @@ config CRYPTO_DEV_QAT_DH895xCCVF
config CRYPTO_DEV_QAT_C3XXXVF
tristate "Support for Intel(R) C3XXX Virtual Function"
- depends on X86 && PCI
+ depends on PCI && !CPU_BIG_ENDIAN
select PCI_IOV
select CRYPTO_DEV_QAT
help
@@ -86,7 +86,7 @@ config CRYPTO_DEV_QAT_C3XXXVF
config CRYPTO_DEV_QAT_C62XVF
tristate "Support for Intel(R) C62X Virtual Function"
- depends on X86 && PCI
+ depends on PCI && !CPU_BIG_ENDIAN
select PCI_IOV
select CRYPTO_DEV_QAT
help
--
2.25.1
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).
The only testing obligation you have is compiling for BE. 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.
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.
Rob
On Thu, 9 Jun 2022 15:36:52 -0600
Rob Herring <[email protected]> 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).
>
> 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.
> 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
On Fri, Jun 10, 2022 at 11:48:40AM +0100, Andre Przywara wrote:
> On Thu, 9 Jun 2022 15:36:52 -0600
> Rob Herring <[email protected]> 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
On Mon, Jun 13, 2022 at 4:41 AM Giovanni Cabiddu
<[email protected]> wrote:
>
> On Fri, Jun 10, 2022 at 11:48:40AM +0100, Andre Przywara wrote:
> > On Thu, 9 Jun 2022 15:36:52 -0600
> > Rob Herring <[email protected]> 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.
Okay, then at least add '|| COMPILE_TEST' so that allyesconfig builds build it.
Rob