* Re: [PATCH 1/4] powerpc/64s: implement probe_kernel_read/write without touching AMR
From: Nicholas Piggin @ 2020-04-03 8:59 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev
In-Reply-To: <d619b378-3a88-f941-b2d5-42b79574e2ab@c-s.fr>
Christophe Leroy's on March 27, 2020 5:13 pm:
>
>
> Le 27/03/2020 à 08:02, Nicholas Piggin a écrit :
>> There is no need to allow user accesses when probing kernel addresses.
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>> arch/powerpc/include/asm/uaccess.h | 25 ++++++++++-----
>> arch/powerpc/lib/Makefile | 2 +-
>> arch/powerpc/lib/uaccess.c | 50 ++++++++++++++++++++++++++++++
>> 3 files changed, 68 insertions(+), 9 deletions(-)
>> create mode 100644 arch/powerpc/lib/uaccess.c
>>
>
> [...]
>
>> diff --git a/arch/powerpc/lib/Makefile b/arch/powerpc/lib/Makefile
>> index b8de3be10eb4..a15060b5008e 100644
>> --- a/arch/powerpc/lib/Makefile
>> +++ b/arch/powerpc/lib/Makefile
>> @@ -36,7 +36,7 @@ extra-$(CONFIG_PPC64) += crtsavres.o
>> endif
>>
>> obj-$(CONFIG_PPC_BOOK3S_64) += copyuser_power7.o copypage_power7.o \
>> - memcpy_power7.o
>> + memcpy_power7.o uaccess.o
>
> Why only book3s/64 ? It applies to the 8xx and book3s/32 as well, I
> think it should just be for all powerpc.
Okay I can do that.
>>
>> obj64-y += copypage_64.o copyuser_64.o mem_64.o hweight_64.o \
>> memcpy_64.o memcpy_mcsafe_64.o
>> diff --git a/arch/powerpc/lib/uaccess.c b/arch/powerpc/lib/uaccess.c
>> new file mode 100644
>> index 000000000000..0057ab52d6fe
>> --- /dev/null
>> +++ b/arch/powerpc/lib/uaccess.c
>> @@ -0,0 +1,50 @@
>> +#include <linux/mm.h>
>> +#include <linux/uaccess.h>
>> +
>> +static __always_inline long
>> +probe_read_common(void *dst, const void __user *src, size_t size)
>> +{
>> + long ret;
>> +
>> + pagefault_disable();
>> + ret = raw_copy_from_user_allowed(dst, src, size);
>> + pagefault_enable();
>> +
>> + return ret ? -EFAULT : 0;
>> +}
>> +
>> +static __always_inline long
>> +probe_write_common(void __user *dst, const void *src, size_t size)
>> +{
>> + long ret;
>> +
>> + pagefault_disable();
>> + ret = raw_copy_to_user_allowed(dst, src, size);
>> + pagefault_enable();
>> +
>> + return ret ? -EFAULT : 0;
>> +}
>> +
>> +long probe_kernel_read(void *dst, const void *src, size_t size)
>> +{
>> + long ret;
>> + mm_segment_t old_fs = get_fs();
>> +
>> + set_fs(KERNEL_DS);
>> + ret = probe_read_common(dst, (__force const void __user *)src, size);
>
> I think you should squash probe_read_common() here, having it separated
> is a lot of lines for no added value. It also may make people believe it
> overwrites the generic probe_read_common()
Yeah I'll see how that looks.
Thanks,
Nick
^ permalink raw reply
* Re: linux-next: Tree for Apr 3
From: Yuehaibing @ 2020-04-03 9:00 UTC (permalink / raw)
To: Stephen Rothwell, Linux Next Mailing List
Cc: linuxppc-dev, Linux Kernel Mailing List, paulus
In-Reply-To: <62438df4-3546-bfea-6e7f-825faaacfaff@huawei.com>
+cc linuxppc-dev@lists.ozlabs.org
On 2020/4/3 16:54, Yuehaibing wrote:
>
>
> On 2020/4/3 13:29, Stephen Rothwell wrote:
>> Hi all,
>>
>> The merge window has opened, so please do not add any material for the
>> next release into your linux-next included trees/branches until after
>> the merge window closes.
>>
>> Changes since 20200402:
>>
>
> On PPC32, randcondfig warning this:
>
> WARNING: unmet direct dependencies detected for HOTPLUG_CPU
> Depends on [n]: SMP [=y] && (PPC_PSERIES [=n] || PPC_PMAC [=n] || PPC_POWERNV [=n] || FSL_SOC_BOOKE [=n])
> Selected by [y]:
> - PM_SLEEP_SMP [=y] && SMP [=y] && (ARCH_SUSPEND_POSSIBLE [=y] || ARCH_HIBERNATION_POSSIBLE [=y]) && PM_SLEEP [=y]
>
^ permalink raw reply
* [RFC] cpuidle/powernv : Support for pre-entry and post exit of stop state in firmware
From: Abhishek Goel @ 2020-04-03 9:27 UTC (permalink / raw)
To: linuxppc-dev, linux-kernel
Cc: ego, mikey, npiggin, oohall, psampat, Abhishek Goel
This patch provides kernel framework fro opal support of save restore
of sprs in idle stop loop. Opal support for stop states is needed to
selectively enable stop states or to introduce a quirk quickly in case
a buggy stop state is present.
We make a opal call from kernel if firmware-stop-support for stop
states is present and enabled. All the quirks for pre-entry of stop
state is handled inside opal. A call from opal is made into kernel
where we execute stop afer saving of NVGPRs.
After waking up from 0x100 vector in kernel, we enter back into opal.
All the quirks in post exit path, if any, are then handled in opal,
from where we return successfully back to kernel.
For deep stop states in which additional SPRs are lost, saving and
restoration will be done in OPAL.
This idea was first proposed by Nick here:
https://patchwork.ozlabs.org/patch/1208159/
The corresponding skiboot patch for this kernel patch is here:
https://patchwork.ozlabs.org/patch/1265959/
When we callback from OPAL into kernel, r13 is clobbered. So, to
access PACA we need to restore it from HSPRGO. In future we can
handle this into OPAL as in here:
https://patchwork.ozlabs.org/patch/1245275/
Signed-off-by: Abhishek Goel <huntbag@linux.vnet.ibm.com>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/include/asm/opal-api.h | 8 ++++-
arch/powerpc/include/asm/opal.h | 3 ++
arch/powerpc/kernel/idle_book3s.S | 5 +++
arch/powerpc/platforms/powernv/idle.c | 37 ++++++++++++++++++++++
arch/powerpc/platforms/powernv/opal-call.c | 2 ++
5 files changed, 54 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/include/asm/opal-api.h b/arch/powerpc/include/asm/opal-api.h
index c1f25a760eb1..a2c782c99c9e 100644
--- a/arch/powerpc/include/asm/opal-api.h
+++ b/arch/powerpc/include/asm/opal-api.h
@@ -214,7 +214,9 @@
#define OPAL_SECVAR_GET 176
#define OPAL_SECVAR_GET_NEXT 177
#define OPAL_SECVAR_ENQUEUE_UPDATE 178
-#define OPAL_LAST 178
+#define OPAL_REGISTER_OS_OPS 181
+#define OPAL_CPU_IDLE 182
+#define OPAL_LAST 182
#define QUIESCE_HOLD 1 /* Spin all calls at entry */
#define QUIESCE_REJECT 2 /* Fail all calls with OPAL_BUSY */
@@ -1181,6 +1183,10 @@ struct opal_mpipl_fadump {
struct opal_mpipl_region region[];
} __packed;
+struct opal_os_ops {
+ __be64 os_idle_stop;
+};
+
#endif /* __ASSEMBLY__ */
#endif /* __OPAL_API_H */
diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h
index 9986ac34b8e2..3c340bc4df8e 100644
--- a/arch/powerpc/include/asm/opal.h
+++ b/arch/powerpc/include/asm/opal.h
@@ -400,6 +400,9 @@ void opal_powercap_init(void);
void opal_psr_init(void);
void opal_sensor_groups_init(void);
+extern int64_t opal_register_os_ops(struct opal_os_ops *os_ops);
+extern int64_t opal_cpu_idle(__be64 srr1_addr, uint64_t psscr);
+
#endif /* __ASSEMBLY__ */
#endif /* _ASM_POWERPC_OPAL_H */
diff --git a/arch/powerpc/kernel/idle_book3s.S b/arch/powerpc/kernel/idle_book3s.S
index 22f249b6f58d..8d287d1d06c0 100644
--- a/arch/powerpc/kernel/idle_book3s.S
+++ b/arch/powerpc/kernel/idle_book3s.S
@@ -49,6 +49,8 @@ _GLOBAL(isa300_idle_stop_noloss)
*/
_GLOBAL(isa300_idle_stop_mayloss)
mtspr SPRN_PSSCR,r3
+ mr r6, r13
+ mfspr r13, SPRN_HSPRG0
std r1,PACAR1(r13)
mflr r4
mfcr r5
@@ -74,6 +76,7 @@ _GLOBAL(isa300_idle_stop_mayloss)
std r31,-8*18(r1)
std r4,-8*19(r1)
std r5,-8*20(r1)
+ std r6,-8*21(r1)
/* 168 bytes */
PPC_STOP
b . /* catch bugs */
@@ -91,8 +94,10 @@ _GLOBAL(idle_return_gpr_loss)
ld r1,PACAR1(r13)
ld r4,-8*19(r1)
ld r5,-8*20(r1)
+ ld r6,-8*21(r1)
mtlr r4
mtcr r5
+ mr r13,r6
/*
* KVM nap requires r2 to be saved, rather than just restoring it
* from PACATOC. This could be avoided for that less common case
diff --git a/arch/powerpc/platforms/powernv/idle.c b/arch/powerpc/platforms/powernv/idle.c
index 78599bca66c2..1841027b25c5 100644
--- a/arch/powerpc/platforms/powernv/idle.c
+++ b/arch/powerpc/platforms/powernv/idle.c
@@ -35,6 +35,7 @@
static u32 supported_cpuidle_states;
struct pnv_idle_states_t *pnv_idle_states;
int nr_pnv_idle_states;
+static bool firmware_stop_supported;
/*
* The default stop state that will be used by ppc_md.power_save
@@ -602,6 +603,25 @@ struct p9_sprs {
u64 uamor;
};
+/*
+ * This function is called from OPAL if firmware support for stop
+ * states is present and enabled. It provides a fallback for idle
+ * stop states via OPAL.
+ */
+static uint64_t os_idle_stop(uint64_t psscr, bool save_gprs)
+{
+ /*
+ * For lite state which does not lose even GPRS we call
+ * idle_stop_noloss while for all other states we call
+ * idle_stop_mayloss. Saving and restoration of other additional
+ * SPRs if required is handled in OPAL. All the quirks are also
+ * handled in OPAL.
+ */
+ if (!save_gprs)
+ return isa300_idle_stop_noloss(psscr);
+ return isa300_idle_stop_mayloss(psscr);
+}
+
static unsigned long power9_idle_stop(unsigned long psscr, bool mmu_on)
{
int cpu = raw_smp_processor_id();
@@ -613,6 +633,16 @@ static unsigned long power9_idle_stop(unsigned long psscr, bool mmu_on)
unsigned long mmcr0 = 0;
struct p9_sprs sprs = {}; /* avoid false used-uninitialised */
bool sprs_saved = false;
+ int rc = 0;
+
+ /*
+ * Kernel takes decision whether to make OPAL call or not. This logic
+ * will be combined with the logic for BE opal to take decision.
+ */
+ if (firmware_stop_supported) {
+ rc = opal_cpu_idle(cpu_to_be64(__pa(&srr1)), (uint64_t) psscr);
+ goto out;
+ }
if (!(psscr & (PSSCR_EC|PSSCR_ESL))) {
/* EC=ESL=0 case */
@@ -1232,6 +1262,10 @@ static int pnv_parse_cpuidle_dt(void)
pr_warn("opal: PowerMgmt Node not found\n");
return -ENODEV;
}
+
+ if (of_device_is_compatible(np, "firmware-stop-supported"))
+ firmware_stop_supported = true;
+
nr_idle_states = of_property_count_u32_elems(np,
"ibm,cpu-idle-state-flags");
@@ -1326,6 +1360,7 @@ static int pnv_parse_cpuidle_dt(void)
static int __init pnv_init_idle_states(void)
{
+ struct opal_os_ops os_ops;
int cpu;
int rc = 0;
@@ -1349,6 +1384,8 @@ static int __init pnv_init_idle_states(void)
}
}
+ os_ops.os_idle_stop = be64_to_cpu(os_idle_stop);
+ rc = opal_register_os_ops((struct opal_os_ops *)(&os_ops));
/* In case we error out nr_pnv_idle_states will be zero */
nr_pnv_idle_states = 0;
supported_cpuidle_states = 0;
diff --git a/arch/powerpc/platforms/powernv/opal-call.c b/arch/powerpc/platforms/powernv/opal-call.c
index 5cd0f52d258f..c885e607ba62 100644
--- a/arch/powerpc/platforms/powernv/opal-call.c
+++ b/arch/powerpc/platforms/powernv/opal-call.c
@@ -293,3 +293,5 @@ OPAL_CALL(opal_mpipl_query_tag, OPAL_MPIPL_QUERY_TAG);
OPAL_CALL(opal_secvar_get, OPAL_SECVAR_GET);
OPAL_CALL(opal_secvar_get_next, OPAL_SECVAR_GET_NEXT);
OPAL_CALL(opal_secvar_enqueue_update, OPAL_SECVAR_ENQUEUE_UPDATE);
+OPAL_CALL(opal_register_os_ops, OPAL_REGISTER_OS_OPS);
+OPAL_CALL(opal_cpu_idle, OPAL_CPU_IDLE);
--
2.17.1
^ permalink raw reply related
* Re: [PATCH 2/4] ocxl: Access interrupt trigger page from xive directly
From: Greg Kurz @ 2020-04-03 9:17 UTC (permalink / raw)
To: Frederic Barrat
Cc: ukrishn, ajd, haren, clg, linuxppc-dev, christophe_lombard,
mrochs
In-Reply-To: <20200402154352.586166-3-fbarrat@linux.ibm.com>
On Thu, 2 Apr 2020 17:43:50 +0200
Frederic Barrat <fbarrat@linux.ibm.com> wrote:
> We can access the trigger page through standard APIs so let's use it
> and avoid saving it when allocating the interrupt. It will also allow
> to simplify allocation in a later patch.
>
> Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
> ---
Reviewed-by: Greg Kurz <groug@kaod.org>
> drivers/misc/ocxl/afu_irq.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/misc/ocxl/afu_irq.c b/drivers/misc/ocxl/afu_irq.c
> index 70f8f1c3929d..b30ec0ef7be7 100644
> --- a/drivers/misc/ocxl/afu_irq.c
> +++ b/drivers/misc/ocxl/afu_irq.c
> @@ -2,6 +2,7 @@
> // Copyright 2017 IBM Corp.
> #include <linux/interrupt.h>
> #include <asm/pnv-ocxl.h>
> +#include <asm/xive.h>
> #include "ocxl_internal.h"
> #include "trace.h"
>
> @@ -196,13 +197,16 @@ void ocxl_afu_irq_free_all(struct ocxl_context *ctx)
>
> u64 ocxl_afu_irq_get_addr(struct ocxl_context *ctx, int irq_id)
> {
> + struct xive_irq_data *xd;
> struct afu_irq *irq;
> u64 addr = 0;
>
> mutex_lock(&ctx->irq_lock);
> irq = idr_find(&ctx->irq_idr, irq_id);
> - if (irq)
> - addr = irq->trigger_page;
> + if (irq) {
> + xd = irq_get_handler_data(irq->virq);
> + addr = xd ? xd->trig_page : 0;
> + }
> mutex_unlock(&ctx->irq_lock);
> return addr;
> }
^ permalink raw reply
* Re: [PATCH 2/4] ocxl: Access interrupt trigger page from xive directly
From: Cédric Le Goater @ 2020-04-03 5:55 UTC (permalink / raw)
To: Frederic Barrat, linuxppc-dev, christophe_lombard, ajd, ukrishn,
mrochs
Cc: haren, groug
In-Reply-To: <20200402154352.586166-3-fbarrat@linux.ibm.com>
On 4/2/20 5:43 PM, Frederic Barrat wrote:
> We can access the trigger page through standard APIs so let's use it
> and avoid saving it when allocating the interrupt. It will also allow
> to simplify allocation in a later patch.
>
> Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
> ---
> drivers/misc/ocxl/afu_irq.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/misc/ocxl/afu_irq.c b/drivers/misc/ocxl/afu_irq.c
> index 70f8f1c3929d..b30ec0ef7be7 100644
> --- a/drivers/misc/ocxl/afu_irq.c
> +++ b/drivers/misc/ocxl/afu_irq.c
> @@ -2,6 +2,7 @@
> // Copyright 2017 IBM Corp.
> #include <linux/interrupt.h>
> #include <asm/pnv-ocxl.h>
> +#include <asm/xive.h>
> #include "ocxl_internal.h"
> #include "trace.h"
>
> @@ -196,13 +197,16 @@ void ocxl_afu_irq_free_all(struct ocxl_context *ctx)
>
> u64 ocxl_afu_irq_get_addr(struct ocxl_context *ctx, int irq_id)
> {
> + struct xive_irq_data *xd;
> struct afu_irq *irq;
> u64 addr = 0;
>
> mutex_lock(&ctx->irq_lock);
> irq = idr_find(&ctx->irq_idr, irq_id);
> - if (irq)
> - addr = irq->trigger_page;
> + if (irq) {
> + xd = irq_get_handler_data(irq->virq);
> + addr = xd ? xd->trig_page : 0;
> + }
> mutex_unlock(&ctx->irq_lock);
> return addr;
> }
>
^ permalink raw reply
* [PATCH v2 1/4] powerpc/64s: implement probe_kernel_read/write without touching AMR
From: Nicholas Piggin @ 2020-04-03 9:35 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Nicholas Piggin
There is no need to allow user accesses when probing kernel addresses.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
v2:
- Enable for all powerpc (suggested by Christophe)
- Fold helper function together (Christophe)
- Rename uaccess.c to maccess.c to match kernel/maccess.c.
arch/powerpc/include/asm/uaccess.h | 25 +++++++++++++++-------
arch/powerpc/lib/Makefile | 2 +-
arch/powerpc/lib/maccess.c | 34 ++++++++++++++++++++++++++++++
3 files changed, 52 insertions(+), 9 deletions(-)
create mode 100644 arch/powerpc/lib/maccess.c
diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index 2f500debae21..670910df3cc7 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -341,8 +341,8 @@ raw_copy_in_user(void __user *to, const void __user *from, unsigned long n)
}
#endif /* __powerpc64__ */
-static inline unsigned long raw_copy_from_user(void *to,
- const void __user *from, unsigned long n)
+static inline unsigned long
+raw_copy_from_user_allowed(void *to, const void __user *from, unsigned long n)
{
unsigned long ret;
if (__builtin_constant_p(n) && (n <= 8)) {
@@ -351,19 +351,19 @@ static inline unsigned long raw_copy_from_user(void *to,
switch (n) {
case 1:
barrier_nospec();
- __get_user_size(*(u8 *)to, from, 1, ret);
+ __get_user_size_allowed(*(u8 *)to, from, 1, ret);
break;
case 2:
barrier_nospec();
- __get_user_size(*(u16 *)to, from, 2, ret);
+ __get_user_size_allowed(*(u16 *)to, from, 2, ret);
break;
case 4:
barrier_nospec();
- __get_user_size(*(u32 *)to, from, 4, ret);
+ __get_user_size_allowed(*(u32 *)to, from, 4, ret);
break;
case 8:
barrier_nospec();
- __get_user_size(*(u64 *)to, from, 8, ret);
+ __get_user_size_allowed(*(u64 *)to, from, 8, ret);
break;
}
if (ret == 0)
@@ -371,9 +371,18 @@ static inline unsigned long raw_copy_from_user(void *to,
}
barrier_nospec();
- allow_read_from_user(from, n);
ret = __copy_tofrom_user((__force void __user *)to, from, n);
- prevent_read_from_user(from, n);
+ return ret;
+}
+
+static inline unsigned long
+raw_copy_from_user(void *to, const void __user *from, unsigned long n)
+{
+ unsigned long ret;
+
+ allow_read_from_user(to, n);
+ ret = raw_copy_from_user_allowed(to, from, n);
+ prevent_read_from_user(to, n);
return ret;
}
diff --git a/arch/powerpc/lib/Makefile b/arch/powerpc/lib/Makefile
index b8de3be10eb4..77af10ad0b0d 100644
--- a/arch/powerpc/lib/Makefile
+++ b/arch/powerpc/lib/Makefile
@@ -16,7 +16,7 @@ CFLAGS_code-patching.o += -DDISABLE_BRANCH_PROFILING
CFLAGS_feature-fixups.o += -DDISABLE_BRANCH_PROFILING
endif
-obj-y += alloc.o code-patching.o feature-fixups.o pmem.o
+obj-y += alloc.o code-patching.o feature-fixups.o pmem.o maccess.o
ifndef CONFIG_KASAN
obj-y += string.o memcmp_$(BITS).o
diff --git a/arch/powerpc/lib/maccess.c b/arch/powerpc/lib/maccess.c
new file mode 100644
index 000000000000..ce5465db1e2d
--- /dev/null
+++ b/arch/powerpc/lib/maccess.c
@@ -0,0 +1,34 @@
+#include <linux/mm.h>
+#include <linux/uaccess.h>
+
+/*
+ * Override the generic weak linkage functions to avoid changing KUP state via
+ * the generic user access functions, as this is accessing kernel addresses.
+ */
+long probe_kernel_read(void *dst, const void *src, size_t size)
+{
+ long ret;
+ mm_segment_t old_fs = get_fs();
+
+ set_fs(KERNEL_DS);
+ pagefault_disable();
+ ret = raw_copy_from_user_allowed(dst, (__force const void __user *)src, size);
+ pagefault_enable();
+ set_fs(old_fs);
+
+ return ret ? -EFAULT : 0;
+}
+
+long probe_kernel_write(void *dst, const void *src, size_t size)
+{
+ long ret;
+ mm_segment_t old_fs = get_fs();
+
+ set_fs(KERNEL_DS);
+ pagefault_disable();
+ ret = raw_copy_to_user_allowed((__force void __user *)dst, src, size);
+ pagefault_enable();
+ set_fs(old_fs);
+
+ return ret ? -EFAULT : 0;
+}
--
2.23.0
^ permalink raw reply related
* [PATCH v2 2/4] powerpc/64s: use mmu_has_feature in set_kuap() and get_kuap()
From: Nicholas Piggin @ 2020-04-03 9:35 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Nicholas Piggin
In-Reply-To: <20200403093529.43587-1-npiggin@gmail.com>
Commit 8150a153c013 ("powerpc/64s: Use early_mmu_has_feature() in
set_kuap()"), had to switch to using the _early feature test, because
probe_kernel_read was being called very early. After the previous
patch, probe_kernel_read no longer touches kuap, so it can go back to
using the non-_early variant, for better performance.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/include/asm/book3s/64/kup-radix.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/include/asm/book3s/64/kup-radix.h b/arch/powerpc/include/asm/book3s/64/kup-radix.h
index 3bcef989a35d..67a7fd0182e6 100644
--- a/arch/powerpc/include/asm/book3s/64/kup-radix.h
+++ b/arch/powerpc/include/asm/book3s/64/kup-radix.h
@@ -79,7 +79,7 @@ static inline void kuap_check_amr(void)
static inline unsigned long get_kuap(void)
{
- if (!early_mmu_has_feature(MMU_FTR_RADIX_KUAP))
+ if (!mmu_has_feature(MMU_FTR_RADIX_KUAP))
return 0;
return mfspr(SPRN_AMR);
@@ -87,7 +87,7 @@ static inline unsigned long get_kuap(void)
static inline void set_kuap(unsigned long value)
{
- if (!early_mmu_has_feature(MMU_FTR_RADIX_KUAP))
+ if (!mmu_has_feature(MMU_FTR_RADIX_KUAP))
return;
/*
--
2.23.0
^ permalink raw reply related
* [PATCH v2 3/4] powerpc/uaccess: evaluate macro arguments once, before user access is allowed
From: Nicholas Piggin @ 2020-04-03 9:35 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Nicholas Piggin
In-Reply-To: <20200403093529.43587-1-npiggin@gmail.com>
get/put_user can be called with nontrivial arguments. fs/proc/page.c
has a good example:
if (put_user(stable_page_flags(ppage), out)) {
stable_page_flags is quite a lot of code, including spin locks in the
page allocator.
Ensure these arguments are evaluated before user access is allowed.
This improves security by reducing code with access to userspace, but
it also fixes a PREEMPT bug with KUAP on powerpc/64s:
stable_page_flags is currently called with AMR set to allow writes,
it ends up calling spin_unlock(), which can call preempt_schedule. But
the task switch code can not be called with AMR set (it relies on
interrupts saving the register), so this blows up.
It's fine if the code inside allow_user_access is preemptible, because
a timer or IPI will save the AMR, but it's not okay to explicitly
cause a reschedule.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
v2:
- Reduce unnecessary lines changed (Christophe)
arch/powerpc/include/asm/uaccess.h | 49 +++++++++++++++++++++---------
1 file changed, 35 insertions(+), 14 deletions(-)
diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index 670910df3cc7..144d01645d68 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -166,13 +166,17 @@ do { \
({ \
long __pu_err; \
__typeof__(*(ptr)) __user *__pu_addr = (ptr); \
+ __typeof__(*(ptr)) __pu_val = (x); \
+ __typeof__(size) __pu_size = (size); \
+ \
if (!is_kernel_addr((unsigned long)__pu_addr)) \
might_fault(); \
- __chk_user_ptr(ptr); \
+ __chk_user_ptr(__pu_addr); \
if (do_allow) \
- __put_user_size((x), __pu_addr, (size), __pu_err); \
+ __put_user_size(__pu_val, __pu_addr, __pu_size, __pu_err); \
else \
- __put_user_size_allowed((x), __pu_addr, (size), __pu_err); \
+ __put_user_size_allowed(__pu_val, __pu_addr, __pu_size, __pu_err); \
+ \
__pu_err; \
})
@@ -180,9 +184,13 @@ do { \
({ \
long __pu_err = -EFAULT; \
__typeof__(*(ptr)) __user *__pu_addr = (ptr); \
+ __typeof__(*(ptr)) __pu_val = (x); \
+ __typeof__(size) __pu_size = (size); \
+ \
might_fault(); \
- if (access_ok(__pu_addr, size)) \
- __put_user_size((x), __pu_addr, (size), __pu_err); \
+ if (access_ok(__pu_addr, __pu_size)) \
+ __put_user_size(__pu_val, __pu_addr, __pu_size, __pu_err); \
+ \
__pu_err; \
})
@@ -190,8 +198,12 @@ do { \
({ \
long __pu_err; \
__typeof__(*(ptr)) __user *__pu_addr = (ptr); \
- __chk_user_ptr(ptr); \
- __put_user_size((x), __pu_addr, (size), __pu_err); \
+ __typeof__(*(ptr)) __pu_val = (x); \
+ __typeof__(size) __pu_size = (size); \
+ \
+ __chk_user_ptr(__pu_addr); \
+ __put_user_size(__pu_val, __pu_addr, __pu_size, __pu_err); \
+ \
__pu_err; \
})
@@ -283,15 +295,18 @@ do { \
long __gu_err; \
__long_type(*(ptr)) __gu_val; \
__typeof__(*(ptr)) __user *__gu_addr = (ptr); \
- __chk_user_ptr(ptr); \
+ __typeof__(size) __gu_size = (size); \
+ \
+ __chk_user_ptr(__gu_addr); \
if (!is_kernel_addr((unsigned long)__gu_addr)) \
might_fault(); \
barrier_nospec(); \
if (do_allow) \
- __get_user_size(__gu_val, __gu_addr, (size), __gu_err); \
+ __get_user_size(__gu_val, __gu_addr, __gu_size, __gu_err); \
else \
- __get_user_size_allowed(__gu_val, __gu_addr, (size), __gu_err); \
+ __get_user_size_allowed(__gu_val, __gu_addr, __gu_size, __gu_err); \
(x) = (__typeof__(*(ptr)))__gu_val; \
+ \
__gu_err; \
})
@@ -300,12 +315,15 @@ do { \
long __gu_err = -EFAULT; \
__long_type(*(ptr)) __gu_val = 0; \
__typeof__(*(ptr)) __user *__gu_addr = (ptr); \
+ __typeof__(size) __gu_size = (size); \
+ \
might_fault(); \
- if (access_ok(__gu_addr, (size))) { \
+ if (access_ok(__gu_addr, __gu_size)) { \
barrier_nospec(); \
- __get_user_size(__gu_val, __gu_addr, (size), __gu_err); \
+ __get_user_size(__gu_val, __gu_addr, __gu_size, __gu_err); \
} \
(x) = (__force __typeof__(*(ptr)))__gu_val; \
+ \
__gu_err; \
})
@@ -314,10 +332,13 @@ do { \
long __gu_err; \
__long_type(*(ptr)) __gu_val; \
__typeof__(*(ptr)) __user *__gu_addr = (ptr); \
- __chk_user_ptr(ptr); \
+ __typeof__(size) __gu_size = (size); \
+ \
+ __chk_user_ptr(__gu_addr); \
barrier_nospec(); \
- __get_user_size(__gu_val, __gu_addr, (size), __gu_err); \
+ __get_user_size(__gu_val, __gu_addr, __gu_size, __gu_err); \
(x) = (__force __typeof__(*(ptr)))__gu_val; \
+ \
__gu_err; \
})
--
2.23.0
^ permalink raw reply related
* [PATCH v2 4/4] powerpc/uaccess: add more __builtin_expect annotations
From: Nicholas Piggin @ 2020-04-03 9:35 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Nicholas Piggin
In-Reply-To: <20200403093529.43587-1-npiggin@gmail.com>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/include/asm/uaccess.h | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index 144d01645d68..c6b0203c3750 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -48,16 +48,16 @@ static inline void set_fs(mm_segment_t fs)
* gap between user addresses and the kernel addresses
*/
#define __access_ok(addr, size, segment) \
- (((addr) <= (segment).seg) && ((size) <= (segment).seg))
+ likely(((addr) <= (segment).seg) && ((size) <= (segment).seg))
#else
static inline int __access_ok(unsigned long addr, unsigned long size,
mm_segment_t seg)
{
- if (addr > seg.seg)
+ if (unlikely(addr > seg.seg))
return 0;
- return (size == 0 || size - 1 <= seg.seg - addr);
+ return likely(size == 0 || size - 1 <= seg.seg - addr);
}
#endif
@@ -177,7 +177,7 @@ do { \
else \
__put_user_size_allowed(__pu_val, __pu_addr, __pu_size, __pu_err); \
\
- __pu_err; \
+ __builtin_expect(__pu_err, 0); \
})
#define __put_user_check(x, ptr, size) \
@@ -192,6 +192,7 @@ do { \
__put_user_size(__pu_val, __pu_addr, __pu_size, __pu_err); \
\
__pu_err; \
+ __builtin_expect(__pu_err, 0); \
})
#define __put_user_nosleep(x, ptr, size) \
@@ -204,7 +205,7 @@ do { \
__chk_user_ptr(__pu_addr); \
__put_user_size(__pu_val, __pu_addr, __pu_size, __pu_err); \
\
- __pu_err; \
+ __builtin_expect(__pu_err, 0); \
})
@@ -307,7 +308,7 @@ do { \
__get_user_size_allowed(__gu_val, __gu_addr, __gu_size, __gu_err); \
(x) = (__typeof__(*(ptr)))__gu_val; \
\
- __gu_err; \
+ __builtin_expect(__gu_err, 0); \
})
#define __get_user_check(x, ptr, size) \
@@ -325,6 +326,7 @@ do { \
(x) = (__force __typeof__(*(ptr)))__gu_val; \
\
__gu_err; \
+ __builtin_expect(__gu_err, 0); \
})
#define __get_user_nosleep(x, ptr, size) \
@@ -339,7 +341,7 @@ do { \
__get_user_size(__gu_val, __gu_addr, __gu_size, __gu_err); \
(x) = (__force __typeof__(*(ptr)))__gu_val; \
\
- __gu_err; \
+ __builtin_expect(__gu_err, 0); \
})
--
2.23.0
^ permalink raw reply related
* Re: [PATCH v8 2/7] powerpc/kprobes: Mark newly allocated probes as RO
From: Naveen N. Rao @ 2020-04-03 9:36 UTC (permalink / raw)
To: linuxppc-dev, Russell Currey; +Cc: kernel-hardening, ajd, npiggin, dja
In-Reply-To: <c336400d5b7765eb72b3090cd9f8a3c57761d0b6.camel@russell.cc>
Russell Currey wrote:
> On Fri, 2020-04-03 at 00:18 +0530, Naveen N. Rao wrote:
>> Naveen N. Rao wrote:
>> > Russell Currey wrote:
>> > > With CONFIG_STRICT_KERNEL_RWX=y and CONFIG_KPROBES=y, there will
>> > > be one
>> > > W+X page at boot by default. This can be tested with
>> > > CONFIG_PPC_PTDUMP=y and CONFIG_PPC_DEBUG_WX=y set, and checking
>> > > the
>> > > kernel log during boot.
>> > >
>> > > powerpc doesn't implement its own alloc() for kprobes like other
>> > > architectures do, but we couldn't immediately mark RO anyway
>> > > since we do
>> > > a memcpy to the page we allocate later. After that, nothing
>> > > should be
>> > > allowed to modify the page, and write permissions are removed
>> > > well
>> > > before the kprobe is armed.
>> > >
>> > > The memcpy() would fail if >1 probes were allocated, so use
>> > > patch_instruction() instead which is safe for RO.
>> > >
>> > > Reviewed-by: Daniel Axtens <dja@axtens.net>
>> > > Signed-off-by: Russell Currey <ruscur@russell.cc>
>> > > Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>> > > ---
>> > > arch/powerpc/kernel/kprobes.c | 17 +++++++++++++----
>> > > 1 file changed, 13 insertions(+), 4 deletions(-)
>> > >
>> > > diff --git a/arch/powerpc/kernel/kprobes.c
>> > > b/arch/powerpc/kernel/kprobes.c
>> > > index 81efb605113e..fa4502b4de35 100644
>> > > --- a/arch/powerpc/kernel/kprobes.c
>> > > +++ b/arch/powerpc/kernel/kprobes.c
>> > > @@ -24,6 +24,8 @@
>> > > #include <asm/sstep.h>
>> > > #include <asm/sections.h>
>> > > #include <linux/uaccess.h>
>> > > +#include <linux/set_memory.h>
>> > > +#include <linux/vmalloc.h>
>> > >
>> > > DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL;
>> > > DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
>> > > @@ -102,6 +104,16 @@ kprobe_opcode_t *kprobe_lookup_name(const
>> > > char *name, unsigned int offset)
>> > > return addr;
>> > > }
>> > >
>> > > +void *alloc_insn_page(void)
>> > > +{
>> > > + void *page = vmalloc_exec(PAGE_SIZE);
>> > > +
>> > > + if (page)
>> > > + set_memory_ro((unsigned long)page, 1);
>> > > +
>> > > + return page;
>> > > +}
>> > > +
>> >
>> > This crashes for me with KPROBES_SANITY_TEST during the kretprobe
>> > test.
>>
>> That isn't needed to reproduce this. After bootup, disabling
>> optprobes
>> also shows the crash with kretprobes:
>> sysctl debug.kprobes-optimization=0
>>
>> The problem happens to be with patch_instruction() in
>> arch_prepare_kprobe(). During boot, on kprobe init, we register a
>> probe
>> on kretprobe_trampoline for use with kretprobes (see
>> arch_init_kprobes()). This results in an instruction slot being
>> allocated, and arch_prepare_kprobe() to be called for copying the
>> instruction (nop) at kretprobe_trampoline. patch_instruction() is
>> failing resulting in corrupt instruction which we try to
>> emulate/single
>> step causing the crash.
>
> OK I think I've fixed it, KPROBES_SANITY_TEST passes too. I'd
> appreciate it if you could test v9, and thanks again for finding this -
> very embarrassing bug on my side.
Great! Thanks.
I think I should also add appropriate error checking to kprobes' use of
patch_instruction() which would have caught this much more easily.
On a related note, I notice that x86 seems to prefer not having any RWX
pages, and so they continue to do 'module_alloc()' followed by
'set_memory_ro()' and then 'set_memory_x()'. Is that something worth
following for powerpc?
- Naveen
^ permalink raw reply
* Re: [PATCH v8 2/7] powerpc/kprobes: Mark newly allocated probes as RO
From: Russell Currey @ 2020-04-03 9:42 UTC (permalink / raw)
To: Naveen N. Rao, linuxppc-dev; +Cc: kernel-hardening, ajd, npiggin, dja
In-Reply-To: <1585906281.fbqgtc3kpy.naveen@linux.ibm.com>
On Fri, 2020-04-03 at 15:06 +0530, Naveen N. Rao wrote:
> Russell Currey wrote:
> > On Fri, 2020-04-03 at 00:18 +0530, Naveen N. Rao wrote:
> > > Naveen N. Rao wrote:
> > > > Russell Currey wrote:
> > > > > With CONFIG_STRICT_KERNEL_RWX=y and CONFIG_KPROBES=y, there
> > > > > will
> > > > > be one
> > > > > W+X page at boot by default. This can be tested with
> > > > > CONFIG_PPC_PTDUMP=y and CONFIG_PPC_DEBUG_WX=y set, and
> > > > > checking
> > > > > the
> > > > > kernel log during boot.
> > > > >
> > > > > powerpc doesn't implement its own alloc() for kprobes like
> > > > > other
> > > > > architectures do, but we couldn't immediately mark RO anyway
> > > > > since we do
> > > > > a memcpy to the page we allocate later. After that, nothing
> > > > > should be
> > > > > allowed to modify the page, and write permissions are removed
> > > > > well
> > > > > before the kprobe is armed.
> > > > >
> > > > > The memcpy() would fail if >1 probes were allocated, so use
> > > > > patch_instruction() instead which is safe for RO.
> > > > >
> > > > > Reviewed-by: Daniel Axtens <dja@axtens.net>
> > > > > Signed-off-by: Russell Currey <ruscur@russell.cc>
> > > > > Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> > > > > ---
> > > > > arch/powerpc/kernel/kprobes.c | 17 +++++++++++++----
> > > > > 1 file changed, 13 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/arch/powerpc/kernel/kprobes.c
> > > > > b/arch/powerpc/kernel/kprobes.c
> > > > > index 81efb605113e..fa4502b4de35 100644
> > > > > --- a/arch/powerpc/kernel/kprobes.c
> > > > > +++ b/arch/powerpc/kernel/kprobes.c
> > > > > @@ -24,6 +24,8 @@
> > > > > #include <asm/sstep.h>
> > > > > #include <asm/sections.h>
> > > > > #include <linux/uaccess.h>
> > > > > +#include <linux/set_memory.h>
> > > > > +#include <linux/vmalloc.h>
> > > > >
> > > > > DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL;
> > > > > DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
> > > > > @@ -102,6 +104,16 @@ kprobe_opcode_t
> > > > > *kprobe_lookup_name(const
> > > > > char *name, unsigned int offset)
> > > > > return addr;
> > > > > }
> > > > >
> > > > > +void *alloc_insn_page(void)
> > > > > +{
> > > > > + void *page = vmalloc_exec(PAGE_SIZE);
> > > > > +
> > > > > + if (page)
> > > > > + set_memory_ro((unsigned long)page, 1);
> > > > > +
> > > > > + return page;
> > > > > +}
> > > > > +
> > > >
> > > > This crashes for me with KPROBES_SANITY_TEST during the
> > > > kretprobe
> > > > test.
> > >
> > > That isn't needed to reproduce this. After bootup, disabling
> > > optprobes
> > > also shows the crash with kretprobes:
> > > sysctl debug.kprobes-optimization=0
> > >
> > > The problem happens to be with patch_instruction() in
> > > arch_prepare_kprobe(). During boot, on kprobe init, we register a
> > > probe
> > > on kretprobe_trampoline for use with kretprobes (see
> > > arch_init_kprobes()). This results in an instruction slot being
> > > allocated, and arch_prepare_kprobe() to be called for copying
> > > the
> > > instruction (nop) at kretprobe_trampoline. patch_instruction()
> > > is
> > > failing resulting in corrupt instruction which we try to
> > > emulate/single
> > > step causing the crash.
> >
> > OK I think I've fixed it, KPROBES_SANITY_TEST passes too. I'd
> > appreciate it if you could test v9, and thanks again for finding
> > this -
> > very embarrassing bug on my side.
>
> Great! Thanks.
>
> I think I should also add appropriate error checking to kprobes' use
> of
> patch_instruction() which would have caught this much more easily.
Only kind of! It turns out that if the initial setup fails for
KPROBES_SANITY_TEST, it silently doesn't run - so you miss the "Kprobe
smoke test" text, but you don't get any kind of error either. I'll
send a patch so that it fails more loudly later.
>
> On a related note, I notice that x86 seems to prefer not having any
> RWX
> pages, and so they continue to do 'module_alloc()' followed by
> 'set_memory_ro()' and then 'set_memory_x()'. Is that something worth
> following for powerpc?
I just noticed that too. arm64 doesn't set theirs executable, as far
as I can tell powerpc doesn't need to.
>
> - Naveen
>
^ permalink raw reply
* Re: [PATCH v11 0/8] Disable compat cruft on ppc64le v11
From: Nicholas Piggin @ 2020-04-03 9:43 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev, Michal Suchanek
Cc: Mark Rutland, Gustavo Luiz Duarte, Alexander Shishkin,
Sebastian Andrzej Siewior, linux-kernel, Paul Mackerras,
Jiri Olsa, Rob Herring, Michael Neuling, Eric Richter,
Masahiro Yamada, Nayna Jain, Peter Zijlstra, Ingo Molnar,
Hari Bathini, Jordan Niethe, Valentin Schneider, Arnd Bergmann,
Arnaldo Carvalho de Melo, Alexander Viro, Jonathan Cameron,
Namhyung Kim, Thomas Gleixner, Andy Shevchenko, Allison Randal,
Greg Kroah-Hartman, Claudio Carvalho, Mauro Carvalho Chehab,
Eric W. Biederman, linux-fsdevel, David S. Miller,
Thiago Jung Bauermann
In-Reply-To: <1e00a725-9710-2b80-4aff-2f284b31d2e5@c-s.fr>
Christophe Leroy's on April 3, 2020 5:26 pm:
>
>
> Le 03/04/2020 à 09:25, Nicholas Piggin a écrit :
>> Michal Suchanek's on March 19, 2020 10:19 pm:
>>> Less code means less bugs so add a knob to skip the compat stuff.
>>>
>>> Changes in v2: saner CONFIG_COMPAT ifdefs
>>> Changes in v3:
>>> - change llseek to 32bit instead of builing it unconditionally in fs
>>> - clanup the makefile conditionals
>>> - remove some ifdefs or convert to IS_DEFINED where possible
>>> Changes in v4:
>>> - cleanup is_32bit_task and current_is_64bit
>>> - more makefile cleanup
>>> Changes in v5:
>>> - more current_is_64bit cleanup
>>> - split off callchain.c 32bit and 64bit parts
>>> Changes in v6:
>>> - cleanup makefile after split
>>> - consolidate read_user_stack_32
>>> - fix some checkpatch warnings
>>> Changes in v7:
>>> - add back __ARCH_WANT_SYS_LLSEEK to fix build with llseek
>>> - remove leftover hunk
>>> - add review tags
>>> Changes in v8:
>>> - consolidate valid_user_sp to fix it in the split callchain.c
>>> - fix build errors/warnings with PPC64 !COMPAT and PPC32
>>> Changes in v9:
>>> - remove current_is_64bit()
>>> Chanegs in v10:
>>> - rebase, sent together with the syscall cleanup
>>> Changes in v11:
>>> - rebase
>>> - add MAINTAINERS pattern for ppc perf
>>
>> These all look good to me. I had some minor comment about one patch but
>> not really a big deal and there were more cleanups on top of it, so I
>> don't mind if it's merged as is.
>>
>> Actually I think we have a bit of stack reading fixes for 64s radix now
>> (not a bug fix as such, but we don't need the hash fault logic in radix),
>> so if I get around to that I can propose the changes in that series.
>>
>
> As far as I can see, there is a v12
For the most part I was looking at the patches in mpe's next-test
tree on github, if that's the v12 series, same comment applies but
it's a pretty small nitpick.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH 3/4] ocxl: Don't return trigger page when allocating an interrupt
From: Greg Kurz @ 2020-04-03 9:25 UTC (permalink / raw)
To: Frederic Barrat
Cc: ukrishn, ajd, haren, clg, linuxppc-dev, christophe_lombard,
mrochs
In-Reply-To: <20200402154352.586166-4-fbarrat@linux.ibm.com>
On Thu, 2 Apr 2020 17:43:51 +0200
Frederic Barrat <fbarrat@linux.ibm.com> wrote:
> Existing users of ocxl_link_irq_alloc() have been converted to obtain
> the trigger page of an interrupt through xive directly, we therefore
> have no need to return the trigger page when allocating an interrupt.
>
> It also allows ocxl to use the xive native interface to allocate
> interrupts, instead of its custom service.
>
> Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
> ---
Reviewed-by: Greg Kurz <groug@kaod.org>
> drivers/misc/ocxl/Kconfig | 2 +-
> drivers/misc/ocxl/afu_irq.c | 4 +---
> drivers/misc/ocxl/link.c | 15 +++++++--------
> drivers/scsi/cxlflash/ocxl_hw.c | 3 +--
> include/misc/ocxl.h | 10 ++--------
> 5 files changed, 12 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/misc/ocxl/Kconfig b/drivers/misc/ocxl/Kconfig
> index 2d2266c1439e..e65773f5cf59 100644
> --- a/drivers/misc/ocxl/Kconfig
> +++ b/drivers/misc/ocxl/Kconfig
> @@ -9,7 +9,7 @@ config OCXL_BASE
>
> config OCXL
> tristate "OpenCAPI coherent accelerator support"
> - depends on PPC_POWERNV && PCI && EEH
> + depends on PPC_POWERNV && PCI && EEH && PPC_XIVE_NATIVE
> select OCXL_BASE
> select HOTPLUG_PCI_POWERNV
> default m
> diff --git a/drivers/misc/ocxl/afu_irq.c b/drivers/misc/ocxl/afu_irq.c
> index b30ec0ef7be7..ecdcfae025b7 100644
> --- a/drivers/misc/ocxl/afu_irq.c
> +++ b/drivers/misc/ocxl/afu_irq.c
> @@ -11,7 +11,6 @@ struct afu_irq {
> int hw_irq;
> unsigned int virq;
> char *name;
> - u64 trigger_page;
> irqreturn_t (*handler)(void *private);
> void (*free_private)(void *private);
> void *private;
> @@ -125,8 +124,7 @@ int ocxl_afu_irq_alloc(struct ocxl_context *ctx, int *irq_id)
> goto err_unlock;
> }
>
> - rc = ocxl_link_irq_alloc(ctx->afu->fn->link, &irq->hw_irq,
> - &irq->trigger_page);
> + rc = ocxl_link_irq_alloc(ctx->afu->fn->link, &irq->hw_irq);
> if (rc)
> goto err_idr;
>
> diff --git a/drivers/misc/ocxl/link.c b/drivers/misc/ocxl/link.c
> index 58d111afd9f6..fd73d3bc0eb6 100644
> --- a/drivers/misc/ocxl/link.c
> +++ b/drivers/misc/ocxl/link.c
> @@ -6,6 +6,7 @@
> #include <linux/mmu_context.h>
> #include <asm/copro.h>
> #include <asm/pnv-ocxl.h>
> +#include <asm/xive.h>
> #include <misc/ocxl.h>
> #include "ocxl_internal.h"
> #include "trace.h"
> @@ -682,23 +683,21 @@ int ocxl_link_remove_pe(void *link_handle, int pasid)
> }
> EXPORT_SYMBOL_GPL(ocxl_link_remove_pe);
>
> -int ocxl_link_irq_alloc(void *link_handle, int *hw_irq, u64 *trigger_addr)
> +int ocxl_link_irq_alloc(void *link_handle, int *hw_irq)
> {
> struct ocxl_link *link = (struct ocxl_link *) link_handle;
> - int rc, irq;
> - u64 addr;
> + int irq;
>
> if (atomic_dec_if_positive(&link->irq_available) < 0)
> return -ENOSPC;
>
> - rc = pnv_ocxl_alloc_xive_irq(&irq, &addr);
> - if (rc) {
> + irq = xive_native_alloc_irq();
> + if (!irq) {
> atomic_inc(&link->irq_available);
> - return rc;
> + return -ENXIO;
> }
>
> *hw_irq = irq;
> - *trigger_addr = addr;
> return 0;
> }
> EXPORT_SYMBOL_GPL(ocxl_link_irq_alloc);
> @@ -707,7 +706,7 @@ void ocxl_link_free_irq(void *link_handle, int hw_irq)
> {
> struct ocxl_link *link = (struct ocxl_link *) link_handle;
>
> - pnv_ocxl_free_xive_irq(hw_irq);
> + xive_native_free_irq(hw_irq);
> atomic_inc(&link->irq_available);
> }
> EXPORT_SYMBOL_GPL(ocxl_link_free_irq);
> diff --git a/drivers/scsi/cxlflash/ocxl_hw.c b/drivers/scsi/cxlflash/ocxl_hw.c
> index 59452850f71c..03bff0cae658 100644
> --- a/drivers/scsi/cxlflash/ocxl_hw.c
> +++ b/drivers/scsi/cxlflash/ocxl_hw.c
> @@ -613,7 +613,6 @@ static int alloc_afu_irqs(struct ocxlflash_context *ctx, int num)
> struct ocxl_hw_afu *afu = ctx->hw_afu;
> struct device *dev = afu->dev;
> struct ocxlflash_irqs *irqs;
> - u64 addr;
> int rc = 0;
> int hwirq;
> int i;
> @@ -638,7 +637,7 @@ static int alloc_afu_irqs(struct ocxlflash_context *ctx, int num)
> }
>
> for (i = 0; i < num; i++) {
> - rc = ocxl_link_irq_alloc(afu->link_token, &hwirq, &addr);
> + rc = ocxl_link_irq_alloc(afu->link_token, &hwirq);
> if (unlikely(rc)) {
> dev_err(dev, "%s: ocxl_link_irq_alloc failed rc=%d\n",
> __func__, rc);
> diff --git a/include/misc/ocxl.h b/include/misc/ocxl.h
> index 06dd5839e438..a2868adec22f 100644
> --- a/include/misc/ocxl.h
> +++ b/include/misc/ocxl.h
> @@ -480,14 +480,8 @@ int ocxl_link_remove_pe(void *link_handle, int pasid);
> * Allocate an AFU interrupt associated to the link.
> *
> * 'hw_irq' is the hardware interrupt number
> - * 'obj_handle' is the 64-bit object handle to be passed to the AFU to
> - * trigger the interrupt.
> - * On P9, 'obj_handle' is an address, which, if written, triggers the
> - * interrupt. It is an MMIO address which needs to be remapped (one
> - * page).
> - */
> -int ocxl_link_irq_alloc(void *link_handle, int *hw_irq,
> - u64 *obj_handle);
> + */
> +int ocxl_link_irq_alloc(void *link_handle, int *hw_irq);
>
> /*
> * Free a previously allocated AFU interrupt
^ permalink raw reply
* Re: [PATCH 4/4] ocxl: Remove custom service to allocate interrupts
From: Greg Kurz @ 2020-04-03 9:27 UTC (permalink / raw)
To: Frederic Barrat
Cc: ukrishn, ajd, haren, clg, linuxppc-dev, christophe_lombard,
mrochs
In-Reply-To: <20200402154352.586166-5-fbarrat@linux.ibm.com>
On Thu, 2 Apr 2020 17:43:52 +0200
Frederic Barrat <fbarrat@linux.ibm.com> wrote:
> We now allocate interrupts through xive directly.
>
> Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
> ---
> arch/powerpc/include/asm/pnv-ocxl.h | 3 ---
> arch/powerpc/platforms/powernv/ocxl.c | 30 ---------------------------
Nice diffstat :)
Reviewed-by: Greg Kurz <groug@kaod.org>
> 2 files changed, 33 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/pnv-ocxl.h b/arch/powerpc/include/asm/pnv-ocxl.h
> index 7de82647e761..e90650328c9c 100644
> --- a/arch/powerpc/include/asm/pnv-ocxl.h
> +++ b/arch/powerpc/include/asm/pnv-ocxl.h
> @@ -30,7 +30,4 @@ extern int pnv_ocxl_spa_setup(struct pci_dev *dev, void *spa_mem, int PE_mask,
> extern void pnv_ocxl_spa_release(void *platform_data);
> extern int pnv_ocxl_spa_remove_pe_from_cache(void *platform_data, int pe_handle);
>
> -extern int pnv_ocxl_alloc_xive_irq(u32 *irq, u64 *trigger_addr);
> -extern void pnv_ocxl_free_xive_irq(u32 irq);
> -
> #endif /* _ASM_PNV_OCXL_H */
> diff --git a/arch/powerpc/platforms/powernv/ocxl.c b/arch/powerpc/platforms/powernv/ocxl.c
> index 8c65aacda9c8..ecdad219d704 100644
> --- a/arch/powerpc/platforms/powernv/ocxl.c
> +++ b/arch/powerpc/platforms/powernv/ocxl.c
> @@ -2,7 +2,6 @@
> // Copyright 2017 IBM Corp.
> #include <asm/pnv-ocxl.h>
> #include <asm/opal.h>
> -#include <asm/xive.h>
> #include <misc/ocxl-config.h>
> #include "pci.h"
>
> @@ -484,32 +483,3 @@ int pnv_ocxl_spa_remove_pe_from_cache(void *platform_data, int pe_handle)
> return rc;
> }
> EXPORT_SYMBOL_GPL(pnv_ocxl_spa_remove_pe_from_cache);
> -
> -int pnv_ocxl_alloc_xive_irq(u32 *irq, u64 *trigger_addr)
> -{
> - __be64 flags, trigger_page;
> - s64 rc;
> - u32 hwirq;
> -
> - hwirq = xive_native_alloc_irq();
> - if (!hwirq)
> - return -ENOENT;
> -
> - rc = opal_xive_get_irq_info(hwirq, &flags, NULL, &trigger_page, NULL,
> - NULL);
> - if (rc || !trigger_page) {
> - xive_native_free_irq(hwirq);
> - return -ENOENT;
> - }
> - *irq = hwirq;
> - *trigger_addr = be64_to_cpu(trigger_page);
> - return 0;
> -
> -}
> -EXPORT_SYMBOL_GPL(pnv_ocxl_alloc_xive_irq);
> -
> -void pnv_ocxl_free_xive_irq(u32 irq)
> -{
> - xive_native_free_irq(irq);
> -}
> -EXPORT_SYMBOL_GPL(pnv_ocxl_free_xive_irq);
^ permalink raw reply
* [PATCH] selftests/powerpc: Always build the tm-poison test 64-bit
From: Michael Ellerman @ 2020-04-03 9:56 UTC (permalink / raw)
To: linuxppc-dev; +Cc: gromero
The tm-poison test includes inline asm which is 64-bit only, so the
test must be built 64-bit in order to work.
Otherwise it fails, eg:
# file tm-poison
tm-poison: ELF 32-bit MSB executable, PowerPC or cisco 4500, version 1 (SYSV) ...
# ./tm-poison
test: tm_poison_test
Unknown value 0x1fff71150 leaked into f31!
Unknown value 0x1fff710c0 leaked into vr31!
failure: tm_poison_test
Fixes: a003365cab64 ("powerpc/tm: Add tm-poison test")
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
tools/testing/selftests/powerpc/tm/Makefile | 1 +
1 file changed, 1 insertion(+)
diff --git a/tools/testing/selftests/powerpc/tm/Makefile b/tools/testing/selftests/powerpc/tm/Makefile
index 0b0db8d3857c..5881e97c73c1 100644
--- a/tools/testing/selftests/powerpc/tm/Makefile
+++ b/tools/testing/selftests/powerpc/tm/Makefile
@@ -25,6 +25,7 @@ $(OUTPUT)/tm-unavailable: CFLAGS += -O0 -pthread -m64 -Wno-error=uninitialized -
$(OUTPUT)/tm-trap: CFLAGS += -O0 -pthread -m64
$(OUTPUT)/tm-signal-context-force-tm: CFLAGS += -pthread -m64
$(OUTPUT)/tm-signal-pagefault: CFLAGS += -pthread -m64
+$(OUTPUT)/tm-poison: CFLAGS += -m64
SIGNAL_CONTEXT_CHK_TESTS := $(patsubst %,$(OUTPUT)/%,$(SIGNAL_CONTEXT_CHK_TESTS))
$(SIGNAL_CONTEXT_CHK_TESTS): tm-signal.S
base-commit: c17eb4dca5a353a9dbbb8ad6934fe57af7165e91
--
2.25.1
^ permalink raw reply related
* Re: [PATCH RESEND 1/4] uaccess: Add user_read_access_begin/end and user_write_access_begin/end
From: Russell King - ARM Linux admin @ 2020-04-03 9:49 UTC (permalink / raw)
To: Al Viro
Cc: linux-arch, linuxppc-dev, Kees Cook, Christian Borntraeger,
airlied, hpa, linux-kernel, linux-mm, Paul Mackerras, daniel,
akpm, torvalds
In-Reply-To: <20200403005831.GI23230@ZenIV.linux.org.uk>
On Fri, Apr 03, 2020 at 01:58:31AM +0100, Al Viro wrote:
> On Thu, Apr 02, 2020 at 11:35:57AM -0700, Kees Cook wrote:
>
> > Yup, I think it's a weakness of the ARM implementation and I'd like to
> > not extend it further. AFAIK we should never nest, but I would not be
> > surprised at all if we did.
> >
> > If we were looking at a design goal for all architectures, I'd like
> > to be doing what the public PaX patchset did for their memory access
> > switching, which is to alarm if calling into "enable" found the access
> > already enabled, etc. Such a condition would show an unexpected nesting
> > (like we've seen with similar constructs with set_fs() not getting reset
> > during an exception handler, etc etc).
>
> FWIW, maybe I'm misreading the ARM uaccess logics, but... it smells like
> KERNEL_DS is somewhat more dangerous there than on e.g. x86.
>
> Look: with CONFIG_CPU_DOMAINS, set_fs(KERNEL_DS) tells MMU to ignore
> per-page permission bits in DOMAIN_KERNEL (i.e. for kernel address
> ranges), allowing them even if they would normally be denied. We need
> that for actual uaccess loads/stores, since those use insns that pretend
> to be done in user mode and we want them to access the kernel pages.
> But that affects the normal loads/stores as well; unless I'm misreading
> that code, it will ignore (supervisor) r/o on a page. And that's not
> just for the code inside the uaccess blocks; *everything* done under
> KERNEL_DS is subject to that.
>
> Why do we do that (modify_domain(), that is) inside set_fs() and not
> in uaccess_enable() et.al.?
First, CONFIG_CPU_DOMAINS is used on older ARMs, not ARMv7. Second,
the kernel image itself is not RO-protected on any ARM32 platform.
If we get rid of CONFIG_CPU_DOMAINS, we will use the ARMv7 method of
user access, which is to use normal load/stores for the user accessors
and every access must check against the address limit, even the
__-accessors.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up
^ permalink raw reply
* Re: [PATCH RESEND 1/4] uaccess: Add user_read_access_begin/end and user_write_access_begin/end
From: Russell King - ARM Linux admin @ 2020-04-03 10:02 UTC (permalink / raw)
To: Kees Cook
Cc: linux-arch, linuxppc-dev, Christian Borntraeger, airlied, hpa,
linux-kernel, linux-mm, Paul Mackerras, Al Viro, daniel, akpm,
torvalds
In-Reply-To: <202004021132.813F8E88@keescook>
On Thu, Apr 02, 2020 at 11:35:57AM -0700, Kees Cook wrote:
> On Thu, Apr 02, 2020 at 06:50:32PM +0100, Al Viro wrote:
> > On Thu, Apr 02, 2020 at 07:03:28PM +0200, Christophe Leroy wrote:
> >
> > > user_access_begin() grants both read and write.
> > >
> > > This patch adds user_read_access_begin() and user_write_access_begin() but
> > > it doesn't remove user_access_begin()
> >
> > Ouch... So the most generic name is for the rarest case?
> >
> > > > What should we do about that? Do we prohibit such blocks outside
> > > > of arch?
> > > >
> > > > What should we do about arm and s390? There we want a cookie passed
> > > > from beginning of block to its end; should that be a return value?
> > >
> > > That was the way I implemented it in January, see
> > > https://patchwork.ozlabs.org/patch/1227926/
> > >
> > > There was some discussion around that and most noticeable was:
> > >
> > > H. Peter (hpa) said about it: "I have *deep* concern with carrying state in
> > > a "key" variable: it's a direct attack vector for a crowbar attack,
> > > especially since it is by definition live inside a user access region."
> >
> > > This patch minimises the change by just adding user_read_access_begin() and
> > > user_write_access_begin() keeping the same parameters as the existing
> > > user_access_begin().
> >
> > Umm... What about the arm situation? The same concerns would apply there,
> > wouldn't they? Currently we have
> > static __always_inline unsigned int uaccess_save_and_enable(void)
> > {
> > #ifdef CONFIG_CPU_SW_DOMAIN_PAN
> > unsigned int old_domain = get_domain();
> >
> > /* Set the current domain access to permit user accesses */
> > set_domain((old_domain & ~domain_mask(DOMAIN_USER)) |
> > domain_val(DOMAIN_USER, DOMAIN_CLIENT));
> >
> > return old_domain;
> > #else
> > return 0;
> > #endif
> > }
> > and
> > static __always_inline void uaccess_restore(unsigned int flags)
> > {
> > #ifdef CONFIG_CPU_SW_DOMAIN_PAN
> > /* Restore the user access mask */
> > set_domain(flags);
> > #endif
> > }
> >
> > How much do we need nesting on those, anyway? rmk?
It's that way because it's easy, logical, and actually *more* efficient
to do it that way, rather than read-modify-write the domain register
each time we want to change it.
> Yup, I think it's a weakness of the ARM implementation and I'd like to
> not extend it further. AFAIK we should never nest, but I would not be
> surprised at all if we did.
There is one known nesting, which is __clear_user() when used with
the (IMHO horrid and I don't care about) UACCESS_WITH_MEMCPY feature.
That's not intentional however.
When I introduced this on ARM, the placement I adopted was to locate
it _as close as sanely possible_ to the userspace access so we
minimised the kernel accesses, so we minimise the number of accesses
that could go stray because of the domain issue - we ideally only
want the access done by the accessor itself to be affected, which
we achieve for most accesses.
Thinking laterally, maybe we should get rid of the whole KERNEL_DS
stuff entirely, and come up with an alternative way of handling the
kernel-wants-to-access-kernelspace-via-user-accessors problem.
Such as, copying some data back to userspace memory?
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up
^ permalink raw reply
* Re: [PATCH v8 2/7] powerpc/kprobes: Mark newly allocated probes as RO
From: Naveen N. Rao @ 2020-04-03 10:03 UTC (permalink / raw)
To: linuxppc-dev, Russell Currey; +Cc: kernel-hardening, ajd, npiggin, dja
In-Reply-To: <02c6c3d0483e217a6d879bb7037f0b549c64ba04.camel@russell.cc>
Russell Currey wrote:
> On Fri, 2020-04-03 at 15:06 +0530, Naveen N. Rao wrote:
>> Russell Currey wrote:
>> > On Fri, 2020-04-03 at 00:18 +0530, Naveen N. Rao wrote:
>> > > Naveen N. Rao wrote:
>> > > > Russell Currey wrote:
>> > > > >
>> > > > > +void *alloc_insn_page(void)
>> > > > > +{
>> > > > > + void *page = vmalloc_exec(PAGE_SIZE);
>> > > > > +
>> > > > > + if (page)
>> > > > > + set_memory_ro((unsigned long)page, 1);
>> > > > > +
>> > > > > + return page;
>> > > > > +}
>> > > > > +
>> > > >
>> > > > This crashes for me with KPROBES_SANITY_TEST during the
>> > > > kretprobe
>> > > > test.
>> > >
>> > > That isn't needed to reproduce this. After bootup, disabling
>> > > optprobes
>> > > also shows the crash with kretprobes:
>> > > sysctl debug.kprobes-optimization=0
>> > >
>> > > The problem happens to be with patch_instruction() in
>> > > arch_prepare_kprobe(). During boot, on kprobe init, we register a
>> > > probe
>> > > on kretprobe_trampoline for use with kretprobes (see
>> > > arch_init_kprobes()). This results in an instruction slot being
>> > > allocated, and arch_prepare_kprobe() to be called for copying
>> > > the
>> > > instruction (nop) at kretprobe_trampoline. patch_instruction()
>> > > is
>> > > failing resulting in corrupt instruction which we try to
>> > > emulate/single
>> > > step causing the crash.
>> >
>> > OK I think I've fixed it, KPROBES_SANITY_TEST passes too. I'd
>> > appreciate it if you could test v9, and thanks again for finding
>> > this -
>> > very embarrassing bug on my side.
>>
>> Great! Thanks.
>>
>> I think I should also add appropriate error checking to kprobes' use
>> of
>> patch_instruction() which would have caught this much more easily.
>
> Only kind of! It turns out that if the initial setup fails for
> KPROBES_SANITY_TEST, it silently doesn't run - so you miss the "Kprobe
> smoke test" text, but you don't get any kind of error either. I'll
> send a patch so that it fails more loudly later.
Ha, I see what you mean. Good catch, we should pass the kprobe init
status to the test and have it error out.
>
>>
>> On a related note, I notice that x86 seems to prefer not having any
>> RWX
>> pages, and so they continue to do 'module_alloc()' followed by
>> 'set_memory_ro()' and then 'set_memory_x()'. Is that something worth
>> following for powerpc?
>
> I just noticed that too. arm64 doesn't set theirs executable, as far
> as I can tell powerpc doesn't need to.
I didn't follow that. We do need it to be executable so that we can
single step the original instruction.
arm64 does vmalloc_exec(), which looks like it sets the page to RWX,
then marks it RO. There is a small window where the page would be WX.
x86 instead seems to first allocate the page as RW, mark as RO, and only
then enable X - removing that small window where the page is both W and
X.
- Naveen
^ permalink raw reply
* Re: [PATCH kernel] powerpc/pseries/ddw: Extend upper limit for huge DMA window for persistent memory
From: Vaibhav Jain @ 2020-04-03 10:04 UTC (permalink / raw)
To: Alexey Kardashevskiy, linuxppc-dev
Cc: Brian J King, Alexey Kardashevskiy, Aneesh Kumar K . V,
Oliver O'Halloran, David Gibson, Wen Xiong
In-Reply-To: <20200331012338.23773-1-aik@ozlabs.ru>
Alexey Kardashevskiy <aik@ozlabs.ru> writes:
> Unlike normal memory ("memory" compatible type in the FDT),
> the persistent memory ("ibm,pmemory" in the FDT) can be mapped anywhere
> in the guest physical space and it can be used for DMA.
>
> In order to maintain 1:1 mapping via the huge DMA window, we need to
> know the maximum physical address at the time of the window setup.
> So far we've been looking at "memory" nodes but "ibm,pmemory" does not
> have fixed addresses and the persistent memory may be mapped afterwards.
>
> Since the persistent memory is still backed with page structs,
> use MAX_PHYSMEM_BITS as the upper limit.
>
> This effectively disables huge DMA window in LPAR under pHyp if
> persistent memory is present but this is the best we can do.
>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Thanks for the patch Alexy,
LGTM.
Reviewed-by: Vaibhav Jain <vaibhav@linux.ibm.com>
> ---
> arch/powerpc/platforms/pseries/iommu.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
> index 2e0a8eab5588..6d47b4a3ce39 100644
> --- a/arch/powerpc/platforms/pseries/iommu.c
> +++ b/arch/powerpc/platforms/pseries/iommu.c
> @@ -945,6 +945,15 @@ static phys_addr_t ddw_memory_hotplug_max(void)
> phys_addr_t max_addr = memory_hotplug_max();
> struct device_node *memory;
>
> + /*
> + * The "ibm,pmemory" can appear anywhere in the address space.
> + * Assuming it is still backed by page structs, set the upper limit
> + * for the huge DMA window as MAX_PHYSMEM_BITS.
> + */
> + if (of_find_node_by_type(NULL, "ibm,pmemory"))
> + return (sizeof(phys_addr_t) * 8 <= MAX_PHYSMEM_BITS) ?
> + (phys_addr_t) -1 : (1ULL << MAX_PHYSMEM_BITS);
> +
> for_each_node_by_type(memory, "memory") {
> unsigned long start, size;
> int n_mem_addr_cells, n_mem_size_cells, len;
> --
> 2.17.1
>
--
Vaibhav Jain <vaibhav@linux.ibm.com>
Linux Technology Center, IBM India Pvt. Ltd.
^ permalink raw reply
* Re: [PATCH v2 08/12] powerpc/pseries: limit machine check stack to 4GB
From: Nicholas Piggin @ 2020-04-03 10:24 UTC (permalink / raw)
To: mahesh; +Cc: linuxppc-dev, Ganesh Goudar, Mahesh Salgaonkar
In-Reply-To: <20200327052430.r3id43ptr6ffwtoq@in.ibm.com>
Mahesh J Salgaonkar's on March 27, 2020 3:24 pm:
> On 2020-03-25 20:34:06 Wed, Nicholas Piggin wrote:
>> This allows rtas_args to be put on the machine check stack, which
>> avoids a lot of complications with re-entrancy deadlocks.
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>> arch/powerpc/kernel/setup_64.c | 15 ++++++++++++++-
>> 1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
>> index 3bf03666ee09..ca1041f8a578 100644
>> --- a/arch/powerpc/kernel/setup_64.c
>> +++ b/arch/powerpc/kernel/setup_64.c
>> @@ -695,6 +695,9 @@ void __init exc_lvl_early_init(void)
>> void __init emergency_stack_init(void)
>> {
>> u64 limit;
>> +#ifdef CONFIG_PPC_BOOK3S_64
>> + u64 mce_limit;
>> +#endif
>> unsigned int i;
>>
>> /*
>> @@ -713,6 +716,16 @@ void __init emergency_stack_init(void)
>> */
>> limit = min(ppc64_bolted_size(), ppc64_rma_size);
>>
>> + /*
>> + * Machine check on pseries calls rtas, but can't use the static
>> + * rtas_args due to a machine check hitting while the lock is held.
>> + * rtas args have to be under 4GB, so the machine check stack is
>> + * limited to 4GB so args can be put on stack.
>> + */
>> + mce_limit = limit;
>> + if (firmware_has_feature(FW_FEATURE_LPAR) && mce_limit > 4UL*1024*1024*1024)
>> + mce_limit = 4UL*1024*1024*1024;
>> +
>
> Don't you need this as well under CONFIG_PPC_BOOK3S_64 #ifdef ??
Good catch.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH v2 1/4] powerpc/64s: implement probe_kernel_read/write without touching AMR
From: Christophe Leroy @ 2020-04-03 10:31 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev
In-Reply-To: <20200403093529.43587-1-npiggin@gmail.com>
Le 03/04/2020 à 11:35, Nicholas Piggin a écrit :
> There is no need to allow user accesses when probing kernel addresses.
I just discovered the following commit
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=75a1a607bb7e6d918be3aca11ec2214a275392f4
This commit adds probe_kernel_read_strict() and probe_kernel_write_strict().
When reading the commit log, I understand that probe_kernel_read() may
be used to access some user memory. Which will not work anymore with
your patch.
Isn't it probe_kernel_read_strict() and probe_kernel_write_strict() that
you want to add ?
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> v2:
> - Enable for all powerpc (suggested by Christophe)
> - Fold helper function together (Christophe)
> - Rename uaccess.c to maccess.c to match kernel/maccess.c.
>
> arch/powerpc/include/asm/uaccess.h | 25 +++++++++++++++-------
> arch/powerpc/lib/Makefile | 2 +-
> arch/powerpc/lib/maccess.c | 34 ++++++++++++++++++++++++++++++
x86 does it in mm/maccess.c
> 3 files changed, 52 insertions(+), 9 deletions(-)
> create mode 100644 arch/powerpc/lib/maccess.c
>
Christophe
^ permalink raw reply
* Re: [PATCH v4 2/6] powerpc/idle: Add accessor function to always read latest idle PURR
From: Naveen N. Rao @ 2020-04-03 10:34 UTC (permalink / raw)
To: ego
Cc: Nathan Lynch, Tyrel Datwyler, linux-kernel, Kamalesh Babulal,
Vaidyanathan Srinivasan, linuxppc-dev
In-Reply-To: <20200403061536.GA9066@in.ibm.com>
Gautham R Shenoy wrote:
> On Wed, Apr 01, 2020 at 03:12:53PM +0530, Naveen N. Rao wrote:
>> Hi Gautham,
>>
>> Gautham R. Shenoy wrote:
>> >From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>
>> >
>> >+
>> >+static inline u64 read_this_idle_purr(void)
>> >+{
>> >+ /*
>> >+ * If we are reading from an idle context, update the
>> >+ * idle-purr cycles corresponding to the last idle period.
>> >+ * Since the idle context is not yet over, take a fresh
>> >+ * snapshot of the idle-purr.
>> >+ */
>> >+ if (unlikely(get_lppaca()->idle == 1)) {
>> >+ update_idle_purr_accounting();
>> >+ snapshot_purr_idle_entry();
>> >+ }
>> >+
>> >+ return be64_to_cpu(get_lppaca()->wait_state_cycles);
>> >+}
>> >+
>>
>> I think this and read_this_idle_spurr() from the next patch should be moved
>> to Patch 4/6, where they are actually used.
>
> The reason I included this function in this patch was to justify why
> we were introducing snapshotting the purr values in a global per-cpu
> variable instead of on a stack variable. The reason being that someone
> might want to read the PURR value from an interrupt context which had
> woken up the CPU from idle. At this point, since epilog() function
> wasn't called, the idle PURR count corresponding to this latest idle
> period would have been accumulated in lppaca->wait_cycles. Thus, this
> helper function safely reads the value by
> 1) First updating the lppaca->wait_cycles with the latest idle_purr
> count.
> 2) Take a fresh snapshot, since the time from now to the epilog()
> call is also counted under idle CPU. So the PURR cycle increment
> during this short period should also be accumulated in lppaca->wait_cycles.
>
>
> prolog()
> | snapshot PURR
> |
> |
> |
> Idle
> |
> | <----- Interrupt . Read idle PURR ---- update idle PURR;
> | snapshot PURR;
> | Read idle PURR.
> |
> epilog()
> update idle PURR
>
Yes, I understand. It makes sense.
>
> However, if you feel that moving this function to Patch 4 where it is
> actually used makes it more readable, I can do that.
My suggestion was from a bisectability standpoint though. This is a
fairly simple function, but it is generally recommended to ensure that
newly added code gets exercized in the patch that it is introduced in:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/5.Posting.rst#n119
Regards,
Naveen
^ permalink raw reply
* Re: [PATCH v2 4/4] powerpc/uaccess: add more __builtin_expect annotations
From: Nicholas Piggin @ 2020-04-03 10:35 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <20200403093529.43587-4-npiggin@gmail.com>
Nicholas Piggin's on April 3, 2020 7:35 pm:
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Sorry that was a bad rebase, here's the fixed patch.
---
arch/powerpc/include/asm/uaccess.h | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index 144d01645d68..8a0474682c9b 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -48,16 +48,16 @@ static inline void set_fs(mm_segment_t fs)
* gap between user addresses and the kernel addresses
*/
#define __access_ok(addr, size, segment) \
- (((addr) <= (segment).seg) && ((size) <= (segment).seg))
+ likely(((addr) <= (segment).seg) && ((size) <= (segment).seg))
#else
static inline int __access_ok(unsigned long addr, unsigned long size,
mm_segment_t seg)
{
- if (addr > seg.seg)
+ if (unlikely(addr > seg.seg))
return 0;
- return (size == 0 || size - 1 <= seg.seg - addr);
+ return likely(size == 0 || size - 1 <= seg.seg - addr);
}
#endif
@@ -177,7 +177,7 @@ do { \
else \
__put_user_size_allowed(__pu_val, __pu_addr, __pu_size, __pu_err); \
\
- __pu_err; \
+ __builtin_expect(__pu_err, 0); \
})
#define __put_user_check(x, ptr, size) \
@@ -191,7 +191,7 @@ do { \
if (access_ok(__pu_addr, __pu_size)) \
__put_user_size(__pu_val, __pu_addr, __pu_size, __pu_err); \
\
- __pu_err; \
+ __builtin_expect(__pu_err, 0); \
})
#define __put_user_nosleep(x, ptr, size) \
@@ -204,7 +204,7 @@ do { \
__chk_user_ptr(__pu_addr); \
__put_user_size(__pu_val, __pu_addr, __pu_size, __pu_err); \
\
- __pu_err; \
+ __builtin_expect(__pu_err, 0); \
})
@@ -307,7 +307,7 @@ do { \
__get_user_size_allowed(__gu_val, __gu_addr, __gu_size, __gu_err); \
(x) = (__typeof__(*(ptr)))__gu_val; \
\
- __gu_err; \
+ __builtin_expect(__gu_err, 0); \
})
#define __get_user_check(x, ptr, size) \
@@ -324,7 +324,7 @@ do { \
} \
(x) = (__force __typeof__(*(ptr)))__gu_val; \
\
- __gu_err; \
+ __builtin_expect(__gu_err, 0); \
})
#define __get_user_nosleep(x, ptr, size) \
@@ -339,7 +339,7 @@ do { \
__get_user_size(__gu_val, __gu_addr, __gu_size, __gu_err); \
(x) = (__force __typeof__(*(ptr)))__gu_val; \
\
- __gu_err; \
+ __builtin_expect(__gu_err, 0); \
})
--
2.23.0
^ permalink raw reply related
* [PATCH] qbman: Remove set but not used variable 'err'
From: Zheng Yongjun @ 2020-04-03 7:35 UTC (permalink / raw)
To: leoyang.li, linuxppc-dev; +Cc: Zheng Yongjun, network
From: network <network@ubuntu.network>
Fixes gcc '-Wunused-but-set-variable' warning:
drivers/soc/fsl/qbman/bman.c:640:6: warning: variable ‘err’ set but not used [-Wunused-but-set-variable]
int err = 0;
^~~
err is never used, so remove it.
Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: Zheng Yongjun <zhengyongjun3@huawei.com>
Signed-off-by: network <network@ubuntu.network>
---
drivers/soc/fsl/qbman/bman.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/soc/fsl/qbman/bman.c b/drivers/soc/fsl/qbman/bman.c
index f4fb527..0a24433 100644
--- a/drivers/soc/fsl/qbman/bman.c
+++ b/drivers/soc/fsl/qbman/bman.c
@@ -637,7 +637,6 @@ int bman_p_irqsource_add(struct bman_portal *p, u32 bits)
int bm_shutdown_pool(u32 bpid)
{
- int err = 0;
struct bm_mc_command *bm_cmd;
union bm_mc_result *bm_res;
@@ -650,7 +649,6 @@ int bm_shutdown_pool(u32 bpid)
bm_mc_commit(&p->p, BM_MCC_VERB_CMD_ACQUIRE | 1);
if (!bm_mc_result_timeout(&p->p, &bm_res)) {
pr_crit("BMan Acquire Command timedout\n");
- err = -ETIMEDOUT;
goto done;
}
if (!(bm_res->verb & BM_MCR_VERB_ACQUIRE_BUFCOUNT)) {
--
2.7.4
^ permalink raw reply related
* Re: [PATCH v11 3/8] powerpc/perf: consolidate read_user_stack_32
From: Michal Suchánek @ 2020-04-03 10:52 UTC (permalink / raw)
To: Nicholas Piggin
Cc: Mark Rutland, Gustavo Luiz Duarte, Alexander Shishkin,
Sebastian Andrzej Siewior, linux-kernel, Paul Mackerras,
Jiri Olsa, Rob Herring, Michael Neuling, Eric Richter,
Masahiro Yamada, Nayna Jain, Peter Zijlstra, Ingo Molnar,
Hari Bathini, Jordan Niethe, Valentin Schneider, Arnd Bergmann,
Arnaldo Carvalho de Melo, Alexander Viro, Jonathan Cameron,
Namhyung Kim, Thomas Gleixner, Andy Shevchenko, Allison Randal,
Greg Kroah-Hartman, Claudio Carvalho, Mauro Carvalho Chehab,
Eric W. Biederman, linux-fsdevel, linuxppc-dev, David S. Miller,
Thiago Jung Bauermann
In-Reply-To: <1585896170.ohti800w9v.astroid@bobo.none>
Hello,
there are 3 variants of the function
read_user_stack_64
32bit read_user_stack_32
64bit read_user_Stack_32
On Fri, Apr 03, 2020 at 05:13:25PM +1000, Nicholas Piggin wrote:
> Michal Suchánek's on March 25, 2020 5:38 am:
> > On Tue, Mar 24, 2020 at 06:48:20PM +1000, Nicholas Piggin wrote:
> >> Michal Suchanek's on March 19, 2020 10:19 pm:
> >> > There are two almost identical copies for 32bit and 64bit.
> >> >
> >> > The function is used only in 32bit code which will be split out in next
> >> > patch so consolidate to one function.
> >> >
> >> > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> >> > Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr>
> >> > ---
> >> > v6: new patch
> >> > v8: move the consolidated function out of the ifdef block.
> >> > v11: rebase on top of def0bfdbd603
> >> > ---
> >> > arch/powerpc/perf/callchain.c | 48 +++++++++++++++++------------------
> >> > 1 file changed, 24 insertions(+), 24 deletions(-)
> >> >
> >> > diff --git a/arch/powerpc/perf/callchain.c b/arch/powerpc/perf/callchain.c
> >> > index cbc251981209..c9a78c6e4361 100644
> >> > --- a/arch/powerpc/perf/callchain.c
> >> > +++ b/arch/powerpc/perf/callchain.c
> >> > @@ -161,18 +161,6 @@ static int read_user_stack_64(unsigned long __user *ptr, unsigned long *ret)
> >> > return read_user_stack_slow(ptr, ret, 8);
> >> > }
> >> >
> >> > -static int read_user_stack_32(unsigned int __user *ptr, unsigned int *ret)
> >> > -{
> >> > - if ((unsigned long)ptr > TASK_SIZE - sizeof(unsigned int) ||
> >> > - ((unsigned long)ptr & 3))
> >> > - return -EFAULT;
> >> > -
> >> > - if (!probe_user_read(ret, ptr, sizeof(*ret)))
> >> > - return 0;
> >> > -
> >> > - return read_user_stack_slow(ptr, ret, 4);
> >> > -}
> >> > -
> >> > static inline int valid_user_sp(unsigned long sp, int is_64)
> >> > {
> >> > if (!sp || (sp & 7) || sp > (is_64 ? TASK_SIZE : 0x100000000UL) - 32)
> >> > @@ -277,19 +265,9 @@ static void perf_callchain_user_64(struct perf_callchain_entry_ctx *entry,
> >> > }
> >> >
> >> > #else /* CONFIG_PPC64 */
> >> > -/*
> >> > - * On 32-bit we just access the address and let hash_page create a
> >> > - * HPTE if necessary, so there is no need to fall back to reading
> >> > - * the page tables. Since this is called at interrupt level,
> >> > - * do_page_fault() won't treat a DSI as a page fault.
> >> > - */
> >> > -static int read_user_stack_32(unsigned int __user *ptr, unsigned int *ret)
> >> > +static int read_user_stack_slow(void __user *ptr, void *buf, int nb)
> >> > {
> >> > - if ((unsigned long)ptr > TASK_SIZE - sizeof(unsigned int) ||
> >> > - ((unsigned long)ptr & 3))
> >> > - return -EFAULT;
> >> > -
> >> > - return probe_user_read(ret, ptr, sizeof(*ret));
> >> > + return 0;
> >> > }
> >> >
> >> > static inline void perf_callchain_user_64(struct perf_callchain_entry_ctx *entry,
> >> > @@ -312,6 +290,28 @@ static inline int valid_user_sp(unsigned long sp, int is_64)
> >> >
> >> > #endif /* CONFIG_PPC64 */
> >> >
> >> > +/*
> >> > + * On 32-bit we just access the address and let hash_page create a
> >> > + * HPTE if necessary, so there is no need to fall back to reading
> >> > + * the page tables. Since this is called at interrupt level,
> >> > + * do_page_fault() won't treat a DSI as a page fault.
> >> > + */
> >>
> >> The comment is actually probably better to stay in the 32-bit
> >> read_user_stack_slow implementation. Is that function defined
> >> on 32-bit purely so that you can use IS_ENABLED()? In that case
> > It documents the IS_ENABLED() and that's where it is. The 32bit
> > definition is only a technical detail.
>
> Sorry for the late reply, busy trying to fix bugs in the C rewrite
> series. I don't think it is the right place, it should be in the
> ppc32 implementation detail. ppc64 has an equivalent comment at the
> top of its read_user_stack functions.
>
> >> I would prefer to put a BUG() there which makes it self documenting.
> > Which will cause checkpatch complaints about introducing new BUG() which
> > is frowned on.
>
> It's fine in this case, that warning is about not introducing
> runtime bugs, but this wouldn't be.
>
> But... I actually don't like adding read_user_stack_slow on 32-bit
> and especially not just to make IS_ENABLED work.
>
> IMO this would be better if you really want to consolidate it
>
> ---
>
> diff --git a/arch/powerpc/perf/callchain.c b/arch/powerpc/perf/callchain.c
> index cbc251981209..ca3a599b3f54 100644
> --- a/arch/powerpc/perf/callchain.c
> +++ b/arch/powerpc/perf/callchain.c
> @@ -108,7 +108,7 @@ perf_callchain_kernel(struct perf_callchain_entry_ctx *entry, struct pt_regs *re
> * interrupt context, so if the access faults, we read the page tables
> * to find which page (if any) is mapped and access it directly.
> */
> -static int read_user_stack_slow(void __user *ptr, void *buf, int nb)
> +static int read_user_stack_slow(const void __user *ptr, void *buf, int nb)
> {
> int ret = -EFAULT;
> pgd_t *pgdir;
> @@ -149,28 +149,21 @@ static int read_user_stack_slow(void __user *ptr, void *buf, int nb)
> return ret;
> }
>
> -static int read_user_stack_64(unsigned long __user *ptr, unsigned long *ret)
> +static int __read_user_stack(const void __user *ptr, void *ret, size_t size)
> {
> - if ((unsigned long)ptr > TASK_SIZE - sizeof(unsigned long) ||
> - ((unsigned long)ptr & 7))
> + if ((unsigned long)ptr > TASK_SIZE - size ||
> + ((unsigned long)ptr & (size - 1)))
> return -EFAULT;
>
> - if (!probe_user_read(ret, ptr, sizeof(*ret)))
> + if (!probe_user_read(ret, ptr, size))
> return 0;
>
> - return read_user_stack_slow(ptr, ret, 8);
> + return read_user_stack_slow(ptr, ret, size);
> }
>
> -static int read_user_stack_32(unsigned int __user *ptr, unsigned int *ret)
> +static int read_user_stack_64(unsigned long __user *ptr, unsigned long *ret)
> {
> - if ((unsigned long)ptr > TASK_SIZE - sizeof(unsigned int) ||
> - ((unsigned long)ptr & 3))
> - return -EFAULT;
> -
> - if (!probe_user_read(ret, ptr, sizeof(*ret)))
> - return 0;
> -
> - return read_user_stack_slow(ptr, ret, 4);
> + return __read_user_stack(ptr, ret, sizeof(*ret));
> }
>
> static inline int valid_user_sp(unsigned long sp, int is_64)
> @@ -283,13 +276,13 @@ static void perf_callchain_user_64(struct perf_callchain_entry_ctx *entry,
> * the page tables. Since this is called at interrupt level,
> * do_page_fault() won't treat a DSI as a page fault.
> */
> -static int read_user_stack_32(unsigned int __user *ptr, unsigned int *ret)
> +static int __read_user_stack(const void __user *ptr, void *ret, size_t size)
> {
> - if ((unsigned long)ptr > TASK_SIZE - sizeof(unsigned int) ||
> - ((unsigned long)ptr & 3))
> + if ((unsigned long)ptr > TASK_SIZE - size ||
> + ((unsigned long)ptr & (size - 1)))
> return -EFAULT;
>
> - return probe_user_read(ret, ptr, sizeof(*ret));
> + return probe_user_read(ret, ptr, size);
> }
>
> static inline void perf_callchain_user_64(struct perf_callchain_entry_ctx *entry,
> @@ -312,6 +305,11 @@ static inline int valid_user_sp(unsigned long sp, int is_64)
>
> #endif /* CONFIG_PPC64 */
>
> +static int read_user_stack_32(unsigned int __user *ptr, unsigned int *ret)
> +{
> + return __read_user_stack(ptr, ret, sizeof(*ret));
Does not work for 64bit read_user_stack_32 ^ this should be 4.
Other than that it should preserve the existing logic just fine.
Thanks
Michal
^ 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