* [PATCH 1/2] [v2] crypto: hisilicon - avoid unused function warning
@ 2019-09-19 14:05 Arnd Bergmann
2019-09-19 14:05 ` [PATCH 2/2] crypto: hisilicon - allow compile-testing on x86 Arnd Bergmann
2019-09-20 13:03 ` [PATCH 1/2] [v2] crypto: hisilicon - avoid unused function warning Herbert Xu
0 siblings, 2 replies; 11+ messages in thread
From: Arnd Bergmann @ 2019-09-19 14:05 UTC (permalink / raw)
To: Zhou Wang, Herbert Xu, David S. Miller
Cc: Arnd Bergmann, Kenneth Lee, Shiju Jose, linux-crypto, linux-kernel
The only caller of hisi_zip_vf_q_assign() is hidden in an #ifdef,
so the function causes a warning when CONFIG_PCI_IOV is disabled:
drivers/crypto/hisilicon/zip/zip_main.c:740:12: error: unused function 'hisi_zip_vf_q_assign' [-Werror,-Wunused-function]
Replace the #ifdef with an IS_ENABLED() check that leads to the
function being dropped based on the configuration.
Fixes: 79e09f30eeba ("crypto: hisilicon - add SRIOV support for ZIP")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/crypto/hisilicon/zip/zip_main.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/crypto/hisilicon/zip/zip_main.c b/drivers/crypto/hisilicon/zip/zip_main.c
index 6e0ca75585d4..1b2ee96c888d 100644
--- a/drivers/crypto/hisilicon/zip/zip_main.c
+++ b/drivers/crypto/hisilicon/zip/zip_main.c
@@ -785,7 +785,6 @@ static int hisi_zip_clear_vft_config(struct hisi_zip *hisi_zip)
static int hisi_zip_sriov_enable(struct pci_dev *pdev, int max_vfs)
{
-#ifdef CONFIG_PCI_IOV
struct hisi_zip *hisi_zip = pci_get_drvdata(pdev);
int pre_existing_vfs, num_vfs, ret;
@@ -815,9 +814,6 @@ static int hisi_zip_sriov_enable(struct pci_dev *pdev, int max_vfs)
}
return num_vfs;
-#else
- return 0;
-#endif
}
static int hisi_zip_sriov_disable(struct pci_dev *pdev)
@@ -948,7 +944,8 @@ static struct pci_driver hisi_zip_pci_driver = {
.id_table = hisi_zip_dev_ids,
.probe = hisi_zip_probe,
.remove = hisi_zip_remove,
- .sriov_configure = hisi_zip_sriov_configure,
+ .sriov_configure = IS_ENABLED(CONFIG_PCI_IOV) ?
+ hisi_zip_sriov_configure : 0,
.err_handler = &hisi_zip_err_handler,
};
--
2.20.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/2] crypto: hisilicon - allow compile-testing on x86
2019-09-19 14:05 [PATCH 1/2] [v2] crypto: hisilicon - avoid unused function warning Arnd Bergmann
@ 2019-09-19 14:05 ` Arnd Bergmann
2019-09-19 14:09 ` [PATCH 2/2] [v2] " Arnd Bergmann
2019-09-20 13:03 ` [PATCH 1/2] [v2] crypto: hisilicon - avoid unused function warning Herbert Xu
1 sibling, 1 reply; 11+ messages in thread
From: Arnd Bergmann @ 2019-09-19 14:05 UTC (permalink / raw)
To: Herbert Xu, David S. Miller, Zhou Wang
Cc: Arnd Bergmann, Jonathan Cameron, Kenneth Lee, Shiju Jose,
Mao Wenan, John Garry, Hao Fang, linux-crypto, linux-kernel
To avoid missing arm64 specific warnings that get introduced
in this driver, allow compile-testing on all 64-bit architectures.
The only actual arm64 specific code in this driver is an open-
coded 128 bit MMIO write. On non-arm64 the same can be done
using memcpy_toio. What I also noticed is that the mmio store
(either one) is not endian-safe, this will only work on little-
endian configurations, so I also add a Kconfig dependency on
that, regardless of the architecture.
Finally, a depenndecy on CONFIG_64BIT is needed because of the
writeq().
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/crypto/hisilicon/Kconfig | 8 +++++---
drivers/crypto/hisilicon/qm.c | 6 ++++++
2 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/crypto/hisilicon/Kconfig b/drivers/crypto/hisilicon/Kconfig
index ebaf91e0146d..dfbd668a431e 100644
--- a/drivers/crypto/hisilicon/Kconfig
+++ b/drivers/crypto/hisilicon/Kconfig
@@ -16,14 +16,15 @@ config CRYPTO_DEV_HISI_SEC
config CRYPTO_DEV_HISI_QM
tristate
- depends on ARM64 && PCI && PCI_MSI
+ depends on ARM64 || COMPILE_TEST
+ depends on PCI && PCI_MSI
help
HiSilicon accelerator engines use a common queue management
interface. Specific engine driver may use this module.
config CRYPTO_HISI_SGL
tristate
- depends on ARM64
+ depends on ARM64 || COMPILE_TEST
help
HiSilicon accelerator engines use a common hardware scatterlist
interface for data format. Specific engine driver may use this
@@ -31,7 +32,8 @@ config CRYPTO_HISI_SGL
config CRYPTO_DEV_HISI_ZIP
tristate "Support for HiSilicon ZIP accelerator"
- depends on ARM64 && PCI && PCI_MSI
+ depends on PCI && PCI_MSI
+ depends on ARM64 || (COMPILE_TEST && 64BIT)
select CRYPTO_DEV_HISI_QM
select CRYPTO_HISI_SGL
select SG_SPLIT
diff --git a/drivers/crypto/hisilicon/qm.c b/drivers/crypto/hisilicon/qm.c
index f975c393a603..a8ed699081b7 100644
--- a/drivers/crypto/hisilicon/qm.c
+++ b/drivers/crypto/hisilicon/qm.c
@@ -331,6 +331,12 @@ static void qm_mb_write(struct hisi_qm *qm, const void *src)
void __iomem *fun_base = qm->io_base + QM_MB_CMD_SEND_BASE;
unsigned long tmp0 = 0, tmp1 = 0;
+ if (!IS_ENABLED(CONFIG_ARM64)) {
+ memcpy_toio(fun_base, src, 16);
+ wmb();
+ return;
+ }
+
asm volatile("ldp %0, %1, %3\n"
"stp %0, %1, %2\n"
"dsb sy\n"
--
2.20.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/2] [v2] crypto: hisilicon - allow compile-testing on x86
2019-09-19 14:05 ` [PATCH 2/2] crypto: hisilicon - allow compile-testing on x86 Arnd Bergmann
@ 2019-09-19 14:09 ` Arnd Bergmann
2019-09-20 8:33 ` John Garry
2019-10-04 15:44 ` Herbert Xu
0 siblings, 2 replies; 11+ messages in thread
From: Arnd Bergmann @ 2019-09-19 14:09 UTC (permalink / raw)
To: Herbert Xu, David S. Miller, Zhou Wang
Cc: Arnd Bergmann, Jonathan Cameron, Kenneth Lee, John Garry,
Mao Wenan, Hao Fang, Shiju Jose, linux-crypto, linux-kernel
To avoid missing arm64 specific warnings that get introduced
in this driver, allow compile-testing on all 64-bit architectures.
The only actual arm64 specific code in this driver is an open-
coded 128 bit MMIO write. On non-arm64 the same can be done
using memcpy_toio. What I also noticed is that the mmio store
(either one) is not endian-safe, this will only work on little-
endian configurations, so I also add a Kconfig dependency on
that, regardless of the architecture.
Finally, a depenndecy on CONFIG_64BIT is needed because of the
writeq().
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
v2: actually add !CPU_BIG_ENDIAN dependency as described in the
changelog
---
drivers/crypto/hisilicon/Kconfig | 9 ++++++---
drivers/crypto/hisilicon/qm.c | 6 ++++++
2 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/crypto/hisilicon/Kconfig b/drivers/crypto/hisilicon/Kconfig
index ebaf91e0146d..7bfcaa7674fd 100644
--- a/drivers/crypto/hisilicon/Kconfig
+++ b/drivers/crypto/hisilicon/Kconfig
@@ -16,14 +16,15 @@ config CRYPTO_DEV_HISI_SEC
config CRYPTO_DEV_HISI_QM
tristate
- depends on ARM64 && PCI && PCI_MSI
+ depends on ARM64 || COMPILE_TEST
+ depends on PCI && PCI_MSI
help
HiSilicon accelerator engines use a common queue management
interface. Specific engine driver may use this module.
config CRYPTO_HISI_SGL
tristate
- depends on ARM64
+ depends on ARM64 || COMPILE_TEST
help
HiSilicon accelerator engines use a common hardware scatterlist
interface for data format. Specific engine driver may use this
@@ -31,7 +32,9 @@ config CRYPTO_HISI_SGL
config CRYPTO_DEV_HISI_ZIP
tristate "Support for HiSilicon ZIP accelerator"
- depends on ARM64 && PCI && PCI_MSI
+ depends on PCI && PCI_MSI
+ depends on ARM64 || (COMPILE_TEST && 64BIT)
+ depends on !CPU_BIG_ENDIAN || COMPILE_TEST
select CRYPTO_DEV_HISI_QM
select CRYPTO_HISI_SGL
select SG_SPLIT
diff --git a/drivers/crypto/hisilicon/qm.c b/drivers/crypto/hisilicon/qm.c
index f975c393a603..a8ed699081b7 100644
--- a/drivers/crypto/hisilicon/qm.c
+++ b/drivers/crypto/hisilicon/qm.c
@@ -331,6 +331,12 @@ static void qm_mb_write(struct hisi_qm *qm, const void *src)
void __iomem *fun_base = qm->io_base + QM_MB_CMD_SEND_BASE;
unsigned long tmp0 = 0, tmp1 = 0;
+ if (!IS_ENABLED(CONFIG_ARM64)) {
+ memcpy_toio(fun_base, src, 16);
+ wmb();
+ return;
+ }
+
asm volatile("ldp %0, %1, %3\n"
"stp %0, %1, %2\n"
"dsb sy\n"
--
2.20.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] [v2] crypto: hisilicon - allow compile-testing on x86
2019-09-19 14:09 ` [PATCH 2/2] [v2] " Arnd Bergmann
@ 2019-09-20 8:33 ` John Garry
2019-09-20 13:26 ` Arnd Bergmann
2019-10-04 15:44 ` Herbert Xu
1 sibling, 1 reply; 11+ messages in thread
From: John Garry @ 2019-09-20 8:33 UTC (permalink / raw)
To: Arnd Bergmann, Herbert Xu, David S. Miller, Zhou Wang
Cc: Jonathan Cameron, Kenneth Lee, Mao Wenan, Hao Fang, Shiju Jose,
linux-crypto, linux-kernel
On 19/09/2019 15:09, Arnd Bergmann wrote:
> To avoid missing arm64 specific warnings that get introduced
> in this driver, allow compile-testing on all 64-bit architectures.
>
> The only actual arm64 specific code in this driver is an open-
> coded 128 bit MMIO write. On non-arm64 the same can be done
> using memcpy_toio. What I also noticed is that the mmio store
> (either one) is not endian-safe, this will only work on little-
> endian configurations, so I also add a Kconfig dependency on
> that, regardless of the architecture.
> Finally, a depenndecy on CONFIG_64BIT is needed because of the
nit: spelling mistake
> writeq().
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> v2: actually add !CPU_BIG_ENDIAN dependency as described in the
> changelog
> ---
> drivers/crypto/hisilicon/Kconfig | 9 ++++++---
> drivers/crypto/hisilicon/qm.c | 6 ++++++
> 2 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/crypto/hisilicon/Kconfig b/drivers/crypto/hisilicon/Kconfig
> index ebaf91e0146d..7bfcaa7674fd 100644
> --- a/drivers/crypto/hisilicon/Kconfig
> +++ b/drivers/crypto/hisilicon/Kconfig
> @@ -16,14 +16,15 @@ config CRYPTO_DEV_HISI_SEC
>
> config CRYPTO_DEV_HISI_QM
> tristate
> - depends on ARM64 && PCI && PCI_MSI
> + depends on ARM64 || COMPILE_TEST
> + depends on PCI && PCI_MSI
> help
> HiSilicon accelerator engines use a common queue management
> interface. Specific engine driver may use this module.
>
> config CRYPTO_HISI_SGL
> tristate
> - depends on ARM64
> + depends on ARM64 || COMPILE_TEST
> help
> HiSilicon accelerator engines use a common hardware scatterlist
> interface for data format. Specific engine driver may use this
> @@ -31,7 +32,9 @@ config CRYPTO_HISI_SGL
>
> config CRYPTO_DEV_HISI_ZIP
> tristate "Support for HiSilicon ZIP accelerator"
> - depends on ARM64 && PCI && PCI_MSI
> + depends on PCI && PCI_MSI
> + depends on ARM64 || (COMPILE_TEST && 64BIT)
> + depends on !CPU_BIG_ENDIAN || COMPILE_TEST
> select CRYPTO_DEV_HISI_QM
> select CRYPTO_HISI_SGL
> select SG_SPLIT
> diff --git a/drivers/crypto/hisilicon/qm.c b/drivers/crypto/hisilicon/qm.c
> index f975c393a603..a8ed699081b7 100644
> --- a/drivers/crypto/hisilicon/qm.c
> +++ b/drivers/crypto/hisilicon/qm.c
> @@ -331,6 +331,12 @@ static void qm_mb_write(struct hisi_qm *qm, const void *src)
> void __iomem *fun_base = qm->io_base + QM_MB_CMD_SEND_BASE;
> unsigned long tmp0 = 0, tmp1 = 0;
>
Hi Arnd,
> + if (!IS_ENABLED(CONFIG_ARM64)) {
> + memcpy_toio(fun_base, src, 16);
> + wmb();
> + return;
> + }
> +
> asm volatile("ldp %0, %1, %3\n"
> "stp %0, %1, %2\n"
> "dsb sy\n"
>
As I understand, this operation needs to be done atomically. So - even
though your change is just for compile testing - the memcpy_to_io() may
not do the same thing on other archs, right?
I just wonder if it's right to make that change, or at least warn the
imaginary user of possible malfunction for !arm64.
Thanks,
John
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] [v2] crypto: hisilicon - avoid unused function warning
2019-09-19 14:05 [PATCH 1/2] [v2] crypto: hisilicon - avoid unused function warning Arnd Bergmann
2019-09-19 14:05 ` [PATCH 2/2] crypto: hisilicon - allow compile-testing on x86 Arnd Bergmann
@ 2019-09-20 13:03 ` Herbert Xu
1 sibling, 0 replies; 11+ messages in thread
From: Herbert Xu @ 2019-09-20 13:03 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Zhou Wang, David S. Miller, Kenneth Lee, Shiju Jose,
linux-crypto, linux-kernel
On Thu, Sep 19, 2019 at 04:05:52PM +0200, Arnd Bergmann wrote:
> The only caller of hisi_zip_vf_q_assign() is hidden in an #ifdef,
> so the function causes a warning when CONFIG_PCI_IOV is disabled:
>
> drivers/crypto/hisilicon/zip/zip_main.c:740:12: error: unused function 'hisi_zip_vf_q_assign' [-Werror,-Wunused-function]
>
> Replace the #ifdef with an IS_ENABLED() check that leads to the
> function being dropped based on the configuration.
>
> Fixes: 79e09f30eeba ("crypto: hisilicon - add SRIOV support for ZIP")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> drivers/crypto/hisilicon/zip/zip_main.c | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
Patch applied. Thanks.
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] [v2] crypto: hisilicon - allow compile-testing on x86
2019-09-20 8:33 ` John Garry
@ 2019-09-20 13:26 ` Arnd Bergmann
2019-09-20 13:36 ` Arnd Bergmann
0 siblings, 1 reply; 11+ messages in thread
From: Arnd Bergmann @ 2019-09-20 13:26 UTC (permalink / raw)
To: John Garry
Cc: Herbert Xu, David S. Miller, Zhou Wang, Jonathan Cameron,
Kenneth Lee, Mao Wenan, Hao Fang, Shiju Jose,
open list:HARDWARE RANDOM NUMBER GENERATOR CORE, linux-kernel
On Fri, Sep 20, 2019 at 10:34 AM John Garry <john.garry@huawei.com> wrote:
> > + if (!IS_ENABLED(CONFIG_ARM64)) {
> > + memcpy_toio(fun_base, src, 16);
> > + wmb();
> > + return;
> > + }
> > +
> > asm volatile("ldp %0, %1, %3\n"
> > "stp %0, %1, %2\n"
> > "dsb sy\n"
> >
>
> As I understand, this operation needs to be done atomically. So - even
> though your change is just for compile testing - the memcpy_to_io() may
> not do the same thing on other archs, right?
>
> I just wonder if it's right to make that change, or at least warn the
> imaginary user of possible malfunction for !arm64.
It's probably not necessary here. From what I can tell from the documentation,
this is only safe on ARMv8.4 or higher anyway, earlier ARMv8.x implementations
don't guarantee that an stp arrives on the bus in one piece either.
Usually, hardware like this has no hard requirement on an atomic store,
it just needs the individual bits to arrive in a particular order, and then
triggers the update on the last bit that gets stored. If that is the case here
as well, it might actually be better to use two writeq_relaxed() and
a barrier. This would also solve the endianess issue.
Arnd
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] [v2] crypto: hisilicon - allow compile-testing on x86
2019-09-20 13:26 ` Arnd Bergmann
@ 2019-09-20 13:36 ` Arnd Bergmann
2019-09-20 14:16 ` John Garry
0 siblings, 1 reply; 11+ messages in thread
From: Arnd Bergmann @ 2019-09-20 13:36 UTC (permalink / raw)
To: John Garry
Cc: Herbert Xu, David S. Miller, Zhou Wang, Jonathan Cameron,
Kenneth Lee, Mao Wenan, Hao Fang, Shiju Jose,
open list:HARDWARE RANDOM NUMBER GENERATOR CORE, linux-kernel,
Will Deacon
On Fri, Sep 20, 2019 at 3:26 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Fri, Sep 20, 2019 at 10:34 AM John Garry <john.garry@huawei.com> wrote:
>
> > > + if (!IS_ENABLED(CONFIG_ARM64)) {
> > > + memcpy_toio(fun_base, src, 16);
> > > + wmb();
> > > + return;
> > > + }
> > > +
> > > asm volatile("ldp %0, %1, %3\n"
> > > "stp %0, %1, %2\n"
> > > "dsb sy\n"
> > >
> >
> > As I understand, this operation needs to be done atomically. So - even
> > though your change is just for compile testing - the memcpy_to_io() may
> > not do the same thing on other archs, right?
> >
> > I just wonder if it's right to make that change, or at least warn the
> > imaginary user of possible malfunction for !arm64.
>
> It's probably not necessary here. From what I can tell from the documentation,
> this is only safe on ARMv8.4 or higher anyway, earlier ARMv8.x implementations
> don't guarantee that an stp arrives on the bus in one piece either.
>
> Usually, hardware like this has no hard requirement on an atomic store,
> it just needs the individual bits to arrive in a particular order, and then
> triggers the update on the last bit that gets stored. If that is the case here
> as well, it might actually be better to use two writeq_relaxed() and
> a barrier. This would also solve the endianess issue.
See also https://lkml.org/lkml/2018/1/26/554 for a previous attempt
to introduce 128-bit MMIO accessors, this got rejected since they
are not atomic even on ARMv8.4.
Arnd
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] [v2] crypto: hisilicon - allow compile-testing on x86
2019-09-20 13:36 ` Arnd Bergmann
@ 2019-09-20 14:16 ` John Garry
2019-09-21 10:26 ` Zhou Wang
0 siblings, 1 reply; 11+ messages in thread
From: John Garry @ 2019-09-20 14:16 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Herbert Xu, David S. Miller, Zhou Wang, Jonathan Cameron,
Kenneth Lee, Mao Wenan, Hao Fang, Shiju Jose,
open list:HARDWARE RANDOM NUMBER GENERATOR CORE, linux-kernel,
Will Deacon
On 20/09/2019 14:36, Arnd Bergmann wrote:
> On Fri, Sep 20, 2019 at 3:26 PM Arnd Bergmann <arnd@arndb.de> wrote:
>>
>> On Fri, Sep 20, 2019 at 10:34 AM John Garry <john.garry@huawei.com> wrote:
>>
>>>> + if (!IS_ENABLED(CONFIG_ARM64)) {
>>>> + memcpy_toio(fun_base, src, 16);
>>>> + wmb();
>>>> + return;
>>>> + }
>>>> +
>>>> asm volatile("ldp %0, %1, %3\n"
>>>> "stp %0, %1, %2\n"
>>>> "dsb sy\n"
>>>>
>>>
>>> As I understand, this operation needs to be done atomically. So - even
>>> though your change is just for compile testing - the memcpy_to_io() may
>>> not do the same thing on other archs, right?
>>>
>>> I just wonder if it's right to make that change, or at least warn the
>>> imaginary user of possible malfunction for !arm64.
>>
Hi Arnd,
>> It's probably not necessary here. From what I can tell from the documentation,
>> this is only safe on ARMv8.4 or higher anyway, earlier ARMv8.x implementations
>> don't guarantee that an stp arrives on the bus in one piece either.
>>
>> Usually, hardware like this has no hard requirement on an atomic store,
>> it just needs the individual bits to arrive in a particular order, and then
>> triggers the update on the last bit that gets stored. If that is the case here
>> as well, it might actually be better to use two writeq_relaxed() and
>> a barrier. This would also solve the endianess issue.
>
> See also https://lkml.org/lkml/2018/1/26/554 for a previous attempt
> to introduce 128-bit MMIO accessors, this got rejected since they
> are not atomic even on ARMv8.4.
So this is proprietary IP integrated with a proprietary ARMv8
implementation, so there could be a tight coupling, the like of which
Will mentioned in that thread, but I'm doubtful.
I'm looking at the electronically translated documentation on this HW,
and it reads "The Mailbox operation performed by the CPU cannot be
interleaved", and then tells that software should lock against
concurrent accesses or alternatively use a 128-bit access. So it seems
that the 128b op used is only to guarantee software is atomic.
Wang Zhou can confirm my understanding.
If true, I see that we seem to be already guaranteeing mutual exclusion
in qm_mb(), in taking a mutex.
Thanks,
John
>
> Arnd
>
> .
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] [v2] crypto: hisilicon - allow compile-testing on x86
2019-09-20 14:16 ` John Garry
@ 2019-09-21 10:26 ` Zhou Wang
2019-10-02 16:47 ` John Garry
0 siblings, 1 reply; 11+ messages in thread
From: Zhou Wang @ 2019-09-21 10:26 UTC (permalink / raw)
To: John Garry, Arnd Bergmann
Cc: Herbert Xu, David S. Miller, Jonathan Cameron, Kenneth Lee,
Mao Wenan, Hao Fang, Shiju Jose,
open list:HARDWARE RANDOM NUMBER GENERATOR CORE, linux-kernel,
Will Deacon
On 2019/9/20 22:16, John Garry wrote:
> On 20/09/2019 14:36, Arnd Bergmann wrote:
>> On Fri, Sep 20, 2019 at 3:26 PM Arnd Bergmann <arnd@arndb.de> wrote:
>>>
>>> On Fri, Sep 20, 2019 at 10:34 AM John Garry <john.garry@huawei.com> wrote:
>>>
>>>>> + if (!IS_ENABLED(CONFIG_ARM64)) {
>>>>> + memcpy_toio(fun_base, src, 16);
>>>>> + wmb();
>>>>> + return;
>>>>> + }
>>>>> +
>>>>> asm volatile("ldp %0, %1, %3\n"
>>>>> "stp %0, %1, %2\n"
>>>>> "dsb sy\n"
>>>>>
>>>>
>>>> As I understand, this operation needs to be done atomically. So - even
>>>> though your change is just for compile testing - the memcpy_to_io() may
>>>> not do the same thing on other archs, right?
>>>>
>>>> I just wonder if it's right to make that change, or at least warn the
>>>> imaginary user of possible malfunction for !arm64.
>>>
>
> Hi Arnd,
>
>>> It's probably not necessary here. From what I can tell from the documentation,
>>> this is only safe on ARMv8.4 or higher anyway, earlier ARMv8.x implementations
>>> don't guarantee that an stp arrives on the bus in one piece either.
>>>
>>> Usually, hardware like this has no hard requirement on an atomic store,
>>> it just needs the individual bits to arrive in a particular order, and then
>>> triggers the update on the last bit that gets stored. If that is the case here
>>> as well, it might actually be better to use two writeq_relaxed() and
>>> a barrier. This would also solve the endianess issue.
>>
>> See also https://lkml.org/lkml/2018/1/26/554 for a previous attempt
>> to introduce 128-bit MMIO accessors, this got rejected since they
>> are not atomic even on ARMv8.4.
>
> So this is proprietary IP integrated with a proprietary ARMv8 implementation,
> so there could be a tight coupling, the like of which Will mentioned in that thread,
> but I'm doubtful.
>
> I'm looking at the electronically translated documentation on this HW, and it reads
> "The Mailbox operation performed by the CPU cannot be interleaved", and then tells
> that software should lock against concurrent accesses or alternatively use a 128-bit
> access. So it seems that the 128b op used is only to guarantee software is atomic.
>
> Wang Zhou can confirm my understanding
We have to do a 128bit atomic write here to trigger a mailbox. The reason is
that one QM hardware entity in one accelerator servers QM mailbox MMIO interfaces in
related PF and VFs.
A mutex can not lock different processing flows in different functions.
As Arnd mentioned, v8.4 extends the support for 16 bytes atomic stp to some kinds of
normal memory, but for device memory, it is still implementation defined. For this
SoC(Kunpeng920) which has QM/ZIP, if the address is 128bit aligned, stp will be atomic.
The offset of QM mailbox is 128bit aligned, so it is safe here.
Best,
Zhou
>
> If true, I see that we seem to be already guaranteeing mutual exclusion in qm_mb(),
> in taking a mutex.
>
> Thanks,
> John
>
>
>>
>> Arnd
>>
>> .
>>
>
>
>
> .
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] [v2] crypto: hisilicon - allow compile-testing on x86
2019-09-21 10:26 ` Zhou Wang
@ 2019-10-02 16:47 ` John Garry
0 siblings, 0 replies; 11+ messages in thread
From: John Garry @ 2019-10-02 16:47 UTC (permalink / raw)
To: Zhou Wang, Arnd Bergmann
Cc: Herbert Xu, David S. Miller, Jonathan Cameron, Kenneth Lee,
Mao Wenan, Hao Fang, Shiju Jose,
open list:HARDWARE RANDOM NUMBER GENERATOR CORE, linux-kernel,
Will Deacon
On 21/09/2019 11:26, Zhou Wang wrote:
> On 2019/9/20 22:16, John Garry wrote:
>> On 20/09/2019 14:36, Arnd Bergmann wrote:
>>> On Fri, Sep 20, 2019 at 3:26 PM Arnd Bergmann <arnd@arndb.de> wrote:
>>>>
>>>> On Fri, Sep 20, 2019 at 10:34 AM John Garry <john.garry@huawei.com> wrote:
>>>>
>>>>>> + if (!IS_ENABLED(CONFIG_ARM64)) {
>>>>>> + memcpy_toio(fun_base, src, 16);
>>>>>> + wmb();
>>>>>> + return;
>>>>>> + }
>>>>>> +
>>>>>> asm volatile("ldp %0, %1, %3\n"
>>>>>> "stp %0, %1, %2\n"
>>>>>> "dsb sy\n"
>>>>>>
>>>>>
***
>>>>> As I understand, this operation needs to be done atomically. So - even
>>>>> though your change is just for compile testing - the memcpy_to_io() may
>>>>> not do the same thing on other archs, right?
>>>>>
>>>>> I just wonder if it's right to make that change, or at least warn the
>>>>> imaginary user of possible malfunction for !arm64.
>>>>
>>
>> Hi Arnd,
>>
>>>> It's probably not necessary here. From what I can tell from the documentation,
>>>> this is only safe on ARMv8.4 or higher anyway, earlier ARMv8.x implementations
>>>> don't guarantee that an stp arrives on the bus in one piece either.
>>>>
>>>> Usually, hardware like this has no hard requirement on an atomic store,
>>>> it just needs the individual bits to arrive in a particular order, and then
>>>> triggers the update on the last bit that gets stored. If that is the case here
>>>> as well, it might actually be better to use two writeq_relaxed() and
>>>> a barrier. This would also solve the endianess issue.
>>>
>>> See also https://lkml.org/lkml/2018/1/26/554 for a previous attempt
>>> to introduce 128-bit MMIO accessors, this got rejected since they
>>> are not atomic even on ARMv8.4.
>>
>> So this is proprietary IP integrated with a proprietary ARMv8 implementation,
>> so there could be a tight coupling, the like of which Will mentioned in that thread,
>> but I'm doubtful.
>>
>> I'm looking at the electronically translated documentation on this HW, and it reads
>> "The Mailbox operation performed by the CPU cannot be interleaved", and then tells
>> that software should lock against concurrent accesses or alternatively use a 128-bit
>> access. So it seems that the 128b op used is only to guarantee software is atomic.
>>
>> Wang Zhou can confirm my understanding
>
Just to add a few more details here:
> We have to do a 128bit atomic write here to trigger a mailbox. The reason is
> that one QM hardware entity in one accelerator servers QM mailbox MMIO interfaces in
> related PF and VFs.
>
> A mutex can not lock different processing flows in different functions.
This means that we can have userspace drivers and the kernel driver
simultaneously accessing these mailboxes.
>
> As Arnd mentioned, v8.4 extends the support for 16 bytes atomic stp to some kinds of
> normal memory, but for device memory, it is still implementation defined. For this
> SoC(Kunpeng920) which has QM/ZIP, if the address is 128bit aligned, stp will be atomic.
> The offset of QM mailbox is 128bit aligned, so it is safe here.
The strange thing (to me) about this hw is that we cannot interleave
mailbox accesses on different functions - with 2x 64b accesses, for
example. This is the reason that we require 128b accesses.
The upshot is that we can't switch to generic code unfortunately.
But I am not sure if the change is right, as I mentioned originally,
above ***.
I'll leave to Arnd's better judgment :)
Thanks,
John
>
> Best,
> Zhou
>
>>
>> If true, I see that we seem to be already guaranteeing mutual exclusion in qm_mb(),
>> in taking a mutex.
>>
>> Thanks,
>> John
>>
>>
>>>
>>> Arnd
>>>
>>> .
>>>
>>
>>
>>
>> .
>>
>
>
> .
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] [v2] crypto: hisilicon - allow compile-testing on x86
2019-09-19 14:09 ` [PATCH 2/2] [v2] " Arnd Bergmann
2019-09-20 8:33 ` John Garry
@ 2019-10-04 15:44 ` Herbert Xu
1 sibling, 0 replies; 11+ messages in thread
From: Herbert Xu @ 2019-10-04 15:44 UTC (permalink / raw)
To: Arnd Bergmann
Cc: David S. Miller, Zhou Wang, Jonathan Cameron, Kenneth Lee,
John Garry, Mao Wenan, Hao Fang, Shiju Jose, linux-crypto,
linux-kernel
On Thu, Sep 19, 2019 at 04:09:06PM +0200, Arnd Bergmann wrote:
> To avoid missing arm64 specific warnings that get introduced
> in this driver, allow compile-testing on all 64-bit architectures.
>
> The only actual arm64 specific code in this driver is an open-
> coded 128 bit MMIO write. On non-arm64 the same can be done
> using memcpy_toio. What I also noticed is that the mmio store
> (either one) is not endian-safe, this will only work on little-
> endian configurations, so I also add a Kconfig dependency on
> that, regardless of the architecture.
> Finally, a depenndecy on CONFIG_64BIT is needed because of the
> writeq().
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> v2: actually add !CPU_BIG_ENDIAN dependency as described in the
> changelog
> ---
> drivers/crypto/hisilicon/Kconfig | 9 ++++++---
> drivers/crypto/hisilicon/qm.c | 6 ++++++
> 2 files changed, 12 insertions(+), 3 deletions(-)
Patch applied. Thanks.
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-10-04 15:44 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-19 14:05 [PATCH 1/2] [v2] crypto: hisilicon - avoid unused function warning Arnd Bergmann
2019-09-19 14:05 ` [PATCH 2/2] crypto: hisilicon - allow compile-testing on x86 Arnd Bergmann
2019-09-19 14:09 ` [PATCH 2/2] [v2] " Arnd Bergmann
2019-09-20 8:33 ` John Garry
2019-09-20 13:26 ` Arnd Bergmann
2019-09-20 13:36 ` Arnd Bergmann
2019-09-20 14:16 ` John Garry
2019-09-21 10:26 ` Zhou Wang
2019-10-02 16:47 ` John Garry
2019-10-04 15:44 ` Herbert Xu
2019-09-20 13:03 ` [PATCH 1/2] [v2] crypto: hisilicon - avoid unused function warning Herbert Xu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).