* [PATCH v4.1 backport 12/15] powerpc/64s: Allow control of RFI flush via debugfs
From: Michael Ellerman @ 2018-03-02 4:45 UTC (permalink / raw)
To: Alexander.Levin; +Cc: stable, linuxppc-dev, npiggin
In-Reply-To: <20180302044549.14930-1-mpe@ellerman.id.au>
commit 236003e6b5443c45c18e613d2b0d776a9f87540e upstream.
Expose the state of the RFI flush (enabled/disabled) via debugfs, and
allow it to be enabled/disabled at runtime.
eg: $ cat /sys/kernel/debug/powerpc/rfi_flush
1
$ echo 0 > /sys/kernel/debug/powerpc/rfi_flush
$ cat /sys/kernel/debug/powerpc/rfi_flush
0
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
arch/powerpc/kernel/setup_64.c | 30 ++++++++++++++++++++++++++++++
1 file changed, 30 insertions(+)
diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
index 55d8da909db5..ec01a98a5978 100644
--- a/arch/powerpc/kernel/setup_64.c
+++ b/arch/powerpc/kernel/setup_64.c
@@ -38,6 +38,7 @@
#include <linux/hugetlb.h>
#include <linux/memory.h>
#include <linux/nmi.h>
+#include <linux/debugfs.h>
#include <asm/io.h>
#include <asm/kdump.h>
@@ -896,5 +897,34 @@ void __init setup_rfi_flush(enum l1d_flush_type types, bool enable)
if (!no_rfi_flush)
rfi_flush_enable(enable);
}
+
+#ifdef CONFIG_DEBUG_FS
+static int rfi_flush_set(void *data, u64 val)
+{
+ if (val == 1)
+ rfi_flush_enable(true);
+ else if (val == 0)
+ rfi_flush_enable(false);
+ else
+ return -EINVAL;
+
+ return 0;
+}
+
+static int rfi_flush_get(void *data, u64 *val)
+{
+ *val = rfi_flush ? 1 : 0;
+ return 0;
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(fops_rfi_flush, rfi_flush_get, rfi_flush_set, "%llu\n");
+
+static __init int rfi_flush_debugfs_init(void)
+{
+ debugfs_create_file("rfi_flush", 0600, powerpc_debugfs_root, NULL, &fops_rfi_flush);
+ return 0;
+}
+device_initcall(rfi_flush_debugfs_init);
+#endif
#endif /* CONFIG_PPC_BOOK3S_64 */
#endif
--
2.14.1
^ permalink raw reply related
* [PATCH v4.1 backport 13/15] powerpc/pseries: include linux/types.h in asm/hvcall.h
From: Michael Ellerman @ 2018-03-02 4:45 UTC (permalink / raw)
To: Alexander.Levin; +Cc: stable, linuxppc-dev, npiggin
In-Reply-To: <20180302044549.14930-1-mpe@ellerman.id.au>
From: Michal Suchanek <msuchanek@suse.de>
commit 1b689a95ce7427075f9ac9fb4aea1af530742b7f upstream.
Commit 6e032b350cd1 ("powerpc/powernv: Check device-tree for RFI flush
settings") uses u64 in asm/hvcall.h without including linux/types.h
This breaks hvcall.h users that do not include the header themselves.
Fixes: 6e032b350cd1 ("powerpc/powernv: Check device-tree for RFI flush settings")
Signed-off-by: Michal Suchanek <msuchanek@suse.de>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
arch/powerpc/include/asm/hvcall.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/powerpc/include/asm/hvcall.h b/arch/powerpc/include/asm/hvcall.h
index 51adbde09845..449bbb87c257 100644
--- a/arch/powerpc/include/asm/hvcall.h
+++ b/arch/powerpc/include/asm/hvcall.h
@@ -298,6 +298,7 @@
#define H_CPU_BEHAV_BNDS_CHK_SPEC_BAR (1ull << 61) // IBM bit 2
#ifndef __ASSEMBLY__
+#include <linux/types.h>
/**
* plpar_hcall_norets: - Make a pseries hypervisor call with no return arguments
--
2.14.1
^ permalink raw reply related
* [PATCH v4.1 backport 14/15] powerpc/ptrace: Fix out of bounds array access warning
From: Michael Ellerman @ 2018-03-02 4:45 UTC (permalink / raw)
To: Alexander.Levin; +Cc: stable, linuxppc-dev, npiggin
In-Reply-To: <20180302044549.14930-1-mpe@ellerman.id.au>
From: Khem Raj <raj.khem@gmail.com>
commit 1e407ee3b21f981140491d5b8a36422979ca246f upstream.
gcc-6 correctly warns about a out of bounds access
arch/powerpc/kernel/ptrace.c:407:24: warning: index 32 denotes an offset greater than size of 'u64[32][1] {aka long long unsigned int[32][1]}' [-Warray-bounds]
offsetof(struct thread_fp_state, fpr[32][0]));
^
check the end of array instead of beginning of next element to fix this
Signed-off-by: Khem Raj <raj.khem@gmail.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Segher Boessenkool <segher@kernel.crashing.org>
Tested-by: Aaro Koskinen <aaro.koskinen@iki.fi>
Acked-by: Olof Johansson <olof@lixom.net>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
arch/powerpc/kernel/ptrace.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index f21897b42057..93f200f14e19 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -376,7 +376,7 @@ static int fpr_get(struct task_struct *target, const struct user_regset *regset,
#else
BUILD_BUG_ON(offsetof(struct thread_fp_state, fpscr) !=
- offsetof(struct thread_fp_state, fpr[32][0]));
+ offsetof(struct thread_fp_state, fpr[32]));
return user_regset_copyout(&pos, &count, &kbuf, &ubuf,
&target->thread.fp_state, 0, -1);
@@ -404,7 +404,7 @@ static int fpr_set(struct task_struct *target, const struct user_regset *regset,
return 0;
#else
BUILD_BUG_ON(offsetof(struct thread_fp_state, fpscr) !=
- offsetof(struct thread_fp_state, fpr[32][0]));
+ offsetof(struct thread_fp_state, fpr[32]));
return user_regset_copyin(&pos, &count, &kbuf, &ubuf,
&target->thread.fp_state, 0, -1);
--
2.14.1
^ permalink raw reply related
* [PATCH v4.1 backport 15/15] powerpc/vdso64: Use double word compare on pointers
From: Michael Ellerman @ 2018-03-02 4:45 UTC (permalink / raw)
To: Alexander.Levin; +Cc: stable, linuxppc-dev, npiggin
In-Reply-To: <20180302044549.14930-1-mpe@ellerman.id.au>
From: Anton Blanchard <anton@samba.org>
commit 5045ea37377ce8cca6890d32b127ad6770e6dce5 upstream.
__kernel_get_syscall_map() and __kernel_clock_getres() use cmpli to
check if the passed in pointer is non zero. cmpli maps to a 32 bit
compare on binutils, so we ignore the top 32 bits.
A simple test case can be created by passing in a bogus pointer with
the bottom 32 bits clear. Using a clk_id that is handled by the VDSO,
then one that is handled by the kernel shows the problem:
printf("%d\n", clock_getres(CLOCK_REALTIME, (void *)0x100000000));
printf("%d\n", clock_getres(CLOCK_BOOTTIME, (void *)0x100000000));
And we get:
0
-1
The bigger issue is if we pass a valid pointer with the bottom 32 bits
clear, in this case we will return success but won't write any data
to the pointer.
I stumbled across this issue because the LLVM integrated assembler
doesn't accept cmpli with 3 arguments. Fix this by converting them to
cmpldi.
Fixes: a7f290dad32e ("[PATCH] powerpc: Merge vdso's and add vdso support to 32 bits kernel")
Cc: stable@vger.kernel.org # v2.6.15+
Signed-off-by: Anton Blanchard <anton@samba.org>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
arch/powerpc/kernel/vdso64/datapage.S | 2 +-
arch/powerpc/kernel/vdso64/gettimeofday.S | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/kernel/vdso64/datapage.S b/arch/powerpc/kernel/vdso64/datapage.S
index 79796de11737..3263ee23170d 100644
--- a/arch/powerpc/kernel/vdso64/datapage.S
+++ b/arch/powerpc/kernel/vdso64/datapage.S
@@ -57,7 +57,7 @@ V_FUNCTION_BEGIN(__kernel_get_syscall_map)
bl V_LOCAL_FUNC(__get_datapage)
mtlr r12
addi r3,r3,CFG_SYSCALL_MAP64
- cmpli cr0,r4,0
+ cmpldi cr0,r4,0
crclr cr0*4+so
beqlr
li r0,__NR_syscalls
diff --git a/arch/powerpc/kernel/vdso64/gettimeofday.S b/arch/powerpc/kernel/vdso64/gettimeofday.S
index a76b4af37ef2..382021324883 100644
--- a/arch/powerpc/kernel/vdso64/gettimeofday.S
+++ b/arch/powerpc/kernel/vdso64/gettimeofday.S
@@ -145,7 +145,7 @@ V_FUNCTION_BEGIN(__kernel_clock_getres)
bne cr0,99f
li r3,0
- cmpli cr0,r4,0
+ cmpldi cr0,r4,0
crclr cr0*4+so
beqlr
lis r5,CLOCK_REALTIME_RES@h
--
2.14.1
^ permalink raw reply related
* [PATCH v3] powerpc/npu-dma.c: Fix deadlock in mmio_invalidate
From: Alistair Popple @ 2018-03-02 5:18 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Balbir Singh, Mark Hairgrove, mpe, Alistair Popple
When sending TLB invalidates to the NPU we need to send extra flushes due
to a hardware issue. The original implementation would lock the all the
ATSD MMIO registers sequentially before unlocking and relocking each of
them sequentially to do the extra flush.
This introduced a deadlock as it is possible for one thread to hold one
ATSD register whilst waiting for another register to be freed while the
other thread is holding that register waiting for the one in the first
thread to be freed.
For example if there are two threads and two ATSD registers:
Thread A Thread B
Acquire 1
Acquire 2
Release 1 Acquire 1
Wait 1 Wait 2
Both threads will be stuck waiting to acquire a register resulting in an
RCU stall warning or soft lockup.
This patch solves the deadlock by refactoring the code to ensure registers
are not released between flushes and to ensure all registers are either
acquired or released together and in order.
Fixes: bbd5ff50afff ("powerpc/powernv/npu-dma: Add explicit flush when sending an ATSD")
Signed-off-by: Alistair Popple <alistair@popple.id.au>
---
v2:
- Added memory barriers around ATSD register aquisition/release
- Added compiler barriers around npdev[][] assignment
v3:
- Added comments to describe required locking
---
arch/powerpc/platforms/powernv/npu-dma.c | 228 +++++++++++++++++++------------
1 file changed, 139 insertions(+), 89 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/npu-dma.c b/arch/powerpc/platforms/powernv/npu-dma.c
index 0a253b64ac5f..0dec96eb3358 100644
--- a/arch/powerpc/platforms/powernv/npu-dma.c
+++ b/arch/powerpc/platforms/powernv/npu-dma.c
@@ -410,6 +410,11 @@ struct npu_context {
void *priv;
};
+struct mmio_atsd_reg {
+ struct npu *npu;
+ int reg;
+};
+
/*
* Find a free MMIO ATSD register and mark it in use. Return -ENOSPC
* if none are available.
@@ -419,7 +424,7 @@ static int get_mmio_atsd_reg(struct npu *npu)
int i;
for (i = 0; i < npu->mmio_atsd_count; i++) {
- if (!test_and_set_bit(i, &npu->mmio_atsd_usage))
+ if (!test_and_set_bit_lock(i, &npu->mmio_atsd_usage))
return i;
}
@@ -428,86 +433,90 @@ static int get_mmio_atsd_reg(struct npu *npu)
static void put_mmio_atsd_reg(struct npu *npu, int reg)
{
- clear_bit(reg, &npu->mmio_atsd_usage);
+ clear_bit_unlock(reg, &npu->mmio_atsd_usage);
}
/* MMIO ATSD register offsets */
#define XTS_ATSD_AVA 1
#define XTS_ATSD_STAT 2
-static int mmio_launch_invalidate(struct npu *npu, unsigned long launch,
- unsigned long va)
+static void mmio_launch_invalidate(struct mmio_atsd_reg *mmio_atsd_reg,
+ unsigned long launch, unsigned long va)
{
- int mmio_atsd_reg;
-
- do {
- mmio_atsd_reg = get_mmio_atsd_reg(npu);
- cpu_relax();
- } while (mmio_atsd_reg < 0);
+ struct npu *npu = mmio_atsd_reg->npu;
+ int reg = mmio_atsd_reg->reg;
__raw_writeq(cpu_to_be64(va),
- npu->mmio_atsd_regs[mmio_atsd_reg] + XTS_ATSD_AVA);
+ npu->mmio_atsd_regs[reg] + XTS_ATSD_AVA);
eieio();
- __raw_writeq(cpu_to_be64(launch), npu->mmio_atsd_regs[mmio_atsd_reg]);
-
- return mmio_atsd_reg;
+ __raw_writeq(cpu_to_be64(launch), npu->mmio_atsd_regs[reg]);
}
-static int mmio_invalidate_pid(struct npu *npu, unsigned long pid, bool flush)
+static void mmio_invalidate_pid(struct mmio_atsd_reg mmio_atsd_reg[NV_MAX_NPUS],
+ unsigned long pid, bool flush)
{
+ int i;
unsigned long launch;
- /* IS set to invalidate matching PID */
- launch = PPC_BIT(12);
+ for (i = 0; i <= max_npu2_index; i++) {
+ if (mmio_atsd_reg[i].reg < 0)
+ continue;
+
+ /* IS set to invalidate matching PID */
+ launch = PPC_BIT(12);
- /* PRS set to process-scoped */
- launch |= PPC_BIT(13);
+ /* PRS set to process-scoped */
+ launch |= PPC_BIT(13);
- /* AP */
- launch |= (u64) mmu_get_ap(mmu_virtual_psize) << PPC_BITLSHIFT(17);
+ /* AP */
+ launch |= (u64)
+ mmu_get_ap(mmu_virtual_psize) << PPC_BITLSHIFT(17);
- /* PID */
- launch |= pid << PPC_BITLSHIFT(38);
+ /* PID */
+ launch |= pid << PPC_BITLSHIFT(38);
- /* No flush */
- launch |= !flush << PPC_BITLSHIFT(39);
+ /* No flush */
+ launch |= !flush << PPC_BITLSHIFT(39);
- /* Invalidating the entire process doesn't use a va */
- return mmio_launch_invalidate(npu, launch, 0);
+ /* Invalidating the entire process doesn't use a va */
+ mmio_launch_invalidate(&mmio_atsd_reg[i], launch, 0);
+ }
}
-static int mmio_invalidate_va(struct npu *npu, unsigned long va,
- unsigned long pid, bool flush)
+static void mmio_invalidate_va(struct mmio_atsd_reg mmio_atsd_reg[NV_MAX_NPUS],
+ unsigned long va, unsigned long pid, bool flush)
{
+ int i;
unsigned long launch;
- /* IS set to invalidate target VA */
- launch = 0;
+ for (i = 0; i <= max_npu2_index; i++) {
+ if (mmio_atsd_reg[i].reg < 0)
+ continue;
- /* PRS set to process scoped */
- launch |= PPC_BIT(13);
+ /* IS set to invalidate target VA */
+ launch = 0;
- /* AP */
- launch |= (u64) mmu_get_ap(mmu_virtual_psize) << PPC_BITLSHIFT(17);
+ /* PRS set to process scoped */
+ launch |= PPC_BIT(13);
- /* PID */
- launch |= pid << PPC_BITLSHIFT(38);
+ /* AP */
+ launch |= (u64)
+ mmu_get_ap(mmu_virtual_psize) << PPC_BITLSHIFT(17);
- /* No flush */
- launch |= !flush << PPC_BITLSHIFT(39);
+ /* PID */
+ launch |= pid << PPC_BITLSHIFT(38);
- return mmio_launch_invalidate(npu, launch, va);
+ /* No flush */
+ launch |= !flush << PPC_BITLSHIFT(39);
+
+ mmio_launch_invalidate(&mmio_atsd_reg[i], launch, va);
+ }
}
#define mn_to_npu_context(x) container_of(x, struct npu_context, mn)
-struct mmio_atsd_reg {
- struct npu *npu;
- int reg;
-};
-
static void mmio_invalidate_wait(
- struct mmio_atsd_reg mmio_atsd_reg[NV_MAX_NPUS], bool flush)
+ struct mmio_atsd_reg mmio_atsd_reg[NV_MAX_NPUS])
{
struct npu *npu;
int i, reg;
@@ -522,16 +531,65 @@ static void mmio_invalidate_wait(
reg = mmio_atsd_reg[i].reg;
while (__raw_readq(npu->mmio_atsd_regs[reg] + XTS_ATSD_STAT))
cpu_relax();
+ }
+}
- put_mmio_atsd_reg(npu, reg);
+/*
+ * Acquires all the address translation shootdown (ATSD) registers required to
+ * launch an ATSD on all links this npu_context is active on.
+ */
+static void acquire_atsd_reg(struct npu_context *npu_context,
+ struct mmio_atsd_reg mmio_atsd_reg[NV_MAX_NPUS])
+{
+ int i, j;
+ struct npu *npu;
+ struct pci_dev *npdev;
+ struct pnv_phb *nphb;
- /*
- * The GPU requires two flush ATSDs to ensure all entries have
- * been flushed. We use PID 0 as it will never be used for a
- * process on the GPU.
+ for (i = 0; i <= max_npu2_index; i++) {
+ mmio_atsd_reg[i].reg = -1;
+ for (j = 0; j < NV_MAX_LINKS; j++) {
+ /* There are no ordering requirements with respect to
+ * the setup of struct npu_context, but to ensure
+ * consistent behaviour we need to ensure npdev[][] is
+ * only read once.
+ */
+ npdev = READ_ONCE(npu_context->npdev[i][j]);
+ if (!npdev)
+ continue;
+
+ nphb = pci_bus_to_host(npdev->bus)->private_data;
+ npu = &nphb->npu;
+ mmio_atsd_reg[i].npu = npu;
+ mmio_atsd_reg[i].reg = get_mmio_atsd_reg(npu);
+ while (mmio_atsd_reg[i].reg < 0) {
+ mmio_atsd_reg[i].reg = get_mmio_atsd_reg(npu);
+ cpu_relax();
+ }
+ break;
+ }
+ }
+}
+
+/*
+ * Release previously acquired ATSD registers. To avoid deadlocks the registers
+ * must be released in the same order they were acquired above in
+ * acquire_atsd_reg.
+ */
+static void release_atsd_reg(struct mmio_atsd_reg mmio_atsd_reg[NV_MAX_NPUS])
+{
+ int i;
+
+ for (i = 0; i <= max_npu2_index; i++) {
+ /* We can't rely on npu_context->npdev[][] being the same here
+ * as when acquire_atsd_reg() was called, hence we use the
+ * values stored in mmio_atsd_reg during the acquire phase
+ * rather than re-reading npdev[][]
*/
- if (flush)
- mmio_invalidate_pid(npu, 0, true);
+ if (mmio_atsd_reg[i].reg < 0)
+ continue;
+
+ put_mmio_atsd_reg(mmio_atsd_reg[i].npu, mmio_atsd_reg[i].reg);
}
}
@@ -542,10 +600,6 @@ static void mmio_invalidate_wait(
static void mmio_invalidate(struct npu_context *npu_context, int va,
unsigned long address, bool flush)
{
- int i, j;
- struct npu *npu;
- struct pnv_phb *nphb;
- struct pci_dev *npdev;
struct mmio_atsd_reg mmio_atsd_reg[NV_MAX_NPUS];
unsigned long pid = npu_context->mm->context.id;
@@ -561,37 +615,25 @@ static void mmio_invalidate(struct npu_context *npu_context, int va,
* Loop over all the NPUs this process is active on and launch
* an invalidate.
*/
- for (i = 0; i <= max_npu2_index; i++) {
- mmio_atsd_reg[i].reg = -1;
- for (j = 0; j < NV_MAX_LINKS; j++) {
- npdev = npu_context->npdev[i][j];
- if (!npdev)
- continue;
-
- nphb = pci_bus_to_host(npdev->bus)->private_data;
- npu = &nphb->npu;
- mmio_atsd_reg[i].npu = npu;
-
- if (va)
- mmio_atsd_reg[i].reg =
- mmio_invalidate_va(npu, address, pid,
- flush);
- else
- mmio_atsd_reg[i].reg =
- mmio_invalidate_pid(npu, pid, flush);
-
- /*
- * The NPU hardware forwards the shootdown to all GPUs
- * so we only have to launch one shootdown per NPU.
- */
- break;
- }
+ acquire_atsd_reg(npu_context, mmio_atsd_reg);
+ if (va)
+ mmio_invalidate_va(mmio_atsd_reg, address, pid, flush);
+ else
+ mmio_invalidate_pid(mmio_atsd_reg, pid, flush);
+
+ mmio_invalidate_wait(mmio_atsd_reg);
+ if (flush) {
+ /*
+ * The GPU requires two flush ATSDs to ensure all entries have
+ * been flushed. We use PID 0 as it will never be used for a
+ * process on the GPU.
+ */
+ mmio_invalidate_pid(mmio_atsd_reg, 0, true);
+ mmio_invalidate_wait(mmio_atsd_reg);
+ mmio_invalidate_pid(mmio_atsd_reg, 0, true);
+ mmio_invalidate_wait(mmio_atsd_reg);
}
-
- mmio_invalidate_wait(mmio_atsd_reg, flush);
- if (flush)
- /* Wait for the flush to complete */
- mmio_invalidate_wait(mmio_atsd_reg, false);
+ release_atsd_reg(mmio_atsd_reg);
}
static void pnv_npu2_mn_release(struct mmu_notifier *mn,
@@ -726,7 +768,15 @@ struct npu_context *pnv_npu2_init_context(struct pci_dev *gpdev,
if (WARN_ON(of_property_read_u32(nvlink_dn, "ibm,npu-link-index",
&nvlink_index)))
return ERR_PTR(-ENODEV);
- npu_context->npdev[npu->index][nvlink_index] = npdev;
+
+ /* npdev is a pci_dev pointer setup by the PCI code. We assign it to
+ * npdev[][] to indicate to the mmu notifiers that an invalidation
+ * should also be sent over this nvlink. The notifiers don't use any
+ * other fields in npu_context, so we just need to ensure that when they
+ * deference npu_context->npdev[][] it is either a valid pointer or
+ * NULL.
+ */
+ WRITE_ONCE(npu_context->npdev[npu->index][nvlink_index], npdev);
if (!nphb->npu.nmmu_flush) {
/*
@@ -778,7 +828,7 @@ void pnv_npu2_destroy_context(struct npu_context *npu_context,
if (WARN_ON(of_property_read_u32(nvlink_dn, "ibm,npu-link-index",
&nvlink_index)))
return;
- npu_context->npdev[npu->index][nvlink_index] = NULL;
+ WRITE_ONCE(npu_context->npdev[npu->index][nvlink_index], NULL);
opal_npu_destroy_context(nphb->opal_id, npu_context->mm->context.id,
PCI_DEVID(gpdev->bus->number, gpdev->devfn));
kref_put(&npu_context->kref, pnv_npu2_release_context);
--
2.11.0
^ permalink raw reply related
* Re: [PATCH 00/23] kconfig: move compiler capability tests to Kconfig
From: Masahiro Yamada @ 2018-03-02 5:50 UTC (permalink / raw)
To: Ulf Magnusson
Cc: Arnd Bergmann, Rich Felker, Kernel Hardening, X86 ML,
Paul Mackerras, H. Peter Anvin, sparclinux, Sam Ravnborg,
Yoshinori Sato, Jonathan Corbet, Richard Weinberger,
Linux-sh list, Ingo Molnar, Emese Revfy, Kees Cook, uml-devel,
Linux Kbuild mailing list, Peter Oberparleiter, Jeff Dike,
linuxppc-dev, user-mode-linux-user, Thomas Gleixner, Michal Marek,
Greg Kroah-Hartman, Randy Dunlap, open list:DOCUMENTATION,
Linux Kernel Mailing List, Linus Torvalds, David S. Miller
In-Reply-To: <20180221213946.w4v6ywy7fbiy6oyc@huvuddator>
2018-02-22 6:39 GMT+09:00 Ulf Magnusson <ulfalizer@gmail.com>:
> On Wed, Feb 21, 2018 at 09:57:03PM +0900, Masahiro Yamada wrote:
>> 2018-02-21 19:52 GMT+09:00 Arnd Bergmann <arnd@arndb.de>:
>> > On Wed, Feb 21, 2018 at 11:20 AM, Masahiro Yamada
>> > <yamada.masahiro@socionext.com> wrote:
>> >> 2018-02-21 18:56 GMT+09:00 Arnd Bergmann <arnd@arndb.de>:
>> >>> On Wed, Feb 21, 2018 at 8:38 AM, Masahiro Yamada
>> >>> <yamada.masahiro@socionext.com> wrote:
>> >>>> 2018-02-20 0:18 GMT+09:00 Ulf Magnusson <ulfalizer@gmail.com>:
>> >>
>> >> Let me clarify my concern.
>> >>
>> >> When we test the compiler flag, is there a case
>> >> where a particular flag depends on -m{32,64} ?
>> >>
>> >> For example, is there a compiler that supports -fstack-protector
>> >> for 64bit mode, but unsupports it for 32bit mode?
>> >>
>> >> $(cc-option -m32) -> y
>> >> $(cc-option -m64) -> y
>> >> $(cc-option -fstack-protector) -> y
>> >> $(cc-option -m32 -fstack-protector) -> n
>> >> $(cc-option -m64 -fstack-protector) -> y
>> >>
>> >> I guess this is unlikely to happen,
>> >> but I am not whether it is zero possibility.
>> >>
>> >> If this could happen,
>> >> $(cc-option ) must be evaluated together with
>> >> correct bi-arch option (either -m32 or -m64).
>> >>
>> >>
>> >> Currently, -m32/-m64 is specified in Makefile,
>> >> but we are moving compiler tests to Kconfig
>> >> and, CONFIG_64BIT can be dynamically toggled in Kconfig.
>> >
>> > I don't think it can happen for this particular combination (stack protector
>> > and word size), but I'm sure we'll eventually run into options that
>> > need to be tested in combination. For the current CFLAGS_KERNEL
>> > setting, we definitely have the case of needing the variables to be
>> > evaluated in a specific order.
>> >
>>
>>
>>
>>
>> I was thinking of how we can handle complex cases
>> in the current approach.
>>
>>
>>
>> (Case 1)
>>
>> Compiler flag -foo and -bar interacts, so
>> we also need to check the combination of the two.
>>
>>
>> config CC_HAS_FOO
>> def_bool $(cc-option -foo)
>>
>> config CC_HAS_BAR
>> def_bool $(cc-option -bar)
>>
>> config CC_HAS_FOO_WITH_BAR
>> def_bool $(cc-option -foo -bar)
>>
>>
>>
>> (Case 2)
>> Compiler flag -foo is sensitive to word-size.
>> So, we need to test this option together with -m32/-m64.
>> User can toggle CONFIG_64BIT, like i386/x86_64.
>>
>>
>> config CC_NEEDS_M64
>> def_bool $(cc-option -m64) && 64BIT
>>
>> config CC_NEEDS_M32
>> def_bool $(cc-option -m32) && !64BIT
>>
>> config CC_HAS_FOO
>> bool
>> default $(cc-option -m64 -foo) if CC_NEEDS_M64
>> default $(cc-option -m32 -foo) if CC_NEEDS_M32
>> default $(cc-option -foo)
>>
>>
>>
>> (Case 3)
>> Compiler flag -foo is sensitive to endian-ness.
>>
>>
>> config CC_NEEDS_BIG_ENDIAN
>> def_bool $(cc-option -mbig-endian) && CPU_BIG_ENDIAN
>>
>> config CC_NEEDS_LITTLE_ENDIAN
>> def_bool $(cc-option -mlittle-endian) && CPU_LITTLE_ENDIAN
>>
>> config CC_HAS_FOO
>> bool
>> default $(cc-option -mbig-endian -foo) if CC_NEEDS_BIG_ENDIAN
>> default $(cc-option -mlittle-endian -foo) if CC_NEEDS_LITTLE_ENDIAN
>> default $(cc-option -foo)
>>
>>
>>
>>
>> Hmm, I think I can implement those somehow.
>> But, I hope we do not have many instances like this...
>>
>>
>> If you know more naive cases, please share your knowledge.
>>
>> Thanks!
>>
>>
>> --
>> Best Regards
>> Masahiro Yamada
>
> Would get pretty bad if a test needs to consider multiple symbols.
> Exponential explosion there...
>
>
> I thought some more about the implementation of dynamic (post-parsing)
> functions to see how bad it would get with the current implementation.
>
> Some background on how things work now:
>
> 1. All expression operands in Kconfig are symbols.
>
> 2. Returning '$ENV' or '$(fn foo)' as a T_WORD during parsing gets
> you symbols with those strings as names and S_UNKNOWN type (because
> they act like references to undefined symbols).
>
> 3. For "foo-$(fn foo)", you also get a symbol with that string as its
> name and S_UNKNOWN type (stored among the SYMBOL_CONST symbols)
>
> 4. Symbols with S_UNKNOWN type get their name as their string value,
> and the tristate value n.
>
> So, if you do string expansion on the names of symbols with S_UNKNOWN
> type in sym_calc_value(), you're almost there with the current
> implementation, except for the tristate case.
>
> Maybe you could set the tristate value of S_UNKNOWN symbols depending on
> the string value you end up with. Things are getting pretty confusing at
> that point.
>
> Could have something like S_DYNAMIC as well. More Kconfig complexity...
>
> Then there's other complications:
>
> 1. SYMBOL_CONST is no longer constant.
>
> 2. Dependency loop detection needs to consider symbol references
> within strings.
>
> 3. Dependency loop detection relies on static knowledge of what
> symbols a symbol depends on. That might get messy for certain
> expansions, though it might be things you wouldn't do in practice.
>
> 4. Symbols still need to be properly invalidated. It looks like at
> least menuconfig just does a dumb invalidate-everything whenever
> the value of a symbol is changed though, so it might not require
> extra work. (Bit messier in Kconfiglib, which does minimal
> invalidation to keep scripts fast, but just need to extract a few
> extra deps there.)
>
>
> It looks like dynamic functions could get quite messy, but might be
> doable if absolutely required. There's probably more devils in the
> details though.
>
> I don't think the static function model precludes switching models later
> btw, when people have more experience.
I really want to start with the static function model
and see if we need the dynamic function implementation.
Here is an idea for the migration path in case
we need to do that in the future.
Currently, every time user input is given,
sym_clear_all_valid() is called.
It is not efficient to blindly re-evaluate expensive $(shell ...)
So, have a list of symbols the function depends on
in its arguments.
For example,
config CC_HAS_SANE_STACKPROTECTOR
def_bool $(shell $srctree/scripts/gcc-has-stack-protector.sh
$CC $(1), CFLAGS_BASE)
Here the first argument
$srctree/scripts/gcc-x86-has-stack-protector.sh $CC $(1)
is the shell command.
$(1), $(2), ... will be replaced with the values of symbols (or expressions)
that follow when running the shell command.
The second argument
CFLAGS_BASE
is the dependency symbol (or expression).
CFLAGS_BASE can be dynamically changed like
config CFLAGS_BASE
string
default "-m64" if 64BIT
default "-m32"
When and only when CFLAGS_BASE is updated, the function should be re-calculated.
(This will require efforts to minimize the amount of re-evaluation.)
cc-option will be implemented like follows:
macro cc-option $(shell $CC -Werror $$(1) $(1) -c -x c /dev/null -o
/dev/null, CFLAGS_BASE)
Please notice the difference between $$(1) and $(1).
$(1) is immediately expanded by cc-option macro.
$$(1) is escaped since we want to expand it by $(shell ...), not by
$(cc-option ...)
For example,
$(cc-option -fstack-protector)
will be expanded to
$(shell gcc -Werror $(1) -fstack-protector -c -x c /dev/null -o
/dev/null, CFLAGS_BASE)
Since macros are just textual shorthand, so this expansion happens
during the parse phase.
Then, the evaluation phase does the following every time CFLAGS_BASE is updated.
gcc -Werror [value of CFLAGS_BASE] -fstack-protector -c -x c /dev/null
-o /dev/null
This is a new form of expression, so it will be managed in AST tree
with a flag E_SHELL (or E_FUNC) etc.
Not implemented at all. Just a rough sketch.
--
Best Regards
Masahiro Yamada
^ permalink raw reply
* Re: Hotplug + Reboot is crashing HPT guest with HPT resizing enabled
From: Bharata B Rao @ 2018-03-02 8:12 UTC (permalink / raw)
To: David Gibson; +Cc: linuxppc-dev
In-Reply-To: <20180302022144.GG13135@umbus.fritz.box>
[-- Attachment #1: Type: text/plain, Size: 1950 bytes --]
On Fri, Mar 2, 2018 at 7:51 AM, David Gibson <david@gibson.dropbear.id.au>
wrote:
> On Fri, Feb 23, 2018 at 03:02:40PM +0530, Bharata B Rao wrote:
> > Hi,
> >
> > Rebooting a hash guest after hotplugging memory to it is crashing the
> > guest. This is seen only when HPT resizing is enabled. I see guest
> crashing
> > at multiple places, but this location is fairly commonly seen:
> >
> > kernel BUG at mm/slub.c:3912!
> >
> > Testing with latest guest kernel and ppc-for-2.12 branch of QEMU.
>
> Ugh. We had several bugs along these lines, but I thought I'd fixed
> them. I wonder what this one is.
>
> > A bit of debugging shows me that when memory is added, the guest kernel
> > tries to resize HPT to a htab_shift value lesser than the value with
> which
> > the guest has booted. For eg. a 8GB guest boots with htab_shift of 26.
> When
> > 1G is hot-added,
> > arch/powerpc/mm/hash_utils_64.c:resize_hpt_for_hotplug() ends up
> assigning
> > 24 to target_hpt_shift. This looks suspicious as we are increasing the
> > memory, but kernel is asking for shrinking the HPT size.
>
> So the shrink-HPT-on-add-memory is actually expected and should be
> harmless. It occurs because qemu estimates HPT size on the
> traditional HPT == RAM size / 64 formular, which was devised with 4k
> pages in mind. The kernel on the other hand, knows it is using 64k
> pages and so estimates a smaller HPT size. Hot plugging memory always
> prompts the guest to re-estimate the required HPT size, but if the
> added memory is small enough, that size can still be smaller than
> qemu's initial guess.
>
Thanks for the clarification.
>
> > HPT resizing
> > requests fail though, but next reboot crashes the guest.
>
> As noted the shrink is expected, so we need to debug the crash
> separately. Do you have 9478956794c11239b7c1c3ef9ce95c883bb839a3 in
> your tree?
>
I do now and I no longer see the crash that I was observing last week.
Regards,
Bharata.
[-- Attachment #2: Type: text/html, Size: 2698 bytes --]
^ permalink raw reply
* Re: [PATCH 00/23] kconfig: move compiler capability tests to Kconfig
From: Ulf Magnusson @ 2018-03-02 9:03 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Arnd Bergmann, Rich Felker, Kernel Hardening, X86 ML,
Paul Mackerras, H. Peter Anvin, sparclinux, Sam Ravnborg,
Yoshinori Sato, Jonathan Corbet, Richard Weinberger,
Linux-sh list, Ingo Molnar, Emese Revfy, Kees Cook, uml-devel,
Linux Kbuild mailing list, Peter Oberparleiter, Jeff Dike,
linuxppc-dev, user-mode-linux-user, Thomas Gleixner, Michal Marek,
Greg Kroah-Hartman, Randy Dunlap, open list:DOCUMENTATION,
Linux Kernel Mailing List, Linus Torvalds, David S. Miller
In-Reply-To: <CAK7LNARd-9KNRTHT3jrNganqLisheAWbDQMGi_ujp+1GpYzSOg@mail.gmail.com>
On Fri, Mar 02, 2018 at 02:50:39PM +0900, Masahiro Yamada wrote:
> 2018-02-22 6:39 GMT+09:00 Ulf Magnusson <ulfalizer@gmail.com>:
> > On Wed, Feb 21, 2018 at 09:57:03PM +0900, Masahiro Yamada wrote:
> >> 2018-02-21 19:52 GMT+09:00 Arnd Bergmann <arnd@arndb.de>:
> >> > On Wed, Feb 21, 2018 at 11:20 AM, Masahiro Yamada
> >> > <yamada.masahiro@socionext.com> wrote:
> >> >> 2018-02-21 18:56 GMT+09:00 Arnd Bergmann <arnd@arndb.de>:
> >> >>> On Wed, Feb 21, 2018 at 8:38 AM, Masahiro Yamada
> >> >>> <yamada.masahiro@socionext.com> wrote:
> >> >>>> 2018-02-20 0:18 GMT+09:00 Ulf Magnusson <ulfalizer@gmail.com>:
> >> >>
> >> >> Let me clarify my concern.
> >> >>
> >> >> When we test the compiler flag, is there a case
> >> >> where a particular flag depends on -m{32,64} ?
> >> >>
> >> >> For example, is there a compiler that supports -fstack-protector
> >> >> for 64bit mode, but unsupports it for 32bit mode?
> >> >>
> >> >> $(cc-option -m32) -> y
> >> >> $(cc-option -m64) -> y
> >> >> $(cc-option -fstack-protector) -> y
> >> >> $(cc-option -m32 -fstack-protector) -> n
> >> >> $(cc-option -m64 -fstack-protector) -> y
> >> >>
> >> >> I guess this is unlikely to happen,
> >> >> but I am not whether it is zero possibility.
> >> >>
> >> >> If this could happen,
> >> >> $(cc-option ) must be evaluated together with
> >> >> correct bi-arch option (either -m32 or -m64).
> >> >>
> >> >>
> >> >> Currently, -m32/-m64 is specified in Makefile,
> >> >> but we are moving compiler tests to Kconfig
> >> >> and, CONFIG_64BIT can be dynamically toggled in Kconfig.
> >> >
> >> > I don't think it can happen for this particular combination (stack protector
> >> > and word size), but I'm sure we'll eventually run into options that
> >> > need to be tested in combination. For the current CFLAGS_KERNEL
> >> > setting, we definitely have the case of needing the variables to be
> >> > evaluated in a specific order.
> >> >
> >>
> >>
> >>
> >>
> >> I was thinking of how we can handle complex cases
> >> in the current approach.
> >>
> >>
> >>
> >> (Case 1)
> >>
> >> Compiler flag -foo and -bar interacts, so
> >> we also need to check the combination of the two.
> >>
> >>
> >> config CC_HAS_FOO
> >> def_bool $(cc-option -foo)
> >>
> >> config CC_HAS_BAR
> >> def_bool $(cc-option -bar)
> >>
> >> config CC_HAS_FOO_WITH_BAR
> >> def_bool $(cc-option -foo -bar)
> >>
> >>
> >>
> >> (Case 2)
> >> Compiler flag -foo is sensitive to word-size.
> >> So, we need to test this option together with -m32/-m64.
> >> User can toggle CONFIG_64BIT, like i386/x86_64.
> >>
> >>
> >> config CC_NEEDS_M64
> >> def_bool $(cc-option -m64) && 64BIT
> >>
> >> config CC_NEEDS_M32
> >> def_bool $(cc-option -m32) && !64BIT
> >>
> >> config CC_HAS_FOO
> >> bool
> >> default $(cc-option -m64 -foo) if CC_NEEDS_M64
> >> default $(cc-option -m32 -foo) if CC_NEEDS_M32
> >> default $(cc-option -foo)
> >>
> >>
> >>
> >> (Case 3)
> >> Compiler flag -foo is sensitive to endian-ness.
> >>
> >>
> >> config CC_NEEDS_BIG_ENDIAN
> >> def_bool $(cc-option -mbig-endian) && CPU_BIG_ENDIAN
> >>
> >> config CC_NEEDS_LITTLE_ENDIAN
> >> def_bool $(cc-option -mlittle-endian) && CPU_LITTLE_ENDIAN
> >>
> >> config CC_HAS_FOO
> >> bool
> >> default $(cc-option -mbig-endian -foo) if CC_NEEDS_BIG_ENDIAN
> >> default $(cc-option -mlittle-endian -foo) if CC_NEEDS_LITTLE_ENDIAN
> >> default $(cc-option -foo)
> >>
> >>
> >>
> >>
> >> Hmm, I think I can implement those somehow.
> >> But, I hope we do not have many instances like this...
> >>
> >>
> >> If you know more naive cases, please share your knowledge.
> >>
> >> Thanks!
> >>
> >>
> >> --
> >> Best Regards
> >> Masahiro Yamada
> >
> > Would get pretty bad if a test needs to consider multiple symbols.
> > Exponential explosion there...
> >
> >
> > I thought some more about the implementation of dynamic (post-parsing)
> > functions to see how bad it would get with the current implementation.
> >
> > Some background on how things work now:
> >
> > 1. All expression operands in Kconfig are symbols.
> >
> > 2. Returning '$ENV' or '$(fn foo)' as a T_WORD during parsing gets
> > you symbols with those strings as names and S_UNKNOWN type (because
> > they act like references to undefined symbols).
> >
> > 3. For "foo-$(fn foo)", you also get a symbol with that string as its
> > name and S_UNKNOWN type (stored among the SYMBOL_CONST symbols)
> >
> > 4. Symbols with S_UNKNOWN type get their name as their string value,
> > and the tristate value n.
> >
> > So, if you do string expansion on the names of symbols with S_UNKNOWN
> > type in sym_calc_value(), you're almost there with the current
> > implementation, except for the tristate case.
> >
> > Maybe you could set the tristate value of S_UNKNOWN symbols depending on
> > the string value you end up with. Things are getting pretty confusing at
> > that point.
> >
> > Could have something like S_DYNAMIC as well. More Kconfig complexity...
> >
> > Then there's other complications:
> >
> > 1. SYMBOL_CONST is no longer constant.
> >
> > 2. Dependency loop detection needs to consider symbol references
> > within strings.
> >
> > 3. Dependency loop detection relies on static knowledge of what
> > symbols a symbol depends on. That might get messy for certain
> > expansions, though it might be things you wouldn't do in practice.
> >
> > 4. Symbols still need to be properly invalidated. It looks like at
> > least menuconfig just does a dumb invalidate-everything whenever
> > the value of a symbol is changed though, so it might not require
> > extra work. (Bit messier in Kconfiglib, which does minimal
> > invalidation to keep scripts fast, but just need to extract a few
> > extra deps there.)
> >
> >
> > It looks like dynamic functions could get quite messy, but might be
> > doable if absolutely required. There's probably more devils in the
> > details though.
> >
> > I don't think the static function model precludes switching models later
> > btw, when people have more experience.
>
>
>
> I really want to start with the static function model
> and see if we need the dynamic function implementation.
Yeah, let's start with static functions, IMO.
Either we'll learn that they're powerful enough in practice, and save
ourselves some work, or we'll gain experience for later. Converting from
static to dynamic functions should be painless, if needed.
My plan would be something like:
1. Implement static functions
2. Convert as many simple cases over to them as possible
3. See how bad the bad cases get. If they get really bad, then decide
what to do next (extend Kconfig, handle them in the Makefiles,
etc.)
>
> Here is an idea for the migration path in case
> we need to do that in the future.
>
>
>
> Currently, every time user input is given,
> sym_clear_all_valid() is called.
>
> It is not efficient to blindly re-evaluate expensive $(shell ...)
I think menuconfig only reevalutes the symbols in the menu that's
currently shown in the interface (along with their dependencies).
Maybe that'd be bad enough though.
>
>
> So, have a list of symbols the function depends on
> in its arguments.
>
> For example,
>
> config CC_HAS_SANE_STACKPROTECTOR
> def_bool $(shell $srctree/scripts/gcc-has-stack-protector.sh
> $CC $(1), CFLAGS_BASE)
>
>
> Here the first argument
> $srctree/scripts/gcc-x86-has-stack-protector.sh $CC $(1)
>
> is the shell command.
> $(1), $(2), ... will be replaced with the values of symbols (or expressions)
> that follow when running the shell command.
>
>
> The second argument
> CFLAGS_BASE
> is the dependency symbol (or expression).
>
>
> CFLAGS_BASE can be dynamically changed like
>
> config CFLAGS_BASE
> string
> default "-m64" if 64BIT
> default "-m32"
>
>
> When and only when CFLAGS_BASE is updated, the function should be re-calculated.
> (This will require efforts to minimize the amount of re-evaluation.)
>
>
>
>
> cc-option will be implemented like follows:
>
> macro cc-option $(shell $CC -Werror $$(1) $(1) -c -x c /dev/null -o
> /dev/null, CFLAGS_BASE)
>
>
>
> Please notice the difference between $$(1) and $(1).
>
> $(1) is immediately expanded by cc-option macro.
>
> $$(1) is escaped since we want to expand it by $(shell ...), not by
> $(cc-option ...)
>
>
>
> For example,
>
> $(cc-option -fstack-protector)
>
> will be expanded to
>
> $(shell gcc -Werror $(1) -fstack-protector -c -x c /dev/null -o
> /dev/null, CFLAGS_BASE)
>
> Since macros are just textual shorthand, so this expansion happens
> during the parse phase.
>
>
>
> Then, the evaluation phase does the following every time CFLAGS_BASE is updated.
>
> gcc -Werror [value of CFLAGS_BASE] -fstack-protector -c -x c /dev/null
> -o /dev/null
>
>
> This is a new form of expression, so it will be managed in AST tree
> with a flag E_SHELL (or E_FUNC) etc.
>
>
> Not implemented at all. Just a rough sketch.
A simpler syntax like
$(shell $CC -Werror {CFLAGS_BASE} -c -x c /dev/null -o /dev/null)
might work as well. Then you wouldn't have to any double escaping, and
having both $(1) and the value for it appear in the same place seems a
bit redundant.
I wonder if dependency management might get messy...
Anyway, let's just go with static functions...
Cheers,
Ulf
^ permalink raw reply
* Re: [PATCH 00/23] kconfig: move compiler capability tests to Kconfig
From: Ulf Magnusson @ 2018-03-02 9:12 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Arnd Bergmann, Rich Felker, Kernel Hardening, X86 ML,
Paul Mackerras, H. Peter Anvin, sparclinux, Sam Ravnborg,
Yoshinori Sato, Jonathan Corbet, Richard Weinberger,
Linux-sh list, Ingo Molnar, Emese Revfy, Kees Cook, uml-devel,
Linux Kbuild mailing list, Peter Oberparleiter, Jeff Dike,
linuxppc-dev, user-mode-linux-user, Thomas Gleixner, Michal Marek,
Greg Kroah-Hartman, Randy Dunlap, open list:DOCUMENTATION,
Linux Kernel Mailing List, Linus Torvalds, David S. Miller
In-Reply-To: <20180302090326.h55wontzv3traxnn@huvuddator>
On Fri, Mar 02, 2018 at 10:03:26AM +0100, Ulf Magnusson wrote:
> On Fri, Mar 02, 2018 at 02:50:39PM +0900, Masahiro Yamada wrote:
> > 2018-02-22 6:39 GMT+09:00 Ulf Magnusson <ulfalizer@gmail.com>:
> > > On Wed, Feb 21, 2018 at 09:57:03PM +0900, Masahiro Yamada wrote:
> > >> 2018-02-21 19:52 GMT+09:00 Arnd Bergmann <arnd@arndb.de>:
> > >> > On Wed, Feb 21, 2018 at 11:20 AM, Masahiro Yamada
> > >> > <yamada.masahiro@socionext.com> wrote:
> > >> >> 2018-02-21 18:56 GMT+09:00 Arnd Bergmann <arnd@arndb.de>:
> > >> >>> On Wed, Feb 21, 2018 at 8:38 AM, Masahiro Yamada
> > >> >>> <yamada.masahiro@socionext.com> wrote:
> > >> >>>> 2018-02-20 0:18 GMT+09:00 Ulf Magnusson <ulfalizer@gmail.com>:
> > >> >>
> > >> >> Let me clarify my concern.
> > >> >>
> > >> >> When we test the compiler flag, is there a case
> > >> >> where a particular flag depends on -m{32,64} ?
> > >> >>
> > >> >> For example, is there a compiler that supports -fstack-protector
> > >> >> for 64bit mode, but unsupports it for 32bit mode?
> > >> >>
> > >> >> $(cc-option -m32) -> y
> > >> >> $(cc-option -m64) -> y
> > >> >> $(cc-option -fstack-protector) -> y
> > >> >> $(cc-option -m32 -fstack-protector) -> n
> > >> >> $(cc-option -m64 -fstack-protector) -> y
> > >> >>
> > >> >> I guess this is unlikely to happen,
> > >> >> but I am not whether it is zero possibility.
> > >> >>
> > >> >> If this could happen,
> > >> >> $(cc-option ) must be evaluated together with
> > >> >> correct bi-arch option (either -m32 or -m64).
> > >> >>
> > >> >>
> > >> >> Currently, -m32/-m64 is specified in Makefile,
> > >> >> but we are moving compiler tests to Kconfig
> > >> >> and, CONFIG_64BIT can be dynamically toggled in Kconfig.
> > >> >
> > >> > I don't think it can happen for this particular combination (stack protector
> > >> > and word size), but I'm sure we'll eventually run into options that
> > >> > need to be tested in combination. For the current CFLAGS_KERNEL
> > >> > setting, we definitely have the case of needing the variables to be
> > >> > evaluated in a specific order.
> > >> >
> > >>
> > >>
> > >>
> > >>
> > >> I was thinking of how we can handle complex cases
> > >> in the current approach.
> > >>
> > >>
> > >>
> > >> (Case 1)
> > >>
> > >> Compiler flag -foo and -bar interacts, so
> > >> we also need to check the combination of the two.
> > >>
> > >>
> > >> config CC_HAS_FOO
> > >> def_bool $(cc-option -foo)
> > >>
> > >> config CC_HAS_BAR
> > >> def_bool $(cc-option -bar)
> > >>
> > >> config CC_HAS_FOO_WITH_BAR
> > >> def_bool $(cc-option -foo -bar)
> > >>
> > >>
> > >>
> > >> (Case 2)
> > >> Compiler flag -foo is sensitive to word-size.
> > >> So, we need to test this option together with -m32/-m64.
> > >> User can toggle CONFIG_64BIT, like i386/x86_64.
> > >>
> > >>
> > >> config CC_NEEDS_M64
> > >> def_bool $(cc-option -m64) && 64BIT
> > >>
> > >> config CC_NEEDS_M32
> > >> def_bool $(cc-option -m32) && !64BIT
> > >>
> > >> config CC_HAS_FOO
> > >> bool
> > >> default $(cc-option -m64 -foo) if CC_NEEDS_M64
> > >> default $(cc-option -m32 -foo) if CC_NEEDS_M32
> > >> default $(cc-option -foo)
> > >>
> > >>
> > >>
> > >> (Case 3)
> > >> Compiler flag -foo is sensitive to endian-ness.
> > >>
> > >>
> > >> config CC_NEEDS_BIG_ENDIAN
> > >> def_bool $(cc-option -mbig-endian) && CPU_BIG_ENDIAN
> > >>
> > >> config CC_NEEDS_LITTLE_ENDIAN
> > >> def_bool $(cc-option -mlittle-endian) && CPU_LITTLE_ENDIAN
> > >>
> > >> config CC_HAS_FOO
> > >> bool
> > >> default $(cc-option -mbig-endian -foo) if CC_NEEDS_BIG_ENDIAN
> > >> default $(cc-option -mlittle-endian -foo) if CC_NEEDS_LITTLE_ENDIAN
> > >> default $(cc-option -foo)
> > >>
> > >>
> > >>
> > >>
> > >> Hmm, I think I can implement those somehow.
> > >> But, I hope we do not have many instances like this...
> > >>
> > >>
> > >> If you know more naive cases, please share your knowledge.
> > >>
> > >> Thanks!
> > >>
> > >>
> > >> --
> > >> Best Regards
> > >> Masahiro Yamada
> > >
> > > Would get pretty bad if a test needs to consider multiple symbols.
> > > Exponential explosion there...
> > >
> > >
> > > I thought some more about the implementation of dynamic (post-parsing)
> > > functions to see how bad it would get with the current implementation.
> > >
> > > Some background on how things work now:
> > >
> > > 1. All expression operands in Kconfig are symbols.
> > >
> > > 2. Returning '$ENV' or '$(fn foo)' as a T_WORD during parsing gets
> > > you symbols with those strings as names and S_UNKNOWN type (because
> > > they act like references to undefined symbols).
> > >
> > > 3. For "foo-$(fn foo)", you also get a symbol with that string as its
> > > name and S_UNKNOWN type (stored among the SYMBOL_CONST symbols)
> > >
> > > 4. Symbols with S_UNKNOWN type get their name as their string value,
> > > and the tristate value n.
> > >
> > > So, if you do string expansion on the names of symbols with S_UNKNOWN
> > > type in sym_calc_value(), you're almost there with the current
> > > implementation, except for the tristate case.
> > >
> > > Maybe you could set the tristate value of S_UNKNOWN symbols depending on
> > > the string value you end up with. Things are getting pretty confusing at
> > > that point.
> > >
> > > Could have something like S_DYNAMIC as well. More Kconfig complexity...
> > >
> > > Then there's other complications:
> > >
> > > 1. SYMBOL_CONST is no longer constant.
> > >
> > > 2. Dependency loop detection needs to consider symbol references
> > > within strings.
> > >
> > > 3. Dependency loop detection relies on static knowledge of what
> > > symbols a symbol depends on. That might get messy for certain
> > > expansions, though it might be things you wouldn't do in practice.
> > >
> > > 4. Symbols still need to be properly invalidated. It looks like at
> > > least menuconfig just does a dumb invalidate-everything whenever
> > > the value of a symbol is changed though, so it might not require
> > > extra work. (Bit messier in Kconfiglib, which does minimal
> > > invalidation to keep scripts fast, but just need to extract a few
> > > extra deps there.)
> > >
> > >
> > > It looks like dynamic functions could get quite messy, but might be
> > > doable if absolutely required. There's probably more devils in the
> > > details though.
> > >
> > > I don't think the static function model precludes switching models later
> > > btw, when people have more experience.
> >
> >
> >
> > I really want to start with the static function model
> > and see if we need the dynamic function implementation.
>
> Yeah, let's start with static functions, IMO.
>
> Either we'll learn that they're powerful enough in practice, and save
> ourselves some work, or we'll gain experience for later. Converting from
> static to dynamic functions should be painless, if needed.
>
> My plan would be something like:
>
> 1. Implement static functions
>
> 2. Convert as many simple cases over to them as possible
>
> 3. See how bad the bad cases get. If they get really bad, then decide
> what to do next (extend Kconfig, handle them in the Makefiles,
> etc.)
>
> >
> > Here is an idea for the migration path in case
> > we need to do that in the future.
> >
> >
> >
> > Currently, every time user input is given,
> > sym_clear_all_valid() is called.
> >
> > It is not efficient to blindly re-evaluate expensive $(shell ...)
>
> I think menuconfig only reevalutes the symbols in the menu that's
> currently shown in the interface (along with their dependencies).
>
> Maybe that'd be bad enough though.
>
> >
> >
> > So, have a list of symbols the function depends on
> > in its arguments.
> >
> > For example,
> >
> > config CC_HAS_SANE_STACKPROTECTOR
> > def_bool $(shell $srctree/scripts/gcc-has-stack-protector.sh
> > $CC $(1), CFLAGS_BASE)
> >
> >
> > Here the first argument
> > $srctree/scripts/gcc-x86-has-stack-protector.sh $CC $(1)
> >
> > is the shell command.
> > $(1), $(2), ... will be replaced with the values of symbols (or expressions)
> > that follow when running the shell command.
> >
> >
> > The second argument
> > CFLAGS_BASE
> > is the dependency symbol (or expression).
> >
> >
> > CFLAGS_BASE can be dynamically changed like
> >
> > config CFLAGS_BASE
> > string
> > default "-m64" if 64BIT
> > default "-m32"
> >
> >
> > When and only when CFLAGS_BASE is updated, the function should be re-calculated.
> > (This will require efforts to minimize the amount of re-evaluation.)
> >
> >
> >
> >
> > cc-option will be implemented like follows:
> >
> > macro cc-option $(shell $CC -Werror $$(1) $(1) -c -x c /dev/null -o
> > /dev/null, CFLAGS_BASE)
> >
> >
> >
> > Please notice the difference between $$(1) and $(1).
> >
> > $(1) is immediately expanded by cc-option macro.
> >
> > $$(1) is escaped since we want to expand it by $(shell ...), not by
> > $(cc-option ...)
> >
> >
> >
> > For example,
> >
> > $(cc-option -fstack-protector)
> >
> > will be expanded to
> >
> > $(shell gcc -Werror $(1) -fstack-protector -c -x c /dev/null -o
> > /dev/null, CFLAGS_BASE)
> >
> > Since macros are just textual shorthand, so this expansion happens
> > during the parse phase.
> >
> >
> >
> > Then, the evaluation phase does the following every time CFLAGS_BASE is updated.
> >
> > gcc -Werror [value of CFLAGS_BASE] -fstack-protector -c -x c /dev/null
> > -o /dev/null
> >
> >
> > This is a new form of expression, so it will be managed in AST tree
> > with a flag E_SHELL (or E_FUNC) etc.
> >
> >
> > Not implemented at all. Just a rough sketch.
>
> A simpler syntax like
>
> $(shell $CC -Werror {CFLAGS_BASE} -c -x c /dev/null -o /dev/null)
*$(shell $CC -Werror {CFLAGS_BASE} -fstack-protector -c -x c /dev/null -o /dev/null)
^ permalink raw reply
* [PATCH v10 1/2] powerpc/powernv: Enable tunneled operations
From: Philippe Bergheaud @ 2018-03-02 9:56 UTC (permalink / raw)
To: linuxppc-dev; +Cc: fbarrat, clombard, benh, Philippe Bergheaud
P9 supports PCI tunneled operations (atomics and as_notify). This
patch adds support for tunneled operations on powernv, with a new
API, to be called by device drivers:
pnv_pci_enable_tunnel()
Enable tunnel operations, tell driver the 16-bit ASN indication
used by kernel.
pnv_pci_disable_tunnel()
Disable tunnel operations.
pnv_pci_set_tunnel_bar()
Tell kernel the Tunnel BAR Response address used by driver.
This function uses two new OPAL calls, as the PBCQ Tunnel BAR
register is configured by skiboot.
pnv_pci_get_as_notify_info()
Return the ASN info of the thread to be woken up.
Signed-off-by: Philippe Bergheaud <felix@linux.vnet.ibm.com>
Reviewed-by: Frederic Barrat <fbarrat@linux.vnet.ibm.com>
---
Changelog:
v2: Do not set the ASN indication. Get it from the device tree.
v3: Make pnv_pci_get_phb_node() available when compiling without cxl.
v4: Add pnv_pci_get_as_notify_info().
Rebase opal call numbers on skiboot 5.9.6.
v5: pnv_pci_get_tunnel_ind():
- fix node reference count
pnv_pci_get_as_notify_info():
- fail if task == NULL
- read pid from mm->context.id
- explain that thread.tidr require CONFIG_PPC64
v6: pnv_pci_get_tunnel_ind():
- check if radix is enabled, or else return an error
pnv_pci_get_as_notify_info():
- remove a capi-specific comment, irrelevant for pci
v7: pnv_pci_set_tunnel_bar():
- setting the tunnel bar more than once with the same value
is not an error
v8: No change
v9: Rename pnv_pci_get_tunnel_ind() into pnv_pci_enable_tunnel():
- Increase real window size to accept as_notify messages.
New api pnv_pci_disable_tunnel():
- Restore real window size to its default value.
Adjust opal call numbers.
v10: Adjust opal call numbers to their final values.
---
arch/powerpc/include/asm/opal-api.h | 4 +-
arch/powerpc/include/asm/opal.h | 2 +
arch/powerpc/include/asm/pnv-pci.h | 6 ++
arch/powerpc/platforms/powernv/opal-wrappers.S | 2 +
arch/powerpc/platforms/powernv/pci-cxl.c | 8 --
arch/powerpc/platforms/powernv/pci.c | 135 +++++++++++++++++++++++++
6 files changed, 148 insertions(+), 9 deletions(-)
diff --git a/arch/powerpc/include/asm/opal-api.h b/arch/powerpc/include/asm/opal-api.h
index 94bd1bf2c873..d886a5b7ff21 100644
--- a/arch/powerpc/include/asm/opal-api.h
+++ b/arch/powerpc/include/asm/opal-api.h
@@ -204,7 +204,9 @@
#define OPAL_NPU_SPA_SETUP 159
#define OPAL_NPU_SPA_CLEAR_CACHE 160
#define OPAL_NPU_TL_SET 161
-#define OPAL_LAST 161
+#define OPAL_PCI_GET_PBCQ_TUNNEL_BAR 164
+#define OPAL_PCI_SET_PBCQ_TUNNEL_BAR 165
+#define OPAL_LAST 165
/* Device tree flags */
diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h
index 12e70fb58700..dde60089d0d4 100644
--- a/arch/powerpc/include/asm/opal.h
+++ b/arch/powerpc/include/asm/opal.h
@@ -204,6 +204,8 @@ int64_t opal_unregister_dump_region(uint32_t id);
int64_t opal_slw_set_reg(uint64_t cpu_pir, uint64_t sprn, uint64_t val);
int64_t opal_config_cpu_idle_state(uint64_t state, uint64_t flag);
int64_t opal_pci_set_phb_cxl_mode(uint64_t phb_id, uint64_t mode, uint64_t pe_number);
+int64_t opal_pci_get_pbcq_tunnel_bar(uint64_t phb_id, uint64_t *addr);
+int64_t opal_pci_set_pbcq_tunnel_bar(uint64_t phb_id, uint64_t addr);
int64_t opal_ipmi_send(uint64_t interface, struct opal_ipmi_msg *msg,
uint64_t msg_len);
int64_t opal_ipmi_recv(uint64_t interface, struct opal_ipmi_msg *msg,
diff --git a/arch/powerpc/include/asm/pnv-pci.h b/arch/powerpc/include/asm/pnv-pci.h
index 3e5cf251ad9a..d2d8c28db336 100644
--- a/arch/powerpc/include/asm/pnv-pci.h
+++ b/arch/powerpc/include/asm/pnv-pci.h
@@ -29,6 +29,12 @@ extern int pnv_pci_set_power_state(uint64_t id, uint8_t state,
extern int pnv_pci_set_p2p(struct pci_dev *initiator, struct pci_dev *target,
u64 desc);
+extern int pnv_pci_enable_tunnel(struct pci_dev *dev, uint64_t *asnind);
+extern int pnv_pci_disable_tunnel(struct pci_dev *dev);
+extern int pnv_pci_set_tunnel_bar(struct pci_dev *dev, uint64_t addr,
+ int enable);
+extern int pnv_pci_get_as_notify_info(struct task_struct *task, u32 *lpid,
+ u32 *pid, u32 *tid);
int pnv_phb_to_cxl_mode(struct pci_dev *dev, uint64_t mode);
int pnv_cxl_ioda_msi_setup(struct pci_dev *dev, unsigned int hwirq,
unsigned int virq);
diff --git a/arch/powerpc/platforms/powernv/opal-wrappers.S b/arch/powerpc/platforms/powernv/opal-wrappers.S
index 1b2936ba6040..3da30c2f26b4 100644
--- a/arch/powerpc/platforms/powernv/opal-wrappers.S
+++ b/arch/powerpc/platforms/powernv/opal-wrappers.S
@@ -323,3 +323,5 @@ OPAL_CALL(opal_sensor_group_clear, OPAL_SENSOR_GROUP_CLEAR);
OPAL_CALL(opal_npu_spa_setup, OPAL_NPU_SPA_SETUP);
OPAL_CALL(opal_npu_spa_clear_cache, OPAL_NPU_SPA_CLEAR_CACHE);
OPAL_CALL(opal_npu_tl_set, OPAL_NPU_TL_SET);
+OPAL_CALL(opal_pci_get_pbcq_tunnel_bar, OPAL_PCI_GET_PBCQ_TUNNEL_BAR);
+OPAL_CALL(opal_pci_set_pbcq_tunnel_bar, OPAL_PCI_SET_PBCQ_TUNNEL_BAR);
diff --git a/arch/powerpc/platforms/powernv/pci-cxl.c b/arch/powerpc/platforms/powernv/pci-cxl.c
index 94498a04558b..cee003de63af 100644
--- a/arch/powerpc/platforms/powernv/pci-cxl.c
+++ b/arch/powerpc/platforms/powernv/pci-cxl.c
@@ -16,14 +16,6 @@
#include "pci.h"
-struct device_node *pnv_pci_get_phb_node(struct pci_dev *dev)
-{
- struct pci_controller *hose = pci_bus_to_host(dev->bus);
-
- return of_node_get(hose->dn);
-}
-EXPORT_SYMBOL(pnv_pci_get_phb_node);
-
int pnv_phb_to_cxl_mode(struct pci_dev *dev, uint64_t mode)
{
struct pci_controller *hose = pci_bus_to_host(dev->bus);
diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c
index 69d102cbf48f..b265ecc0836a 100644
--- a/arch/powerpc/platforms/powernv/pci.c
+++ b/arch/powerpc/platforms/powernv/pci.c
@@ -18,6 +18,7 @@
#include <linux/io.h>
#include <linux/msi.h>
#include <linux/iommu.h>
+#include <linux/sched/mm.h>
#include <asm/sections.h>
#include <asm/io.h>
@@ -38,6 +39,7 @@
#include "pci.h"
static DEFINE_MUTEX(p2p_mutex);
+static DEFINE_MUTEX(tunnel_mutex);
int pnv_pci_get_slot_id(struct device_node *np, uint64_t *id)
{
@@ -1092,6 +1094,139 @@ int pnv_pci_set_p2p(struct pci_dev *initiator, struct pci_dev *target, u64 desc)
}
EXPORT_SYMBOL_GPL(pnv_pci_set_p2p);
+struct device_node *pnv_pci_get_phb_node(struct pci_dev *dev)
+{
+ struct pci_controller *hose = pci_bus_to_host(dev->bus);
+
+ return of_node_get(hose->dn);
+}
+EXPORT_SYMBOL(pnv_pci_get_phb_node);
+
+int pnv_pci_enable_tunnel(struct pci_dev *dev, u64 *asnind)
+{
+ struct device_node *np;
+ const __be32 *prop;
+ struct pnv_ioda_pe *pe;
+ uint16_t window_id;
+ int rc;
+
+ if (!radix_enabled())
+ return -ENXIO;
+
+ if (!(np = pnv_pci_get_phb_node(dev)))
+ return -ENXIO;
+
+ prop = of_get_property(np, "ibm,phb-indications", NULL);
+ of_node_put(np);
+
+ if (!prop || !prop[1])
+ return -ENXIO;
+
+ *asnind = (u64)be32_to_cpu(prop[1]);
+ pe = pnv_ioda_get_pe(dev);
+ if (!pe)
+ return -ENODEV;
+
+ /* Increase real window size to accept as_notify messages. */
+ window_id = (pe->pe_number << 1 ) + 1;
+ rc = opal_pci_map_pe_dma_window_real(pe->phb->opal_id, pe->pe_number,
+ window_id, pe->tce_bypass_base,
+ (uint64_t)1 << 48);
+ return opal_error_code(rc);
+}
+EXPORT_SYMBOL_GPL(pnv_pci_enable_tunnel);
+
+int pnv_pci_disable_tunnel(struct pci_dev *dev)
+{
+ struct pnv_ioda_pe *pe;
+
+ pe = pnv_ioda_get_pe(dev);
+ if (!pe)
+ return -ENODEV;
+
+ /* Restore default real window size. */
+ pnv_pci_ioda2_set_bypass(pe, true);
+ return 0;
+}
+EXPORT_SYMBOL_GPL(pnv_pci_disable_tunnel);
+
+int pnv_pci_set_tunnel_bar(struct pci_dev *dev, u64 addr, int enable)
+{
+ __be64 val;
+ struct pci_controller *hose;
+ struct pnv_phb *phb;
+ u64 tunnel_bar;
+ int rc;
+
+ if (!opal_check_token(OPAL_PCI_GET_PBCQ_TUNNEL_BAR))
+ return -ENXIO;
+ if (!opal_check_token(OPAL_PCI_SET_PBCQ_TUNNEL_BAR))
+ return -ENXIO;
+
+ hose = pci_bus_to_host(dev->bus);
+ phb = hose->private_data;
+
+ mutex_lock(&tunnel_mutex);
+ rc = opal_pci_get_pbcq_tunnel_bar(phb->opal_id, &val);
+ if (rc != OPAL_SUCCESS) {
+ rc = -EIO;
+ goto out;
+ }
+ tunnel_bar = be64_to_cpu(val);
+ if (enable) {
+ /*
+ * Only one device per PHB can use atomics.
+ * Our policy is first-come, first-served.
+ */
+ if (tunnel_bar) {
+ if (tunnel_bar != addr)
+ rc = -EBUSY;
+ else
+ rc = 0; /* Setting same address twice is ok */
+ goto out;
+ }
+ } else {
+ /*
+ * The device that owns atomics and wants to release
+ * them must pass the same address with enable == 0.
+ */
+ if (tunnel_bar != addr) {
+ rc = -EPERM;
+ goto out;
+ }
+ addr = 0x0ULL;
+ }
+ rc = opal_pci_set_pbcq_tunnel_bar(phb->opal_id, addr);
+ rc = opal_error_code(rc);
+out:
+ mutex_unlock(&tunnel_mutex);
+ return rc;
+}
+EXPORT_SYMBOL_GPL(pnv_pci_set_tunnel_bar);
+
+#ifdef CONFIG_PPC64 /* for thread.tidr */
+int pnv_pci_get_as_notify_info(struct task_struct *task, u32 *lpid, u32 *pid,
+ u32 *tid)
+{
+ struct mm_struct *mm = NULL;
+
+ if (task == NULL)
+ return -EINVAL;
+
+ mm = get_task_mm(task);
+ if (mm == NULL)
+ return -EINVAL;
+
+ *pid = mm->context.id;
+ mmput(mm);
+
+ *tid = task->thread.tidr;
+ *lpid = mfspr(SPRN_LPID);
+ return 0;
+}
+EXPORT_SYMBOL_GPL(pnv_pci_get_as_notify_info);
+#endif
+
void pnv_pci_shutdown(void)
{
struct pci_controller *hose;
--
2.16.1
^ permalink raw reply related
* [PATCH v10 2/2] cxl: read PHB indications from the device tree
From: Philippe Bergheaud @ 2018-03-02 9:56 UTC (permalink / raw)
To: linuxppc-dev; +Cc: fbarrat, clombard, benh, Philippe Bergheaud
In-Reply-To: <20180302095612.19493-1-felix@linux.vnet.ibm.com>
Configure the P9 XSL_DSNCTL register with PHB indications found
in the device tree, or else use legacy hard-coded values.
Signed-off-by: Philippe Bergheaud <felix@linux.vnet.ibm.com>
Reviewed-by: Frederic Barrat <fbarrat@linux.vnet.ibm.com>
---
Changelog:
v2: New patch. Use the new device tree property "ibm,phb-indications".
v3: No change.
v4: No functional change.
Drop cosmetic fix in comment.
v5: get_phb_indications():
- make static variables local to function.
- return static variable values by arguments.
v6: get_phb_indications():
- acquire a mutex before setting the phb indications.
v7: get_phb_indications():
cxl_get_xsl9_dsnctl():
- return -ENODEV instead of -1.
v8: get_phb_indications():
- stay on the safe side: acquire the mutex unconditionally
v9,v10: No change.
---
drivers/misc/cxl/cxl.h | 2 +-
drivers/misc/cxl/cxllib.c | 2 +-
drivers/misc/cxl/pci.c | 48 ++++++++++++++++++++++++++++++++++++++++++-----
3 files changed, 45 insertions(+), 7 deletions(-)
diff --git a/drivers/misc/cxl/cxl.h b/drivers/misc/cxl/cxl.h
index 4f015da78f28..a7689944b351 100644
--- a/drivers/misc/cxl/cxl.h
+++ b/drivers/misc/cxl/cxl.h
@@ -1065,7 +1065,7 @@ int cxl_psl_purge(struct cxl_afu *afu);
int cxl_calc_capp_routing(struct pci_dev *dev, u64 *chipid,
u32 *phb_index, u64 *capp_unit_id);
int cxl_slot_is_switched(struct pci_dev *dev);
-int cxl_get_xsl9_dsnctl(u64 capp_unit_id, u64 *reg);
+int cxl_get_xsl9_dsnctl(struct pci_dev *dev, u64 capp_unit_id, u64 *reg);
u64 cxl_calculate_sr(bool master, bool kernel, bool real_mode, bool p9);
void cxl_native_irq_dump_regs_psl9(struct cxl_context *ctx);
diff --git a/drivers/misc/cxl/cxllib.c b/drivers/misc/cxl/cxllib.c
index 30ccba436b3b..bea1eb004b49 100644
--- a/drivers/misc/cxl/cxllib.c
+++ b/drivers/misc/cxl/cxllib.c
@@ -99,7 +99,7 @@ int cxllib_get_xsl_config(struct pci_dev *dev, struct cxllib_xsl_config *cfg)
if (rc)
return rc;
- rc = cxl_get_xsl9_dsnctl(capp_unit_id, &cfg->dsnctl);
+ rc = cxl_get_xsl9_dsnctl(dev, capp_unit_id, &cfg->dsnctl);
if (rc)
return rc;
if (cpu_has_feature(CPU_FTR_POWER9_DD1)) {
diff --git a/drivers/misc/cxl/pci.c b/drivers/misc/cxl/pci.c
index 758842f65a1b..8d179e64a296 100644
--- a/drivers/misc/cxl/pci.c
+++ b/drivers/misc/cxl/pci.c
@@ -407,21 +407,59 @@ int cxl_calc_capp_routing(struct pci_dev *dev, u64 *chipid,
return 0;
}
-int cxl_get_xsl9_dsnctl(u64 capp_unit_id, u64 *reg)
+static DEFINE_MUTEX(indications_mutex);
+
+static int get_phb_indications(struct pci_dev *dev, u64 *capiind, u64 *asnind,
+ u64 *nbwind)
+{
+ static u64 nbw, asn, capi = 0;
+ struct device_node *np;
+ const __be32 *prop;
+
+ mutex_lock(&indications_mutex);
+ if (!capi) {
+ if (!(np = pnv_pci_get_phb_node(dev))) {
+ mutex_unlock(&indications_mutex);
+ return -ENODEV;
+ }
+
+ prop = of_get_property(np, "ibm,phb-indications", NULL);
+ if (!prop) {
+ nbw = 0x0300UL; /* legacy values */
+ asn = 0x0400UL;
+ capi = 0x0200UL;
+ } else {
+ nbw = (u64)be32_to_cpu(prop[2]);
+ asn = (u64)be32_to_cpu(prop[1]);
+ capi = (u64)be32_to_cpu(prop[0]);
+ }
+ of_node_put(np);
+ }
+ *capiind = capi;
+ *asnind = asn;
+ *nbwind = nbw;
+ mutex_unlock(&indications_mutex);
+ return 0;
+}
+
+int cxl_get_xsl9_dsnctl(struct pci_dev *dev, u64 capp_unit_id, u64 *reg)
{
u64 xsl_dsnctl;
+ u64 capiind, asnind, nbwind;
/*
* CAPI Identifier bits [0:7]
* bit 61:60 MSI bits --> 0
* bit 59 TVT selector --> 0
*/
+ if (get_phb_indications(dev, &capiind, &asnind, &nbwind))
+ return -ENODEV;
/*
* Tell XSL where to route data to.
* The field chipid should match the PHB CAPI_CMPM register
*/
- xsl_dsnctl = ((u64)0x2 << (63-7)); /* Bit 57 */
+ xsl_dsnctl = (capiind << (63-15)); /* Bit 57 */
xsl_dsnctl |= (capp_unit_id << (63-15));
/* nMMU_ID Defaults to: b’000001001’*/
@@ -435,14 +473,14 @@ int cxl_get_xsl9_dsnctl(u64 capp_unit_id, u64 *reg)
* nbwind=0x03, bits [57:58], must include capi indicator.
* Not supported on P9 DD1.
*/
- xsl_dsnctl |= ((u64)0x03 << (63-47));
+ xsl_dsnctl |= (nbwind << (63-55));
/*
* Upper 16b address bits of ASB_Notify messages sent to the
* system. Need to match the PHB’s ASN Compare/Mask Register.
* Not supported on P9 DD1.
*/
- xsl_dsnctl |= ((u64)0x04 << (63-55));
+ xsl_dsnctl |= asnind;
}
*reg = xsl_dsnctl;
@@ -462,7 +500,7 @@ static int init_implementation_adapter_regs_psl9(struct cxl *adapter,
if (rc)
return rc;
- rc = cxl_get_xsl9_dsnctl(capp_unit_id, &xsl_dsnctl);
+ rc = cxl_get_xsl9_dsnctl(dev, capp_unit_id, &xsl_dsnctl);
if (rc)
return rc;
--
2.16.1
^ permalink raw reply related
* [PATCH] KVM: PPC: Book3S HV: Fix guest time accounting with VIRT_CPU_ACCOUNTING_GEN
From: Laurent Vivier @ 2018-03-02 10:51 UTC (permalink / raw)
To: linux-kernel
Cc: linuxppc-dev, kvm-ppc, Paul Mackerras, David Gibson,
Paolo Bonzini, Laurent Vivier
Since commit 8b24e69fc47e ("KVM: PPC: Book3S HV: Close race with testing
for signals on guest entry"), if CONFIG_VIRT_CPU_ACCOUNTING_GEN is set, the
guest time is not accounted to guest time and user time, but instead to
system time.
This is because guest_enter()/guest_exit() are called while interrupts
are disabled and the tick counter cannot be updated between them.
To fix that, move guest_exit() after local_irq_enable(), and as
guest_enter() is called with IRQ disabled, calls guest_enter_irqoff()
instead.
Fixes: 8b24e69fc47e
("KVM: PPC: Book3S HV: Close race with testing for signals on guest entry")
Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
arch/powerpc/kvm/book3s_hv.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index 89707354c2ef..8274c2807202 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -2885,7 +2885,7 @@ static noinline void kvmppc_run_core(struct kvmppc_vcore *vc)
*/
trace_hardirqs_on();
- guest_enter();
+ guest_enter_irqoff();
srcu_idx = srcu_read_lock(&vc->kvm->srcu);
@@ -2893,8 +2893,6 @@ static noinline void kvmppc_run_core(struct kvmppc_vcore *vc)
srcu_read_unlock(&vc->kvm->srcu, srcu_idx);
- guest_exit();
-
trace_hardirqs_off();
set_irq_happened(trap);
@@ -2937,6 +2935,7 @@ static noinline void kvmppc_run_core(struct kvmppc_vcore *vc)
kvmppc_set_host_core(pcpu);
local_irq_enable();
+ guest_exit();
/* Let secondaries go back to the offline loop */
for (i = 0; i < controlled_threads; ++i) {
--
2.14.3
^ permalink raw reply related
* Re: [PATCH] KVM: PPC: Book3S HV: Fix guest time accounting with VIRT_CPU_ACCOUNTING_GEN
From: Paolo Bonzini @ 2018-03-02 11:52 UTC (permalink / raw)
To: Laurent Vivier, linux-kernel
Cc: linuxppc-dev, kvm-ppc, Paul Mackerras, David Gibson
In-Reply-To: <20180302105156.19506-1-lvivier@redhat.com>
On 02/03/2018 11:51, Laurent Vivier wrote:
> Since commit 8b24e69fc47e ("KVM: PPC: Book3S HV: Close race with testing
> for signals on guest entry"), if CONFIG_VIRT_CPU_ACCOUNTING_GEN is set, the
> guest time is not accounted to guest time and user time, but instead to
> system time.
>
> This is because guest_enter()/guest_exit() are called while interrupts
> are disabled and the tick counter cannot be updated between them.
>
> To fix that, move guest_exit() after local_irq_enable(), and as
> guest_enter() is called with IRQ disabled, calls guest_enter_irqoff()
> instead.
>
> Fixes: 8b24e69fc47e
> ("KVM: PPC: Book3S HV: Close race with testing for signals on guest entry")
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> arch/powerpc/kvm/book3s_hv.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index 89707354c2ef..8274c2807202 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -2885,7 +2885,7 @@ static noinline void kvmppc_run_core(struct kvmppc_vcore *vc)
> */
> trace_hardirqs_on();
>
> - guest_enter();
> + guest_enter_irqoff();
>
> srcu_idx = srcu_read_lock(&vc->kvm->srcu);
>
> @@ -2893,8 +2893,6 @@ static noinline void kvmppc_run_core(struct kvmppc_vcore *vc)
>
> srcu_read_unlock(&vc->kvm->srcu, srcu_idx);
>
> - guest_exit();
> -
> trace_hardirqs_off();
> set_irq_happened(trap);
>
> @@ -2937,6 +2935,7 @@ static noinline void kvmppc_run_core(struct kvmppc_vcore *vc)
> kvmppc_set_host_core(pcpu);
>
> local_irq_enable();
> + guest_exit();
>
> /* Let secondaries go back to the offline loop */
> for (i = 0; i < controlled_threads; ++i) {
>
^ permalink raw reply
* [RFC PATCH 7/10] powerpc/powernv: Use the security flags in pnv_setup_rfi_flush()
From: Michael Ellerman @ 2018-03-02 11:58 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <20180228145316.11347-1-mpe@ellerman.id.au>
Now that we have the security flags we can significantly simplify the
code in pnv_setup_rfi_flush(), because we can use the flags instead of
checking device tree properties and because the security flags have
pessimistic defaults.
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
arch/powerpc/platforms/powernv/setup.c | 39 ++++++++--------------------------
1 file changed, 9 insertions(+), 30 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/setup.c b/arch/powerpc/platforms/powernv/setup.c
index 5f242b1bab01..8f3e7a84bbf5 100644
--- a/arch/powerpc/platforms/powernv/setup.c
+++ b/arch/powerpc/platforms/powernv/setup.c
@@ -99,11 +99,10 @@ static void pnv_setup_rfi_flush(void)
{
struct device_node *np, *fw_features;
enum l1d_flush_type type;
- int enable;
+ bool enable;
/* Default to fallback in case fw-features are not available */
type = L1D_FLUSH_FALLBACK;
- enable = 1;
np = of_find_node_by_name(NULL, "ibm,opal");
fw_features = of_get_child_by_name(np, "fw-features");
@@ -111,40 +110,20 @@ static void pnv_setup_rfi_flush(void)
if (fw_features) {
init_fw_feat_flags(fw_features);
+ of_node_put(fw_features);
- np = of_get_child_by_name(fw_features, "inst-l1d-flush-trig2");
- if (np && of_property_read_bool(np, "enabled"))
+ if (security_ftr_enabled(SEC_FTR_L1D_FLUSH_TRIG2))
type = L1D_FLUSH_MTTRIG;
- of_node_put(np);
-
- np = of_get_child_by_name(fw_features, "inst-l1d-flush-ori30,30,0");
- if (np && of_property_read_bool(np, "enabled"))
+ if (security_ftr_enabled(SEC_FTR_L1D_FLUSH_ORI30))
type = L1D_FLUSH_ORI;
-
- of_node_put(np);
-
- /* Enable unless firmware says NOT to */
- enable = 2;
- np = of_get_child_by_name(fw_features, "needs-l1d-flush-msr-hv-1-to-0");
- if (np && of_property_read_bool(np, "disabled"))
- enable--;
-
- of_node_put(np);
-
- np = of_get_child_by_name(fw_features, "needs-l1d-flush-msr-pr-0-to-1");
- if (np && of_property_read_bool(np, "disabled"))
- enable--;
-
- np = of_get_child_by_name(fw_features, "speculation-policy-favor-security");
- if (np && of_property_read_bool(np, "disabled"))
- enable = 0;
-
- of_node_put(np);
- of_node_put(fw_features);
}
- setup_rfi_flush(type, enable > 0);
+ enable = security_ftr_enabled(SEC_FTR_FAVOUR_SECURITY) && \
+ (security_ftr_enabled(SEC_FTR_L1D_FLUSH_PR) || \
+ security_ftr_enabled(SEC_FTR_L1D_FLUSH_HV));
+
+ setup_rfi_flush(type, enable);
}
static void __init pnv_setup_arch(void)
--
2.14.1
^ permalink raw reply related
* [RFC PATCH 8/10] powerpc/pseries: Use the security flags in pseries_setup_rfi_flush()
From: Michael Ellerman @ 2018-03-02 11:58 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <20180302115846.12240-1-mpe@ellerman.id.au>
Now that we have the security flags we can simplify the code in
pseries_setup_rfi_flush() because the security flags have pessimistic
defaults.
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
arch/powerpc/platforms/pseries/setup.c | 39 ++++++++++++++--------------------
1 file changed, 16 insertions(+), 23 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c
index 8ae04b586abe..2af14af6c410 100644
--- a/arch/powerpc/platforms/pseries/setup.c
+++ b/arch/powerpc/platforms/pseries/setup.c
@@ -501,38 +501,31 @@ static void pseries_setup_rfi_flush(void)
bool enable;
long rc;
- /* Enable by default */
- enable = true;
-
rc = plpar_get_cpu_characteristics(&result);
- if (rc == H_SUCCESS) {
+ if (rc == H_SUCCESS)
init_cpu_char_feature_flags(&result);
- types = L1D_FLUSH_NONE;
-
- if (result.character & H_CPU_CHAR_L1D_FLUSH_TRIG2)
- types |= L1D_FLUSH_MTTRIG;
- if (result.character & H_CPU_CHAR_L1D_FLUSH_ORI30)
- types |= L1D_FLUSH_ORI;
-
- /* Use fallback if nothing set in hcall */
- if (types == L1D_FLUSH_NONE)
- types = L1D_FLUSH_FALLBACK;
-
- if ((!(result.behaviour & H_CPU_BEHAV_L1D_FLUSH_PR)) ||
- (!(result.behaviour & H_CPU_BEHAV_FAVOUR_SECURITY)))
- enable = false;
- } else {
- /* Default to fallback if case hcall is not available */
- types = L1D_FLUSH_FALLBACK;
- }
-
/*
* We're the guest so this doesn't apply to us, clear it to simplify
* handling of it elsewhere.
*/
security_ftr_clear(SEC_FTR_L1D_FLUSH_HV);
+ types = L1D_FLUSH_NONE;
+
+ if (security_ftr_enabled(SEC_FTR_L1D_FLUSH_TRIG2))
+ types |= L1D_FLUSH_MTTRIG;
+
+ if (security_ftr_enabled(SEC_FTR_L1D_FLUSH_ORI30))
+ types |= L1D_FLUSH_ORI;
+
+ /* Use fallback if nothing set in hcall */
+ if (types == L1D_FLUSH_NONE)
+ types = L1D_FLUSH_FALLBACK;
+
+ enable = security_ftr_enabled(SEC_FTR_FAVOUR_SECURITY) && \
+ security_ftr_enabled(SEC_FTR_L1D_FLUSH_PR);
+
setup_rfi_flush(types, enable);
}
--
2.14.1
^ permalink raw reply related
* [RFC PATCH 9/10] powerpc/64s: Wire up cpu_show_spectre_v1()
From: Michael Ellerman @ 2018-03-02 11:58 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <20180302115846.12240-1-mpe@ellerman.id.au>
Add a definition for cpu_show_spectre_v1() to override the generic
version. Currently this just prints "Not affected" or "Vulnerable"
based on the firmware flag.
Although the kernel does have array_index_nospec() in a few places, we
haven't yet audited all the powerpc code to see where it's necessary,
so for now we don't list that as a mitigation.
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
arch/powerpc/kernel/security.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/arch/powerpc/kernel/security.c b/arch/powerpc/kernel/security.c
index 865db6f8bcca..0eace3cac818 100644
--- a/arch/powerpc/kernel/security.c
+++ b/arch/powerpc/kernel/security.c
@@ -50,3 +50,11 @@ ssize_t cpu_show_meltdown(struct device *dev, struct device_attribute *attr, cha
return sprintf(buf, "Vulnerable\n");
}
+
+ssize_t cpu_show_spectre_v1(struct device *dev, struct device_attribute *attr, char *buf)
+{
+ if (!security_ftr_enabled(SEC_FTR_BNDS_CHK_SPEC_BAR))
+ return sprintf(buf, "Not affected\n");
+
+ return sprintf(buf, "Vulnerable\n");
+}
--
2.14.1
^ permalink raw reply related
* [RFC PATCH 10/10] powerpc/64s: Wire up cpu_show_spectre_v2()
From: Michael Ellerman @ 2018-03-02 11:58 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <20180302115846.12240-1-mpe@ellerman.id.au>
Add a definition for cpu_show_spectre_v2() to override the generic
version. This has several permuations, though in practice some may not
occur we cater for any combination.
The most verbose is:
Mitigation: Indirect branch serialisation (kernel only), Indirect
branch cache disabled, ori31 speculation barrier enabled
We don't treat the ori31 speculation barrier as a mitigation on its
own, because it has to be *used* by code in order to be a mitigation
and we don't know if userspace is doing that. So if that's all we see
we say:
Vulnerable, ori31 speculation barrier enabled
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
arch/powerpc/kernel/security.c | 33 +++++++++++++++++++++++++++++++++
1 file changed, 33 insertions(+)
diff --git a/arch/powerpc/kernel/security.c b/arch/powerpc/kernel/security.c
index 0eace3cac818..2cee3dcd231b 100644
--- a/arch/powerpc/kernel/security.c
+++ b/arch/powerpc/kernel/security.c
@@ -58,3 +58,36 @@ ssize_t cpu_show_spectre_v1(struct device *dev, struct device_attribute *attr, c
return sprintf(buf, "Vulnerable\n");
}
+
+ssize_t cpu_show_spectre_v2(struct device *dev, struct device_attribute *attr, char *buf)
+{
+ bool bcs, ccd, ori;
+ struct seq_buf s;
+
+ seq_buf_init(&s, buf, PAGE_SIZE - 1);
+
+ bcs = security_ftr_enabled(SEC_FTR_BCCTRL_SERIALISED);
+ ccd = security_ftr_enabled(SEC_FTR_COUNT_CACHE_DISABLED);
+ ori = security_ftr_enabled(SEC_FTR_SPEC_BAR_ORI31);
+
+ if (bcs || ccd) {
+ seq_buf_printf(&s, "Mitigation: ");
+
+ if (bcs)
+ seq_buf_printf(&s, "Indirect branch serialisation (kernel only)");
+
+ if (bcs && ccd)
+ seq_buf_printf(&s, ", ");
+
+ if (ccd)
+ seq_buf_printf(&s, "Indirect branch cache disabled");
+ } else
+ seq_buf_printf(&s, "Vulnerable");
+
+ if (ori)
+ seq_buf_printf(&s, ", ori31 speculation barrier enabled");
+
+ seq_buf_printf(&s, "\n");
+
+ return s.len;
+}
--
2.14.1
^ permalink raw reply related
* Re: [PATCH] powerpc: dts: replace 'linux, stdout-path' with 'stdout-path'
From: Michael Ellerman @ 2018-03-02 12:16 UTC (permalink / raw)
To: Rob Herring
Cc: devicetree, linux-kernel@vger.kernel.org, Mark Rutland,
Benjamin Herrenschmidt, Paul Mackerras, linuxppc-dev
In-Reply-To: <CAL_JsqKn3hXaaYYef47afK+E=WUfZyfF6ejGD0vX7C8sNjhdFw@mail.gmail.com>
Rob Herring <robh@kernel.org> writes:
> On Wed, Feb 28, 2018 at 7:33 PM, Michael Ellerman <mpe@ellerman.id.au> wrote:
>> Rob Herring <robh@kernel.org> writes:
>>
>>> 'linux,stdout-path' has been deprecated for some time in favor of
>>> 'stdout-path'. Now dtc will warn on occurrences of 'linux,stdout-path'.
>>> Search and replace all the of occurrences with 'stdout-path'.
>>
>> This patch looks OK.
>>
>> But please remember that not all device trees are generated with dtc, we
>> still have machines in the wild that have firmware which use
>> "linux,stdout-path" and may never be updated.
>
> Absolutely. The core code still supports both.
Phew ;)
It has prompted us to look at various firmware pieces and some we can
update to start using the new property or both. It's still going to be
many years before we can actually remove support for linux,stdout-path
though.
> The only scenario I could come up with is someone has an old
> bootloader that only understands linux,stdout-path and modifies it and
> they update the dtb. We may end up with both properties with
> stdout-path taking preference.
Yeah I think this is fairly safe, and in the unlikely case it does cause
a problem we can always back the change out for an individual file.
cheers
^ permalink raw reply
* Re: [PATCH] powerpc: boot: add strrchr function
From: Michael Ellerman @ 2018-03-02 12:17 UTC (permalink / raw)
To: Rob Herring
Cc: linux-kernel, linuxppc-dev, Benjamin Herrenschmidt,
Paul Mackerras
In-Reply-To: <20180301152654.29275-1-robh@kernel.org>
Rob Herring <robh@kernel.org> writes:
> libfdt gained a new dependency on strrchr, so copy the implementation
> from lib/string.c. Most of the string functions are in assembly, but
> stdio.c already has strnlen, so add strrchr there.
>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> Please ack. This is a dependency for dtc/libfdt sync with upstream.
Yeah seems fine. At some point we should try and duplicate less code in
boot, but now is not that time.
Acked-by: Michael Ellerman <mpe@ellerman.id.au>
cheers
> diff --git a/arch/powerpc/boot/stdio.c b/arch/powerpc/boot/stdio.c
> index a701261b1781..98042eff7b26 100644
> --- a/arch/powerpc/boot/stdio.c
> +++ b/arch/powerpc/boot/stdio.c
> @@ -21,6 +21,16 @@ size_t strnlen(const char * s, size_t count)
> return sc - s;
> }
>
> +char *strrchr(const char *s, int c)
> +{
> + const char *last = NULL;
> + do {
> + if (*s == (char)c)
> + last = s;
> + } while (*s++);
> + return (char *)last;
> +}
> +
> #ifdef __powerpc64__
>
> # define do_div(n, base) ({ \
> --
> 2.14.1
^ permalink raw reply
* Re: [PATCH 17/18] crypto: talitos - chain in buffered data for ahash on SEC1
From: Horia Geantă @ 2018-03-02 17:27 UTC (permalink / raw)
To: Christophe Leroy, Herbert Xu, David S. Miller
Cc: linux-crypto@vger.kernel.org, linux-kernel@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <28d351a973d31d140239c7e7e64999d95a011e8c.1507284818.git.christophe.leroy@c-s.fr>
On 10/6/2017 4:05 PM, Christophe Leroy wrote:=0A=
[...]=0A=
> @@ -1778,6 +1814,36 @@ static int common_nonsnoop_hash(struct talitos_ede=
sc *edesc,=0A=
> if (is_sec1 && from_talitos_ptr_len(&desc->ptr[3], true) =3D=3D 0)=0A=
> talitos_handle_buggy_hash(ctx, edesc, &desc->ptr[3]);=0A=
> =0A=
> + if (is_sec1 && req_ctx->nbuf && length) {=0A=
> + struct talitos_desc *desc2 =3D desc + 1;=0A=
> + dma_addr_t next_desc;=0A=
[...]=0A=
> + next_desc =3D dma_map_single(dev, &desc2->hdr1, TALITOS_DESC_SIZE,=0A=
> + DMA_BIDIRECTIONAL);=0A=
> + desc->next_desc =3D cpu_to_be32(next_desc);=0A=
Where is desc->next_desc initialized for the !is_sec1 case?=0A=
Memory allocation is done using kmalloc(), and since desc->next_desc is che=
cked=0A=
in some cases also for SEC 2.x+, it should be initialized to 0.=0A=
=0A=
Thanks,=0A=
Horia=0A=
=0A=
^ permalink raw reply
* Re: [PATCH 17/18] crypto: talitos - chain in buffered data for ahash on SEC1
From: Christophe LEROY @ 2018-03-02 17:42 UTC (permalink / raw)
To: Horia Geantă, Herbert Xu, David S. Miller
Cc: linux-crypto@vger.kernel.org, linux-kernel@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <VI1PR0402MB3342BDF9D4E389B4E296A53298C50@VI1PR0402MB3342.eurprd04.prod.outlook.com>
Le 02/03/2018 à 18:27, Horia Geantă a écrit :
> On 10/6/2017 4:05 PM, Christophe Leroy wrote:
> [...]
>> @@ -1778,6 +1814,36 @@ static int common_nonsnoop_hash(struct talitos_edesc *edesc,
>> if (is_sec1 && from_talitos_ptr_len(&desc->ptr[3], true) == 0)
>> talitos_handle_buggy_hash(ctx, edesc, &desc->ptr[3]);
>>
>> + if (is_sec1 && req_ctx->nbuf && length) {
>> + struct talitos_desc *desc2 = desc + 1;
>> + dma_addr_t next_desc;
> [...]
>> + next_desc = dma_map_single(dev, &desc2->hdr1, TALITOS_DESC_SIZE,
>> + DMA_BIDIRECTIONAL);
>> + desc->next_desc = cpu_to_be32(next_desc);
> Where is desc->next_desc initialized for the !is_sec1 case?
> Memory allocation is done using kmalloc(), and since desc->next_desc is checked
> in some cases also for SEC 2.x+, it should be initialized to 0.
See
https://elixir.bootlin.com/linux/v4.16-rc3/source/drivers/crypto/talitos.c#L1411
edesc = kmalloc(alloc_len, GFP_DMA | flags);
if (!edesc) {
dev_err(dev, "could not allocate edescriptor\n");
err = ERR_PTR(-ENOMEM);
goto error_sg;
}
memset(&edesc->desc, 0, sizeof(edesc->desc));
Christophe
^ permalink raw reply
* Re: [PATCH 1/2] crypto: talitos - don't persistently map req_ctx->hw_context and req_ctx->buf
From: Horia Geantă @ 2018-03-02 18:19 UTC (permalink / raw)
To: Christophe Leroy, Herbert Xu, David S. Miller
Cc: linux-crypto@vger.kernel.org, linux-kernel@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <7c934f0788d60bd82fb1cb51d712c9ab1ea7fcb4.1519660621.git.christophe.leroy@c-s.fr>
On 2/26/2018 6:40 PM, Christophe Leroy wrote:=0A=
> Commit 49f9783b0cea ("crypto: talitos - do hw_context DMA mapping=0A=
> outside the requests") introduced a persistent dma mapping of=0A=
> req_ctx->hw_context=0A=
> Commit 37b5e8897eb5 ("crypto: talitos - chain in buffered data for ahash=
=0A=
> on SEC1") introduced a persistent dma mapping of req_ctx->buf=0A=
> =0A=
> As there is no destructor for req_ctx (the request context), the=0A=
> associated dma handlers where set in ctx (the tfm context). This is=0A=
> wrong as several hash operations can run with the same ctx.=0A=
> =0A=
> This patch removes this persistent mapping.=0A=
> =0A=
> Reported-by: Horia Geanta <horia.geanta@nxp.com>=0A=
> Fixes: 49f9783b0cea ("crypto: talitos - do hw_context DMA mapping outside=
the requests")=0A=
> Fixes: 37b5e8897eb5 ("crypto: talitos - chain in buffered data for ahash =
on SEC1")=0A=
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>=0A=
Tested-by: Horia Geant=E3 <horia.geanta@nxp.com>=0A=
=0A=
Please add this to 4.15.y -stable tree.=0A=
=0A=
Thanks,=0A=
Horia=0A=
^ permalink raw reply
* [PATCH v2 01/21] powerpc: Remove warning on array size when empty
From: Mathieu Malaterre @ 2018-03-02 19:49 UTC (permalink / raw)
To: Michael Ellerman
Cc: Andy Shevchenko, Christophe LEROY, Mathieu Malaterre,
Benjamin Herrenschmidt, Paul Mackerras, linuxppc-dev,
linux-kernel
In-Reply-To: <20180225172236.29650-2-malat@debian.org>
When neither CONFIG_ALTIVEC, nor CONFIG_VSX or CONFIG_PPC64 is defined, the
array feature_properties is defined as an empty array, which in turn
triggers the following warning (treated as error on W=1):
CC arch/powerpc/kernel/prom.o
arch/powerpc/kernel/prom.c: In function ‘check_cpu_feature_properties’:
arch/powerpc/kernel/prom.c:298:16: error: comparison of unsigned expression < 0 is always false [-Werror=type-limits]
for (i = 0; i < ARRAY_SIZE(feature_properties); ++i, ++fp) {
^
cc1: all warnings being treated as errors
Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Mathieu Malaterre <malat@debian.org>
---
arch/powerpc/kernel/prom.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
index 4dffef947b8a..330c65f04820 100644
--- a/arch/powerpc/kernel/prom.c
+++ b/arch/powerpc/kernel/prom.c
@@ -291,11 +291,11 @@ static inline void identical_pvr_fixup(unsigned long node)
static void __init check_cpu_feature_properties(unsigned long node)
{
- unsigned long i;
+ int i;
struct feature_property *fp = feature_properties;
const __be32 *prop;
- for (i = 0; i < ARRAY_SIZE(feature_properties); ++i, ++fp) {
+ for (i = 0; i < (int)ARRAY_SIZE(feature_properties); ++i, ++fp) {
prop = of_get_flat_dt_prop(node, fp->name, NULL);
if (prop && be32_to_cpup(prop) >= fp->min_value) {
cur_cpu_spec->cpu_features |= fp->cpu_feature;
--
2.11.0
^ permalink raw reply related
* [PATCH v2 06/21] powerpc: Avoid comparison of unsigned long >= 0 in __access_ok
From: Mathieu Malaterre @ 2018-03-02 19:50 UTC (permalink / raw)
To: Michael Ellerman
Cc: Christophe LEROY, Mathieu Malaterre, Benjamin Herrenschmidt,
Paul Mackerras, linuxppc-dev, linux-kernel
In-Reply-To: <20180225172236.29650-7-malat@debian.org>
Rewrite function-like macro into regular static inline function to avoid a
warning during macro expansion.
Fix warning (treated as error in W=1):
CC arch/powerpc/kernel/signal_32.o
In file included from ./include/linux/uaccess.h:14:0,
from ./include/asm-generic/termios-base.h:8,
from ./arch/powerpc/include/asm/termios.h:20,
from ./include/uapi/linux/termios.h:6,
from ./include/linux/tty.h:7,
from arch/powerpc/kernel/signal_32.c:36:
./include/asm-generic/termios-base.h: In function ‘user_termio_to_kernel_termios’:
./arch/powerpc/include/asm/uaccess.h:52:35: error: comparison of unsigned expression >= 0 is always true [-Werror=type-limits]
(((size) == 0) || (((size) - 1) <= ((segment).seg - (addr)))))
^
./arch/powerpc/include/asm/uaccess.h:58:3: note: in expansion of macro ‘__access_ok’
__access_ok((__force unsigned long)(addr), (size), get_fs()))
^~~~~~~~~~~
./arch/powerpc/include/asm/uaccess.h:262:6: note: in expansion of macro ‘access_ok’
if (access_ok(VERIFY_READ, __gu_addr, (size))) \
^~~~~~~~~
./arch/powerpc/include/asm/uaccess.h:80:2: note: in expansion of macro ‘__get_user_check’
__get_user_check((x), (ptr), sizeof(*(ptr)))
^~~~~~~~~~~~~~~~
./include/asm-generic/termios-base.h:36:6: note: in expansion of macro ‘get_user’
if (get_user(termios->c_line, &termio->c_line) < 0)
^~~~~~~~
[...]
cc1: all warnings being treated as errors
Suggested-by: Segher Boessenkool <segher@kernel.crashing.org>
Signed-off-by: Mathieu Malaterre <malat@debian.org>
---
arch/powerpc/include/asm/uaccess.h | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index 51bfeb8777f0..a62ee663b2c8 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -47,9 +47,13 @@
#else
-#define __access_ok(addr, size, segment) \
- (((addr) <= (segment).seg) && \
- (((size) == 0) || (((size) - 1) <= ((segment).seg - (addr)))))
+static inline int __access_ok(unsigned long addr, unsigned long size,
+ mm_segment_t seg)
+{
+ if (addr > seg.seg)
+ return 0;
+ return (size == 0 || size - 1 <= seg.seg - addr);
+}
#endif
--
2.11.0
^ permalink raw reply related
* Re: [PATCH 05/21] powerpc: Avoid comparison of unsigned long >= 0 in pfn_valid
From: Mathieu Malaterre @ 2018-03-02 19:54 UTC (permalink / raw)
To: Segher Boessenkool
Cc: Christophe LEROY, Michael Ellerman, linuxppc-dev, Paul Mackerras,
LKML, Jiri Slaby
In-Reply-To: <20180226084602.GR21977@gate.crashing.org>
On Mon, Feb 26, 2018 at 9:46 AM, Segher Boessenkool
<segher@kernel.crashing.org> wrote:
> On Mon, Feb 26, 2018 at 07:32:03AM +0100, Christophe LEROY wrote:
>> Le 25/02/2018 =C3=A0 18:22, Mathieu Malaterre a =C3=A9crit :
>> >-#define pfn_valid(pfn) ((pfn) >=3D ARCH_PFN_OFFSET && (pf=
n) <
>> >max_mapnr)
>> >+#define pfn_valid(pfn) \
>> >+ (((pfn) - ARCH_PFN_OFFSET) < (max_mapnr - ARCH_PFN_OFFSET)=
)
>>
>> What will happen when ARCH_PFN_OFFSET is not nul and pfn is lower than
>> ARCH_PFN_OFFSET ?
>
> It will work fine.
>
> Say you are asking for a <=3D x < b so (in actual integers, no overflow=
)
> that is 0 <=3D x-a < b-a and you also assume x-a overflows, so that we
> are actually comparing x-a+M < b-a with M =3D 2**32 or such (the maximu=
m
> value in the unsigned integer type plus one). This comparison is
> obviously always false.
>
> (It also works if b < a btw).
>
>
Thanks Segher !
Christophe does that clarify things or do you want me to update the
commit message ?
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox