Re: memset() in crypto code?

2014-10-08 Thread Daniel Borkmann

On 10/08/2014 04:30 AM, Sandy Harris wrote:

I have started a thread about this on the gcc help mailing list
https://gcc.gnu.org/ml/gcc-help/2014-10/msg00047.html


Great, perhaps you want to pass a patch proposal to gcc folks?


We might consider replacinging memzero_explicit with memset_s() since
that is in the C!! standard, albeit I think as optional. IBM, Apple,
NetBSD, ... have that.
https://mail-index.netbsd.org/tech-userlevel/2012/02/24/msg006125.html


The patch you point to for NetBSD does nothing else what memzero_explicit()
or bzero_explicit() variants already internally do, only that they're
discussing about whether to adopt it or not in their user space libc ...

Cheers,
Daniel
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] crypto, qat, use generic numa functions

2014-10-08 Thread Prarit Bhargava


On 10/08/2014 11:50 AM, Tadeusz Struk wrote:
 Hi Prarit,
 On 10/07/2014 05:12 PM, Prarit Bhargava wrote:
 The method in which the qat code determines the numa node for memory
 allocations is a bit clunky.  On 2 socket, single node systems it is
 possible that adf_get_dev_node_id() returns node 1, even though node 1
 doesn't exist.

 This code transitions the qat code to the generic numa functions.
 Changing adf_get_dev_node_id() to a simple call to dev_get_node() results
 in a change to the adf_accel_dev struct as well.
 
 The problem with that is we don't want to use any valid numa node, but
 the node we are connected to or we don't want to use the accelerator at
 all. Otherwise, when the first valid numa node happens to be the remote
 node the dma transactions we be slow and instead of accelerating we will
 slow things down.
 A patch that enforces this is on it's way.

Yeah, I was actually wondering if

dev_get_node() returns NO_NODE, then we should just default to 0?

I'll wait for your patch ...

P.
 
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] crypto, qat, use generic numa functions

2014-10-08 Thread Tadeusz Struk
Hi Prarit,
On 10/07/2014 05:12 PM, Prarit Bhargava wrote:
 The method in which the qat code determines the numa node for memory
 allocations is a bit clunky.  On 2 socket, single node systems it is
 possible that adf_get_dev_node_id() returns node 1, even though node 1
 doesn't exist.
 
 This code transitions the qat code to the generic numa functions.
 Changing adf_get_dev_node_id() to a simple call to dev_get_node() results
 in a change to the adf_accel_dev struct as well.

The problem with that is we don't want to use any valid numa node, but
the node we are connected to or we don't want to use the accelerator at
all. Otherwise, when the first valid numa node happens to be the remote
node the dma transactions we be slow and instead of accelerating we will
slow things down.
A patch that enforces this is on it's way.

--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 0/2] crypto: qat - Fix for invalid dma mapping and numa

2014-10-08 Thread Tadeusz Struk
Hi,
These two patches fix invalid (zero length) dma mapping and
enforce numa configuration for maximum performance.

Signed-off-by: Tadeusz Struk tadeusz.st...@intel.com
---

Tadeusz Struk (2):
  crypto: qat - Prevent dma mapping zero length assoc data.
  crypto: qat - Enforce valid numa configuration.


 drivers/crypto/qat/qat_common/adf_accel_devices.h |3 +--
 drivers/crypto/qat/qat_common/adf_transport.c |   12 +++-
 drivers/crypto/qat/qat_common/qat_algs.c  |7 +--
 drivers/crypto/qat/qat_common/qat_crypto.c|8 +---
 drivers/crypto/qat/qat_dh895xcc/adf_admin.c   |2 +-
 drivers/crypto/qat/qat_dh895xcc/adf_drv.c |9 -
 drivers/crypto/qat/qat_dh895xcc/adf_isr.c |2 +-
 7 files changed, 28 insertions(+), 15 deletions(-)

-- 
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 2/2] crypto: qat - Enforce valid numa configuration.

2014-10-08 Thread Tadeusz Struk
In a system with NUMA configuration we want to enforce that the accelerator is
connected to a node with memory to avoid cross QPI memory transaction.
Otherwise there is no point in using the accelerator as the encryption in
software will be faster.

Signed-off-by: Tadeusz Struk tadeusz.st...@intel.com
---
 drivers/crypto/qat/qat_common/adf_accel_devices.h |3 +--
 drivers/crypto/qat/qat_common/adf_transport.c |   12 +++-
 drivers/crypto/qat/qat_common/qat_algs.c  |5 +++--
 drivers/crypto/qat/qat_common/qat_crypto.c|8 +---
 drivers/crypto/qat/qat_dh895xcc/adf_admin.c   |2 +-
 drivers/crypto/qat/qat_dh895xcc/adf_drv.c |9 -
 drivers/crypto/qat/qat_dh895xcc/adf_isr.c |2 +-
 7 files changed, 26 insertions(+), 15 deletions(-)

diff --git a/drivers/crypto/qat/qat_common/adf_accel_devices.h 
b/drivers/crypto/qat/qat_common/adf_accel_devices.h
index 3cfe195..96e0b06 100644
--- a/drivers/crypto/qat/qat_common/adf_accel_devices.h
+++ b/drivers/crypto/qat/qat_common/adf_accel_devices.h
@@ -203,8 +203,7 @@ struct adf_accel_dev {
struct dentry *debugfs_dir;
struct list_head list;
struct module *owner;
-   uint8_t accel_id;
-   uint8_t numa_node;
struct adf_accel_pci accel_pci_dev;
+   uint8_t accel_id;
 } __packed;
 #endif
diff --git a/drivers/crypto/qat/qat_common/adf_transport.c 
b/drivers/crypto/qat/qat_common/adf_transport.c
index 5f3fa45..9dd2cb7 100644
--- a/drivers/crypto/qat/qat_common/adf_transport.c
+++ b/drivers/crypto/qat/qat_common/adf_transport.c
@@ -419,9 +419,10 @@ static int adf_init_bank(struct adf_accel_dev *accel_dev,
WRITE_CSR_RING_BASE(csr_addr, bank_num, i, 0);
ring = bank-rings[i];
if (hw_data-tx_rings_mask  (1  i)) {
-   ring-inflights = kzalloc_node(sizeof(atomic_t),
-  GFP_KERNEL,
-  accel_dev-numa_node);
+   ring-inflights =
+   kzalloc_node(sizeof(atomic_t),
+GFP_KERNEL,
+dev_to_node(GET_DEV(accel_dev)));
if (!ring-inflights)
goto err;
} else {
@@ -469,13 +470,14 @@ int adf_init_etr_data(struct adf_accel_dev *accel_dev)
int i, ret;
 
etr_data = kzalloc_node(sizeof(*etr_data), GFP_KERNEL,
-   accel_dev-numa_node);
+   dev_to_node(GET_DEV(accel_dev)));
if (!etr_data)
return -ENOMEM;
 
num_banks = GET_MAX_BANKS(accel_dev);
size = num_banks * sizeof(struct adf_etr_bank_data);
-   etr_data-banks = kzalloc_node(size, GFP_KERNEL, accel_dev-numa_node);
+   etr_data-banks = kzalloc_node(size, GFP_KERNEL,
+  dev_to_node(GET_DEV(accel_dev)));
if (!etr_data-banks) {
ret = -ENOMEM;
goto err_bank;
diff --git a/drivers/crypto/qat/qat_common/qat_algs.c 
b/drivers/crypto/qat/qat_common/qat_algs.c
index bffa8bf..0897a1c 100644
--- a/drivers/crypto/qat/qat_common/qat_algs.c
+++ b/drivers/crypto/qat/qat_common/qat_algs.c
@@ -598,7 +598,8 @@ static int qat_alg_sgl_to_bufl(struct qat_crypto_instance 
*inst,
if (unlikely(!n))
return -EINVAL;
 
-   bufl = kmalloc_node(sz, GFP_ATOMIC, inst-accel_dev-numa_node);
+   bufl = kmalloc_node(sz, GFP_ATOMIC,
+   dev_to_node(GET_DEV(inst-accel_dev)));
if (unlikely(!bufl))
return -ENOMEM;
 
@@ -644,7 +645,7 @@ static int qat_alg_sgl_to_bufl(struct qat_crypto_instance 
*inst,
struct qat_alg_buf *bufers;
 
buflout = kmalloc_node(sz, GFP_ATOMIC,
-  inst-accel_dev-numa_node);
+  dev_to_node(GET_DEV(inst-accel_dev)));
if (unlikely(!buflout))
goto err;
bloutp = dma_map_single(dev, buflout, sz, DMA_TO_DEVICE);
diff --git a/drivers/crypto/qat/qat_common/qat_crypto.c 
b/drivers/crypto/qat/qat_common/qat_crypto.c
index 060dc0a..c1eefc4 100644
--- a/drivers/crypto/qat/qat_common/qat_crypto.c
+++ b/drivers/crypto/qat/qat_common/qat_crypto.c
@@ -109,12 +109,14 @@ struct qat_crypto_instance 
*qat_crypto_get_instance_node(int node)
 
list_for_each(itr, adf_devmgr_get_head()) {
accel_dev = list_entry(itr, struct adf_accel_dev, list);
-   if (accel_dev-numa_node == node  adf_dev_started(accel_dev))
+   if ((node == dev_to_node(GET_DEV(accel_dev)) ||
+   dev_to_node(GET_DEV(accel_dev))  0)
+adf_dev_started(accel_dev))
break;

[PATCH 1/2] crypto: qat - Prevent dma mapping zero length assoc data.

2014-10-08 Thread Tadeusz Struk
Do not attempt to dma map associated data if it is zero length.

Signed-off-by: Tadeusz Struk tadeusz.st...@intel.com
---
 drivers/crypto/qat/qat_common/qat_algs.c |2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/crypto/qat/qat_common/qat_algs.c 
b/drivers/crypto/qat/qat_common/qat_algs.c
index e03afe9..bffa8bf 100644
--- a/drivers/crypto/qat/qat_common/qat_algs.c
+++ b/drivers/crypto/qat/qat_common/qat_algs.c
@@ -607,6 +607,8 @@ static int qat_alg_sgl_to_bufl(struct qat_crypto_instance 
*inst,
goto err;
 
for_each_sg(assoc, sg, assoc_n, i) {
+   if (!sg-length)
+   continue;
bufl-bufers[bufs].addr = dma_map_single(dev,
 sg_virt(sg),
 sg-length,

--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 2/2] crypto: qat - Enforce valid numa configuration.

2014-10-08 Thread Prarit Bhargava


On 10/08/2014 01:38 PM, Tadeusz Struk wrote:
 In a system with NUMA configuration we want to enforce that the accelerator is
 connected to a node with memory to avoid cross QPI memory transaction.
 Otherwise there is no point in using the accelerator as the encryption in
 software will be faster.
 
 Signed-off-by: Tadeusz Struk tadeusz.st...@intel.com
 ---
  drivers/crypto/qat/qat_common/adf_accel_devices.h |3 +--
  drivers/crypto/qat/qat_common/adf_transport.c |   12 +++-
  drivers/crypto/qat/qat_common/qat_algs.c  |5 +++--
  drivers/crypto/qat/qat_common/qat_crypto.c|8 +---
  drivers/crypto/qat/qat_dh895xcc/adf_admin.c   |2 +-
  drivers/crypto/qat/qat_dh895xcc/adf_drv.c |9 -
  drivers/crypto/qat/qat_dh895xcc/adf_isr.c |2 +-
  7 files changed, 26 insertions(+), 15 deletions(-)
 
 diff --git a/drivers/crypto/qat/qat_common/adf_accel_devices.h 
 b/drivers/crypto/qat/qat_common/adf_accel_devices.h
 index 3cfe195..96e0b06 100644
 --- a/drivers/crypto/qat/qat_common/adf_accel_devices.h
 +++ b/drivers/crypto/qat/qat_common/adf_accel_devices.h
 @@ -203,8 +203,7 @@ struct adf_accel_dev {
   struct dentry *debugfs_dir;
   struct list_head list;
   struct module *owner;
 - uint8_t accel_id;
 - uint8_t numa_node;
   struct adf_accel_pci accel_pci_dev;
 + uint8_t accel_id;
  } __packed;
  #endif
 diff --git a/drivers/crypto/qat/qat_common/adf_transport.c 
 b/drivers/crypto/qat/qat_common/adf_transport.c
 index 5f3fa45..9dd2cb7 100644
 --- a/drivers/crypto/qat/qat_common/adf_transport.c
 +++ b/drivers/crypto/qat/qat_common/adf_transport.c
 @@ -419,9 +419,10 @@ static int adf_init_bank(struct adf_accel_dev *accel_dev,
   WRITE_CSR_RING_BASE(csr_addr, bank_num, i, 0);
   ring = bank-rings[i];
   if (hw_data-tx_rings_mask  (1  i)) {
 - ring-inflights = kzalloc_node(sizeof(atomic_t),
 -GFP_KERNEL,
 -accel_dev-numa_node);
 + ring-inflights =
 + kzalloc_node(sizeof(atomic_t),
 +  GFP_KERNEL,
 +  dev_to_node(GET_DEV(accel_dev)));
   if (!ring-inflights)
   goto err;
   } else {
 @@ -469,13 +470,14 @@ int adf_init_etr_data(struct adf_accel_dev *accel_dev)
   int i, ret;
  
   etr_data = kzalloc_node(sizeof(*etr_data), GFP_KERNEL,
 - accel_dev-numa_node);
 + dev_to_node(GET_DEV(accel_dev)));
   if (!etr_data)
   return -ENOMEM;
  
   num_banks = GET_MAX_BANKS(accel_dev);
   size = num_banks * sizeof(struct adf_etr_bank_data);
 - etr_data-banks = kzalloc_node(size, GFP_KERNEL, accel_dev-numa_node);
 + etr_data-banks = kzalloc_node(size, GFP_KERNEL,
 +dev_to_node(GET_DEV(accel_dev)));
   if (!etr_data-banks) {
   ret = -ENOMEM;
   goto err_bank;
 diff --git a/drivers/crypto/qat/qat_common/qat_algs.c 
 b/drivers/crypto/qat/qat_common/qat_algs.c
 index bffa8bf..0897a1c 100644
 --- a/drivers/crypto/qat/qat_common/qat_algs.c
 +++ b/drivers/crypto/qat/qat_common/qat_algs.c
 @@ -598,7 +598,8 @@ static int qat_alg_sgl_to_bufl(struct qat_crypto_instance 
 *inst,
   if (unlikely(!n))
   return -EINVAL;
  
 - bufl = kmalloc_node(sz, GFP_ATOMIC, inst-accel_dev-numa_node);
 + bufl = kmalloc_node(sz, GFP_ATOMIC,
 + dev_to_node(GET_DEV(inst-accel_dev)));
   if (unlikely(!bufl))
   return -ENOMEM;
  
 @@ -644,7 +645,7 @@ static int qat_alg_sgl_to_bufl(struct qat_crypto_instance 
 *inst,
   struct qat_alg_buf *bufers;
  
   buflout = kmalloc_node(sz, GFP_ATOMIC,
 -inst-accel_dev-numa_node);
 +dev_to_node(GET_DEV(inst-accel_dev)));
   if (unlikely(!buflout))
   goto err;
   bloutp = dma_map_single(dev, buflout, sz, DMA_TO_DEVICE);
 diff --git a/drivers/crypto/qat/qat_common/qat_crypto.c 
 b/drivers/crypto/qat/qat_common/qat_crypto.c
 index 060dc0a..c1eefc4 100644
 --- a/drivers/crypto/qat/qat_common/qat_crypto.c
 +++ b/drivers/crypto/qat/qat_common/qat_crypto.c
 @@ -109,12 +109,14 @@ struct qat_crypto_instance 
 *qat_crypto_get_instance_node(int node)
  
   list_for_each(itr, adf_devmgr_get_head()) {
   accel_dev = list_entry(itr, struct adf_accel_dev, list);
 - if (accel_dev-numa_node == node  adf_dev_started(accel_dev))
 + if ((node == dev_to_node(GET_DEV(accel_dev)) ||
 + dev_to_node(GET_DEV(accel_dev))  0)
 +  adf_dev_started(accel_dev))
   

Re: [PATCH 2/2] crypto: qat - Enforce valid numa configuration.

2014-10-08 Thread Tadeusz Struk
On 10/08/2014 10:57 AM, Prarit Bhargava wrote:
  node = adf_get_dev_node_id(pdev);
 ^^^ I don't think you should ever make this call.  IMO it is wrong to do it 
 that
 way.  Just stick with
 
   node = dev_to_node(pdev-dev)
 
 as the line below forces a default to that anyway.

But then how do I know which node I'm physically connected to?

 
  +  if (node != dev_to_node(pdev-dev)  dev_to_node(pdev-dev)  0) {
  +  /* If the accelerator is connected to a node with no memory
  +   * there is no point in using the accelerator since the remote
  +   * memory transaction will be very slow. */
  +  dev_err(pdev-dev, Invalid NUMA configuration.\n);
  +  return -EINVAL;
 Hmm ... I wonder if it would be safe to do
 
   /* force allocations to node 0 */
   node = 0;
   dev_err(pdev-dev, Invalid NUMA configuration detected, node id = %d .
 Defaulting node to 0.  \n,
 node);
 
 and then continue?

As the comment say there is no point continuing if the configuration is
wrong. Defaulting to 0 will cause the same panic you pointed out in your
first patch if node 0 has no memory.

 
 And maybe even a FW_WARN of some sort here might be appropriate to indicate 
 that
 something is wrong with the mapping?  In any case a better error message is a
 always good idea IMO.

--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 2/2] crypto: qat - Enforce valid numa configuration.

2014-10-08 Thread Prarit Bhargava


On 10/08/2014 02:11 PM, Tadeusz Struk wrote:
 On 10/08/2014 10:57 AM, Prarit Bhargava wrote:
 node = adf_get_dev_node_id(pdev);
 ^^^ I don't think you should ever make this call.  IMO it is wrong to do it 
 that
 way.  Just stick with

  node = dev_to_node(pdev-dev)

 as the line below forces a default to that anyway.
 
 But then how do I know which node I'm physically connected to?

The pci_dev maps to the bus which maps to a numa node.  The pci_dev's numa value
is copied directly from the bus (or busses depending on how deep it is).

I'd argue (strongly) that the pci_dev's numa ID better be correct o/w that is a
FW bug (and a bad one at that these days).

dev_to_node() should return the correct value.


 +  if (node != dev_to_node(pdev-dev)  dev_to_node(pdev-dev)  0) {
 +  /* If the accelerator is connected to a node with no memory
 +   * there is no point in using the accelerator since the remote
 +   * memory transaction will be very slow. */
 +  dev_err(pdev-dev, Invalid NUMA configuration.\n);
 +  return -EINVAL;
 Hmm ... I wonder if it would be safe to do

  /* force allocations to node 0 */
  node = 0;
  dev_err(pdev-dev, Invalid NUMA configuration detected, node id = %d .
 Defaulting node to 0.  \n,
 node);

 and then continue?
 
 As the comment say there is no point continuing if the configuration is
 wrong. Defaulting to 0 will cause the same panic you pointed out in your
 first patch if node 0 has no memory.

Okay, but at least fix up the warning message to output the node_id.  That's
sort of the important piece here.

P.

 

 And maybe even a FW_WARN of some sort here might be appropriate to indicate 
 that
 something is wrong with the mapping?  In any case a better error message is a
 always good idea IMO.
 
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 2/2] crypto: qat - Enforce valid numa configuration.

2014-10-08 Thread Tadeusz Struk
On 10/08/2014 11:35 AM, Prarit Bhargava wrote:
 But then how do I know which node I'm physically connected to?
 The pci_dev maps to the bus which maps to a numa node.  The pci_dev's numa 
 value
 is copied directly from the bus (or busses depending on how deep it is).
 
 I'd argue (strongly) that the pci_dev's numa ID better be correct o/w that is 
 a
 FW bug (and a bad one at that these days).
 
 dev_to_node() should return the correct value.

I'm not saying that the dev_to_node() returns incorrect value. It will
always return the closest numa node for the given device.
What we want to enforce is that the closest numa node is the node that
the device is physically connected to. In case if the closest numa node
is the remote node we don't want to use this accelerator.

 
 
  +if (node != dev_to_node(pdev-dev)  dev_to_node(pdev-dev) 
   0) {
  +/* If the accelerator is connected to a node with no 
  memory
  + * there is no point in using the accelerator since the 
  remote
  + * memory transaction will be very slow. */
  +dev_err(pdev-dev, Invalid NUMA configuration.\n);
  +return -EINVAL;
  Hmm ... I wonder if it would be safe to do
 
   /* force allocations to node 0 */
   node = 0;
   dev_err(pdev-dev, Invalid NUMA configuration detected, node id = %d .
  Defaulting node to 0.  \n,
  node);
 
  and then continue?
  
  As the comment say there is no point continuing if the configuration is
  wrong. Defaulting to 0 will cause the same panic you pointed out in your
  first patch if node 0 has no memory.
 Okay, but at least fix up the warning message to output the node_id.  That's
 sort of the important piece here.

Sure, I can update the message add print both the numa node and node id
the device is connected to.

--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 2/2] crypto: qat - Enforce valid numa configuration.

2014-10-08 Thread Prarit Bhargava


On 10/08/2014 02:57 PM, Tadeusz Struk wrote:
 On 10/08/2014 11:35 AM, Prarit Bhargava wrote:
 But then how do I know which node I'm physically connected to?
 The pci_dev maps to the bus which maps to a numa node.  The pci_dev's numa 
 value
 is copied directly from the bus (or busses depending on how deep it is).

 I'd argue (strongly) that the pci_dev's numa ID better be correct o/w that 
 is a
 FW bug (and a bad one at that these days).

 dev_to_node() should return the correct value.
 
 I'm not saying that the dev_to_node() returns incorrect value. It will
 always return the closest numa node for the given device.

No that isn't correct.  dev_to_node() will return the node the device is
connected to.

 What we want to enforce is that the closest numa node is the node that
 the device is physically connected to. In case if the closest numa node
 is the remote node we don't want to use this accelerator.
 

P.
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 2/2] crypto: qat - Enforce valid numa configuration.

2014-10-08 Thread Tadeusz Struk
On 10/08/2014 12:01 PM, Prarit Bhargava wrote:
 No that isn't correct.  dev_to_node() will return the node the device is
 connected to.

include/linux/device.h:

static inline int dev_to_node(struct device *dev)
{
  return dev-numa_node;
}

struct device {
.
  int numa_node; /* NUMA node this device is close to */
...
};

In case when there are two nodes and only node 0 has memory,
dev-numa_node will be 0 even though the device will be connected to the
pci root port of node 1.
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html