Re: [PATCH] iommu: spapr_tce: Disable compile testing to fix build on book3s_32 config

2020-04-18 Thread Christophe Leroy




On 04/14/2020 02:26 PM, Krzysztof Kozlowski wrote:

Although SPAPR_TCE_IOMMU itself can be compile tested on certain PowerPC
configurations, its presence makes arch/powerpc/kvm/Makefile to select
modules which do not build in such configuration.

The arch/powerpc/kvm/ modules use kvm_arch.spapr_tce_tables which exists
only with CONFIG_PPC_BOOK3S_64.  However these modules are selected when
COMPILE_TEST and SPAPR_TCE_IOMMU are chosen leading to build failures:

 In file included from arch/powerpc/include/asm/book3s/64/mmu-hash.h:20:0,
  from arch/powerpc/kvm/book3s_64_vio_hv.c:22:
 arch/powerpc/include/asm/book3s/64/pgtable.h:17:0: error: "_PAGE_EXEC" 
redefined [-Werror]
  #define _PAGE_EXEC  0x1 /* execute permission */

 In file included from arch/powerpc/include/asm/book3s/32/pgtable.h:8:0,
  from arch/powerpc/include/asm/book3s/pgtable.h:8,
  from arch/powerpc/include/asm/pgtable.h:18,
  from include/linux/mm.h:95,
  from arch/powerpc/include/asm/io.h:29,
  from include/linux/io.h:13,
  from include/linux/irq.h:20,
  from arch/powerpc/include/asm/hardirq.h:6,
  from include/linux/hardirq.h:9,
  from include/linux/kvm_host.h:7,
  from arch/powerpc/kvm/book3s_64_vio_hv.c:12:
 arch/powerpc/include/asm/book3s/32/hash.h:29:0: note: this is the location 
of the previous definition
  #define _PAGE_EXEC 0x200 /* software: exec allowed */

Reported-by: Geert Uytterhoeven 
Fixes: e93a1695d7fb ("iommu: Enable compile testing for some of drivers")
Signed-off-by: Krzysztof Kozlowski 
---
  drivers/iommu/Kconfig | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index 58b4a4dbfc78..3532b1ead19d 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -362,7 +362,7 @@ config IPMMU_VMSA
  
  config SPAPR_TCE_IOMMU

bool "sPAPR TCE IOMMU Support"
-   depends on PPC_POWERNV || PPC_PSERIES || (PPC && COMPILE_TEST)
+   depends on PPC_POWERNV || PPC_PSERIES
select IOMMU_API
help
  Enables bits of IOMMU API required by VFIO. The iommu_ops



Should it be fixed the other way round, something like:

diff --git a/arch/powerpc/kvm/Makefile b/arch/powerpc/kvm/Makefile
index 2bfeaa13befb..906707d15810 100644
--- a/arch/powerpc/kvm/Makefile
+++ b/arch/powerpc/kvm/Makefile
@@ -135,4 +135,4 @@ obj-$(CONFIG_KVM_BOOK3S_32) += kvm.o
 obj-$(CONFIG_KVM_BOOK3S_64_PR) += kvm-pr.o
 obj-$(CONFIG_KVM_BOOK3S_64_HV) += kvm-hv.o

-obj-y += $(kvm-book3s_64-builtin-objs-y)
+obj-$(CONFIG_KVM_BOOK3S_64) += $(kvm-book3s_64-builtin-objs-y)


Christophe
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [PATCH v2 2/2] crypto: Remove unnecessary memzero_explicit()

2020-04-14 Thread Christophe Leroy



Le 14/04/2020 à 00:28, Waiman Long a écrit :

Since kfree_sensitive() will do an implicit memzero_explicit(), there
is no need to call memzero_explicit() before it. Eliminate those
memzero_explicit() and simplify the call sites. For better correctness,
the setting of keylen is also moved down after the key pointer check.

Signed-off-by: Waiman Long 
---
  .../allwinner/sun8i-ce/sun8i-ce-cipher.c  | 19 +-
  .../allwinner/sun8i-ss/sun8i-ss-cipher.c  | 20 +--
  drivers/crypto/amlogic/amlogic-gxl-cipher.c   | 12 +++
  drivers/crypto/inside-secure/safexcel_hash.c  |  3 +--
  4 files changed, 14 insertions(+), 40 deletions(-)

diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c 
b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
index aa4e8fdc2b32..8358fac98719 100644
--- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
+++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
@@ -366,10 +366,7 @@ void sun8i_ce_cipher_exit(struct crypto_tfm *tfm)
  {
struct sun8i_cipher_tfm_ctx *op = crypto_tfm_ctx(tfm);
  
-	if (op->key) {

-   memzero_explicit(op->key, op->keylen);
-   kfree(op->key);
-   }
+   kfree_sensitive(op->key);
crypto_free_sync_skcipher(op->fallback_tfm);
pm_runtime_put_sync_suspend(op->ce->dev);
  }
@@ -391,14 +388,11 @@ int sun8i_ce_aes_setkey(struct crypto_skcipher *tfm, 
const u8 *key,
dev_dbg(ce->dev, "ERROR: Invalid keylen %u\n", keylen);
return -EINVAL;
}
-   if (op->key) {
-   memzero_explicit(op->key, op->keylen);
-   kfree(op->key);
-   }
-   op->keylen = keylen;
+   kfree_sensitive(op->key);
op->key = kmemdup(key, keylen, GFP_KERNEL | GFP_DMA);
if (!op->key)
return -ENOMEM;
+   op->keylen = keylen;


Does it matter at all to ensure op->keylen is not set when of->key is 
NULL ? I'm not sure.


But if it does, then op->keylen should be set to 0 when freeing op->key.

  
  	crypto_sync_skcipher_clear_flags(op->fallback_tfm, CRYPTO_TFM_REQ_MASK);

crypto_sync_skcipher_set_flags(op->fallback_tfm, tfm->base.crt_flags & 
CRYPTO_TFM_REQ_MASK);
@@ -416,14 +410,11 @@ int sun8i_ce_des3_setkey(struct crypto_skcipher *tfm, 
const u8 *key,
if (err)
return err;
  
-	if (op->key) {

-   memzero_explicit(op->key, op->keylen);
-   kfree(op->key);
-   }
-   op->keylen = keylen;
+   kfree_sensitive(op->key);
op->key = kmemdup(key, keylen, GFP_KERNEL | GFP_DMA);
if (!op->key)
return -ENOMEM;
+   op->keylen = keylen;


Same comment as above.

  
  	crypto_sync_skcipher_clear_flags(op->fallback_tfm, CRYPTO_TFM_REQ_MASK);

crypto_sync_skcipher_set_flags(op->fallback_tfm, tfm->base.crt_flags & 
CRYPTO_TFM_REQ_MASK);
diff --git a/drivers/crypto/allwinner/sun8i-ss/sun8i-ss-cipher.c 
b/drivers/crypto/allwinner/sun8i-ss/sun8i-ss-cipher.c
index 5246ef4f5430..0495fbc27fcc 100644
--- a/drivers/crypto/allwinner/sun8i-ss/sun8i-ss-cipher.c
+++ b/drivers/crypto/allwinner/sun8i-ss/sun8i-ss-cipher.c
@@ -249,7 +249,6 @@ static int sun8i_ss_cipher(struct skcipher_request *areq)
offset = areq->cryptlen - ivsize;
if (rctx->op_dir & SS_DECRYPTION) {
memcpy(areq->iv, backup_iv, ivsize);
-   memzero_explicit(backup_iv, ivsize);
kfree_sensitive(backup_iv);
} else {
scatterwalk_map_and_copy(areq->iv, areq->dst, 
offset,
@@ -367,10 +366,7 @@ void sun8i_ss_cipher_exit(struct crypto_tfm *tfm)
  {
struct sun8i_cipher_tfm_ctx *op = crypto_tfm_ctx(tfm);
  
-	if (op->key) {

-   memzero_explicit(op->key, op->keylen);
-   kfree(op->key);
-   }
+   kfree_sensitive(op->key);
crypto_free_sync_skcipher(op->fallback_tfm);
pm_runtime_put_sync(op->ss->dev);
  }
@@ -392,14 +388,11 @@ int sun8i_ss_aes_setkey(struct crypto_skcipher *tfm, 
const u8 *key,
dev_dbg(ss->dev, "ERROR: Invalid keylen %u\n", keylen);
return -EINVAL;
}
-   if (op->key) {
-   memzero_explicit(op->key, op->keylen);
-   kfree(op->key);
-   }
-   op->keylen = keylen;
+   kfree_sensitive(op->key);
op->key = kmemdup(key, keylen, GFP_KERNEL | GFP_DMA);
if (!op->key)
return -ENOMEM;
+   op->keylen = keylen;


Same comment as above.

  
  	crypto_sync_skcipher_clear_flags(op->fallback_tfm, CRYPTO_TFM_REQ_MASK);

crypto_sync_skcipher_set_flags(op->fallback_tfm, tfm->base.crt_flags & 
CRYPTO_TFM_REQ_MASK);
@@ -418,14 +411,11 @@ int sun8i_ss_des3_setkey(struct crypto_skcipher *tfm, 
const u8 *key,
return -EINVAL;
}
  
-	if (op->key) {

-  

Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument

2020-01-08 Thread Christophe Leroy



Le 08/01/2020 à 09:18, Krzysztof Kozlowski a écrit :

On Wed, 8 Jan 2020 at 09:13, Geert Uytterhoeven  wrote:


Hi Krzysztof,

On Wed, Jan 8, 2020 at 9:07 AM Geert Uytterhoeven  wrote:

On Tue, Jan 7, 2020 at 5:53 PM Krzysztof Kozlowski  wrote:

The ioread8/16/32() and others have inconsistent interface among the
architectures: some taking address as const, some not.

It seems there is nothing really stopping all of them to take
pointer to const.


Shouldn't all of them take const volatile __iomem pointers?
It seems the "volatile" is missing from all but the implementations in
include/asm-generic/io.h.


As my "volatile" comment applies to iowrite*(), too, probably that should be
done in a separate patch.

Hence with patches 1-5 squashed, and for patches 11-13:
Reviewed-by: Geert Uytterhoeven 


I'll add to this one also changes to ioreadX_rep() and add another
patch for volatile for reads and writes. I guess your review will be
appreciated once more because of ioreadX_rep()



volatile should really only be used where deemed necessary:

https://www.kernel.org/doc/html/latest/process/volatile-considered-harmful.html

It is said: " ...  accessor functions might use volatile on 
architectures where direct I/O memory access does work. Essentially, 
each accessor call becomes a little critical section on its own and 
ensures that the access happens as expected by the programmer."


Christophe
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument

2020-01-08 Thread Christophe Leroy

Hi Geert,

Le 08/01/2020 à 09:43, Geert Uytterhoeven a écrit :

Hi Christophe,

On Wed, Jan 8, 2020 at 9:35 AM Christophe Leroy  wrote:

Le 08/01/2020 à 09:18, Krzysztof Kozlowski a écrit :

On Wed, 8 Jan 2020 at 09:13, Geert Uytterhoeven  wrote:

On Wed, Jan 8, 2020 at 9:07 AM Geert Uytterhoeven  wrote:

On Tue, Jan 7, 2020 at 5:53 PM Krzysztof Kozlowski  wrote:

The ioread8/16/32() and others have inconsistent interface among the
architectures: some taking address as const, some not.

It seems there is nothing really stopping all of them to take
pointer to const.


Shouldn't all of them take const volatile __iomem pointers?
It seems the "volatile" is missing from all but the implementations in
include/asm-generic/io.h.


As my "volatile" comment applies to iowrite*(), too, probably that should be
done in a separate patch.

Hence with patches 1-5 squashed, and for patches 11-13:
Reviewed-by: Geert Uytterhoeven 


I'll add to this one also changes to ioreadX_rep() and add another
patch for volatile for reads and writes. I guess your review will be
appreciated once more because of ioreadX_rep()


volatile should really only be used where deemed necessary:

https://www.kernel.org/doc/html/latest/process/volatile-considered-harmful.html

It is said: " ...  accessor functions might use volatile on
architectures where direct I/O memory access does work. Essentially,
each accessor call becomes a little critical section on its own and
ensures that the access happens as expected by the programmer."


That is exactly the use case here: all above are accessor functions.

Why would ioreadX() not need volatile, while readY() does?



My point was: it might be necessary for some arches and not for others.

And as pointed by Arnd, the volatile is really only necessary for the 
dereference itself, should the arch use dereferencing.


So I guess the best would be to go in the other direction: remove 
volatile keyword wherever possible instead of adding it where it is not 
needed.


Christophe
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 22/22] docs: fix broken documentation links

2019-07-24 Thread Christophe Leroy



Le 30/05/2019 à 01:23, Mauro Carvalho Chehab a écrit :

Mostly due to x86 and acpi conversion, several documentation
links are still pointing to the old file. Fix them.

Signed-off-by: Mauro Carvalho Chehab 
---
  Documentation/acpi/dsd/leds.txt  |  2 +-
  Documentation/admin-guide/kernel-parameters.rst  |  6 +++---
  Documentation/admin-guide/kernel-parameters.txt  | 16 
  Documentation/admin-guide/ras.rst|  2 +-
  .../devicetree/bindings/net/fsl-enetc.txt|  7 +++
  .../bindings/pci/amlogic,meson-pcie.txt  |  2 +-
  .../bindings/regulator/qcom,rpmh-regulator.txt   |  2 +-
  Documentation/devicetree/booting-without-of.txt  |  2 +-
  Documentation/driver-api/gpio/board.rst  |  2 +-
  Documentation/driver-api/gpio/consumer.rst   |  2 +-
  .../firmware-guide/acpi/enumeration.rst  |  2 +-
  .../firmware-guide/acpi/method-tracing.rst   |  2 +-
  Documentation/i2c/instantiating-devices  |  2 +-
  Documentation/sysctl/kernel.txt  |  4 ++--
  .../translations/it_IT/process/howto.rst |  2 +-
  .../it_IT/process/stable-kernel-rules.rst|  4 ++--
  .../translations/zh_CN/process/4.Coding.rst  |  2 +-
  Documentation/x86/x86_64/5level-paging.rst   |  2 +-
  Documentation/x86/x86_64/boot-options.rst|  4 ++--
  .../x86/x86_64/fake-numa-for-cpusets.rst |  2 +-
  MAINTAINERS  |  6 +++---
  arch/arm/Kconfig |  2 +-
  arch/arm64/kernel/kexec_image.c  |  2 +-
  arch/powerpc/Kconfig |  2 +-
  arch/x86/Kconfig | 16 
  arch/x86/Kconfig.debug   |  2 +-
  arch/x86/boot/header.S   |  2 +-
  arch/x86/entry/entry_64.S|  2 +-
  arch/x86/include/asm/bootparam_utils.h   |  2 +-
  arch/x86/include/asm/page_64_types.h |  2 +-
  arch/x86/include/asm/pgtable_64_types.h  |  2 +-
  arch/x86/kernel/cpu/microcode/amd.c  |  2 +-
  arch/x86/kernel/kexec-bzimage64.c|  2 +-
  arch/x86/kernel/pci-dma.c|  2 +-
  arch/x86/mm/tlb.c|  2 +-
  arch/x86/platform/pvh/enlighten.c|  2 +-
  drivers/acpi/Kconfig | 10 +-
  drivers/net/ethernet/faraday/ftgmac100.c |  2 +-
  .../fieldbus/Documentation/fieldbus_dev.txt  |  4 ++--
  drivers/vhost/vhost.c|  2 +-
  include/acpi/acpi_drivers.h  |  2 +-
  include/linux/fs_context.h   |  2 +-
  include/linux/lsm_hooks.h|  2 +-
  mm/Kconfig   |  2 +-
  security/Kconfig |  2 +-
  tools/include/linux/err.h|  2 +-
  tools/objtool/Documentation/stack-validation.txt |  4 ++--
  tools/testing/selftests/x86/protection_keys.c|  2 +-
  48 files changed, 77 insertions(+), 78 deletions(-)


[...]


diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 8c1c636308c8..e868d2bd48b8 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -898,7 +898,7 @@ config PPC_MEM_KEYS
  page-based protections, but without requiring modification of the
  page tables when an application changes protection domains.
  
-	  For details, see Documentation/vm/protection-keys.rst

+ For details, see Documentation/x86/protection-keys.rst


It looks strange to reference an x86 file, for powerpc arch.

Christophe

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization