[PATCH v2] powerpc: Add force enable of DAWR on P9 option

[PATCH v2] powerpc: Add force enable of DAWR on P9 option

[Michael Neuling]

This adds a flag so that the DAWR can be enabled on P9 via:
  echo Y > /sys/kernel/debug/powerpc/dawr_enable_dangerous

The DAWR was previously force disabled on POWER9 in:
  9654153158 powerpc: Disable DAWR in the base POWER9 CPU features
Also see Documentation/powerpc/DAWR-POWER9.txt

This is a dangerous setting, USE AT YOUR OWN RISK.

Some users may not care about a bad user crashing their box
(ie. single user/desktop systems) and really want the DAWR.  This
allows them to force enable DAWR.

This flag can also be used to disable DAWR access. Once this is
cleared, all DAWR access should be cleared immediately and your
machine once again safe from crashing.

Userspace may get confused by toggling this. If DAWR is force
enabled/disabled between getting the number of breakpoints (via
PTRACE_GETHWDBGINFO) and setting the breakpoint, userspace will get an
inconsistent view of what's available. Similarly for guests.

For the DAWR to be enabled in a KVM guest, the DAWR needs to be force
enabled in the host AND the guest. For this reason, this won't work on
POWERVM as it doesn't allow the HCALL to work. Writes of 'Y' to the
dawr_enable_dangerous file will fail if the hypervisor doesn't support
writing the DAWR.

To double check the DAWR is working, run this kernel selftest:
  tools/testing/selftests/powerpc/ptrace/ptrace-hwbreak.c
Any errors/failures/skips mean something is wrong.

Signed-off-by: Michael Neuling <mikey@neuling.org>
---
v2:
  Fix compile errors found by kbuild test robot.
---
 Documentation/powerpc/DAWR-POWER9.txt    | 32 ++++++++++++
 arch/powerpc/include/asm/hw_breakpoint.h |  8 +++
 arch/powerpc/kernel/hw_breakpoint.c      | 62 +++++++++++++++++++++++-
 arch/powerpc/kernel/process.c            |  9 ++--
 arch/powerpc/kernel/ptrace.c             |  3 +-
 arch/powerpc/kvm/book3s_hv.c             |  3 +-
 arch/powerpc/kvm/book3s_hv_rmhandlers.S  | 23 +++++----
 7 files changed, 123 insertions(+), 17 deletions(-)

diff --git a/Documentation/powerpc/DAWR-POWER9.txt b/Documentation/powerpc/DAWR-POWER9.txt
index 2feaa66196..bdec036509 100644
--- a/Documentation/powerpc/DAWR-POWER9.txt
+++ b/Documentation/powerpc/DAWR-POWER9.txt
@@ -56,3 +56,35 @@ POWER9. Loads and stores to the watchpoint locations will not be
 trapped in GDB. The watchpoint is remembered, so if the guest is
 migrated back to the POWER8 host, it will start working again.
 
+Force enabling the DAWR
+=============================
+Kernels (since ~v5.2) have an option to force enable the DAWR via:
+
+  echo Y > /sys/kernel/debug/powerpc/dawr_enable_dangerous
+
+This enables the DAWR even on POWER9.
+
+This is a dangerous setting, USE AT YOUR OWN RISK.
+
+Some users may not care about a bad user crashing their box
+(ie. single user/desktop systems) and really want the DAWR.  This
+allows them to force enable DAWR.
+
+This flag can also be used to disable DAWR access. Once this is
+cleared, all DAWR access should be cleared immediately and your
+machine once again safe from crashing.
+
+Userspace may get confused by toggling this. If DAWR is force
+enabled/disabled between getting the number of breakpoints (via
+PTRACE_GETHWDBGINFO) and setting the breakpoint, userspace will get an
+inconsistent view of what's available. Similarly for guests.
+
+For the DAWR to be enabled in a KVM guest, the DAWR needs to be force
+enabled in the host AND the guest. For this reason, this won't work on
+POWERVM as it doesn't allow the HCALL to work. Writes of 'Y' to the
+dawr_enable_dangerous file will fail if the hypervisor doesn't support
+writing the DAWR.
+
+To double check the DAWR is working, run this kernel selftest:
+  tools/testing/selftests/powerpc/ptrace/ptrace-hwbreak.c
+Any errors/failures/skips mean something is wrong.
diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h
index ece4dc89c9..0fe8c1e46b 100644
--- a/arch/powerpc/include/asm/hw_breakpoint.h
+++ b/arch/powerpc/include/asm/hw_breakpoint.h
@@ -90,10 +90,18 @@ static inline void hw_breakpoint_disable(void)
 extern void thread_change_pc(struct task_struct *tsk, struct pt_regs *regs);
 int hw_breakpoint_handler(struct die_args *args);
 
+extern int set_dawr(struct arch_hw_breakpoint *brk);
+extern bool dawr_force_enable;
+static inline bool dawr_enabled(void)
+{
+	return dawr_force_enable;
+}
+
 #else	/* CONFIG_HAVE_HW_BREAKPOINT */
 static inline void hw_breakpoint_disable(void) { }
 static inline void thread_change_pc(struct task_struct *tsk,
 					struct pt_regs *regs) { }
+static inline bool dawr_enabled(void) { return false; }
 #endif	/* CONFIG_HAVE_HW_BREAKPOINT */
 #endif	/* __KERNEL__ */
 #endif	/* _PPC_BOOK3S_64_HW_BREAKPOINT_H */
diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
index fec8a67731..da307dd93e 100644
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -29,11 +29,15 @@
 #include <linux/kernel.h>
 #include <linux/sched.h>
 #include <linux/smp.h>
+#include <linux/debugfs.h>
+#include <linux/init.h>
 
 #include <asm/hw_breakpoint.h>
 #include <asm/processor.h>
 #include <asm/sstep.h>
 #include <asm/debug.h>
+#include <asm/debugfs.h>
+#include <asm/hvcall.h>
 #include <linux/uaccess.h>
 
 /*
@@ -174,7 +178,7 @@ int hw_breakpoint_arch_parse(struct perf_event *bp,
 	if (!ppc_breakpoint_available())
 		return -ENODEV;
 	length_max = 8; /* DABR */
-	if (cpu_has_feature(CPU_FTR_DAWR)) {
+	if (dawr_enabled()) {
 		length_max = 512 ; /* 64 doublewords */
 		/* DAWR region can't cross 512 boundary */
 		if ((attr->bp_addr >> 9) !=
@@ -376,3 +380,59 @@ void hw_breakpoint_pmu_read(struct perf_event *bp)
 {
 	/* TODO */
 }
+
+bool dawr_force_enable;
+EXPORT_SYMBOL_GPL(dawr_force_enable);
+
+static ssize_t dawr_write_file_bool(struct file *file,
+				    const char __user *user_buf,
+				    size_t count, loff_t *ppos)
+{
+	struct arch_hw_breakpoint null_brk = {0, 0, 0};
+	size_t rc;
+
+	/* Send error to user if they hypervisor won't allow us to write DAWR */
+	if ((!dawr_force_enable) &&
+	    (firmware_has_feature(FW_FEATURE_LPAR)) &&
+	    (set_dawr(&null_brk) != H_SUCCESS))
+		return -1;
+
+	rc = debugfs_write_file_bool(file, user_buf, count, ppos);
+	if (rc)
+		return rc;
+
+	/* If we are clearing, make sure all CPUs have the DAWR cleared */
+	if (!dawr_force_enable)
+		smp_call_function((smp_call_func_t)set_dawr, &null_brk, 0);
+
+	return rc;
+}
+
+static const struct file_operations dawr_enable_fops = {
+	.read =		debugfs_read_file_bool,
+	.write =	dawr_write_file_bool,
+	.open =		simple_open,
+	.llseek =	default_llseek,
+};
+
+static int __init dawr_force_setup(void)
+{
+	dawr_force_enable = false;
+
+	if (cpu_has_feature(CPU_FTR_DAWR)) {
+		/* Don't setup sysfs file for user control on P8 */
+		dawr_force_enable = true;
+		return 0;
+	}
+
+	if (PVR_VER(mfspr(SPRN_PVR)) == PVR_POWER9) {
+		/* Turn DAWR off by default, but allow admin to turn it on */
+		dawr_force_enable = false;
+		debugfs_create_file_unsafe("dawr_enable_dangerous", 0600,
+					   powerpc_debugfs_root,
+					   &dawr_force_enable,
+					   &dawr_enable_fops);
+	}
+	return 0;
+}
+arch_initcall(dawr_force_setup);
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index dd9e0d5386..225705aac8 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -67,6 +67,7 @@
 #include <asm/cpu_has_feature.h>
 #include <asm/asm-prototypes.h>
 #include <asm/stacktrace.h>
+#include <asm/hw_breakpoint.h>
 
 #include <linux/kprobes.h>
 #include <linux/kdebug.h>
@@ -784,7 +785,7 @@ static inline int set_dabr(struct arch_hw_breakpoint *brk)
 	return __set_dabr(dabr, dabrx);
 }
 
-static inline int set_dawr(struct arch_hw_breakpoint *brk)
+int set_dawr(struct arch_hw_breakpoint *brk)
 {
 	unsigned long dawr, dawrx, mrd;
 
@@ -816,7 +817,7 @@ void __set_breakpoint(struct arch_hw_breakpoint *brk)
 {
 	memcpy(this_cpu_ptr(&current_brk), brk, sizeof(*brk));
 
-	if (cpu_has_feature(CPU_FTR_DAWR))
+	if (dawr_enabled())
 		// Power8 or later
 		set_dawr(brk);
 	else if (!cpu_has_feature(CPU_FTR_ARCH_207S))
@@ -830,8 +831,8 @@ void __set_breakpoint(struct arch_hw_breakpoint *brk)
 /* Check if we have DAWR or DABR hardware */
 bool ppc_breakpoint_available(void)
 {
-	if (cpu_has_feature(CPU_FTR_DAWR))
-		return true; /* POWER8 DAWR */
+	if (dawr_enabled())
+		return true; /* POWER8 DAWR or POWER9 forced DAWR */
 	if (cpu_has_feature(CPU_FTR_ARCH_207S))
 		return false; /* POWER9 with DAWR disabled */
 	/* DABR: Everything but POWER8 and POWER9 */
diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index d9ac7d9465..684b0b315c 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -43,6 +43,7 @@
 #include <asm/tm.h>
 #include <asm/asm-prototypes.h>
 #include <asm/debug.h>
+#include <asm/hw_breakpoint.h>
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/syscalls.h>
@@ -3088,7 +3089,7 @@ long arch_ptrace(struct task_struct *child, long request,
 		dbginfo.sizeof_condition = 0;
 #ifdef CONFIG_HAVE_HW_BREAKPOINT
 		dbginfo.features = PPC_DEBUG_FEATURE_DATA_BP_RANGE;
-		if (cpu_has_feature(CPU_FTR_DAWR))
+		if (dawr_enabled())
 			dbginfo.features |= PPC_DEBUG_FEATURE_DATA_BP_DAWR;
 #else
 		dbginfo.features = 0;
diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index 06964350b9..0fab0a2010 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -74,6 +74,7 @@
 #include <asm/opal.h>
 #include <asm/xics.h>
 #include <asm/xive.h>
+#include <asm/hw_breakpoint.h>
 
 #include "book3s.h"
 
@@ -3374,7 +3375,7 @@ static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
 	mtspr(SPRN_PURR, vcpu->arch.purr);
 	mtspr(SPRN_SPURR, vcpu->arch.spurr);
 
-	if (cpu_has_feature(CPU_FTR_DAWR)) {
+	if (dawr_enabled()) {
 		mtspr(SPRN_DAWR, vcpu->arch.dawr);
 		mtspr(SPRN_DAWRX, vcpu->arch.dawrx);
 	}
diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
index 3a5e719ef0..139027c62d 100644
--- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
+++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
@@ -822,18 +822,21 @@ END_FTR_SECTION_IFCLR(CPU_FTR_ARCH_207S)
 	mtspr	SPRN_IAMR, r5
 	mtspr	SPRN_PSPB, r6
 	mtspr	SPRN_FSCR, r7
-	ld	r5, VCPU_DAWR(r4)
-	ld	r6, VCPU_DAWRX(r4)
-	ld	r7, VCPU_CIABR(r4)
-	ld	r8, VCPU_TAR(r4)
 	/*
 	 * Handle broken DAWR case by not writing it. This means we
 	 * can still store the DAWR register for migration.
 	 */
-BEGIN_FTR_SECTION
+	LOAD_REG_ADDR(r5, dawr_force_enable)
+	lbz	r5, 0(r5)
+	cmpdi	r5, 0
+	beq	1f
+	ld	r5, VCPU_DAWR(r4)
+	ld	r6, VCPU_DAWRX(r4)
 	mtspr	SPRN_DAWR, r5
 	mtspr	SPRN_DAWRX, r6
-END_FTR_SECTION_IFSET(CPU_FTR_DAWR)
+1:
+	ld	r7, VCPU_CIABR(r4)
+	ld	r8, VCPU_TAR(r4)
 	mtspr	SPRN_CIABR, r7
 	mtspr	SPRN_TAR, r8
 	ld	r5, VCPU_IC(r4)
@@ -2513,11 +2516,11 @@ END_FTR_SECTION_IFSET(CPU_FTR_ARCH_207S)
 	blr
 
 2:
-BEGIN_FTR_SECTION
-	/* POWER9 with disabled DAWR */
+	LOAD_REG_ADDR(r11, dawr_force_enable)
+	lbz	r11, 0(r11)
+	cmpdi	r11, 0
 	li	r3, H_HARDWARE
-	blr
-END_FTR_SECTION_IFCLR(CPU_FTR_DAWR)
+	beqlr
 	/* Emulate H_SET_DABR/X on P8 for the sake of compat mode guests */
 	rlwimi	r5, r4, 5, DAWRX_DR | DAWRX_DW
 	rlwimi	r5, r4, 2, DAWRX_WT
-- 
2.20.1

Re: [PATCH v2] powerpc: Add force enable of DAWR on P9 option

[Michael Ellerman]

On Mon, 2019-04-01 at 06:03:12 UTC, Michael Neuling wrote:
> This adds a flag so that the DAWR can be enabled on P9 via:
>   echo Y > /sys/kernel/debug/powerpc/dawr_enable_dangerous
> 
> The DAWR was previously force disabled on POWER9 in:
>   9654153158 powerpc: Disable DAWR in the base POWER9 CPU features
> Also see Documentation/powerpc/DAWR-POWER9.txt
> 
> This is a dangerous setting, USE AT YOUR OWN RISK.
> 
> Some users may not care about a bad user crashing their box
> (ie. single user/desktop systems) and really want the DAWR.  This
> allows them to force enable DAWR.
> 
> This flag can also be used to disable DAWR access. Once this is
> cleared, all DAWR access should be cleared immediately and your
> machine once again safe from crashing.
> 
> Userspace may get confused by toggling this. If DAWR is force
> enabled/disabled between getting the number of breakpoints (via
> PTRACE_GETHWDBGINFO) and setting the breakpoint, userspace will get an
> inconsistent view of what's available. Similarly for guests.
> 
> For the DAWR to be enabled in a KVM guest, the DAWR needs to be force
> enabled in the host AND the guest. For this reason, this won't work on
> POWERVM as it doesn't allow the HCALL to work. Writes of 'Y' to the
> dawr_enable_dangerous file will fail if the hypervisor doesn't support
> writing the DAWR.
> 
> To double check the DAWR is working, run this kernel selftest:
>   tools/testing/selftests/powerpc/ptrace/ptrace-hwbreak.c
> Any errors/failures/skips mean something is wrong.
> 
> Signed-off-by: Michael Neuling <mikey@neuling.org>

Applied to powerpc topic/ppc-kvm, thanks.

https://git.kernel.org/powerpc/c/c1fe190c06723322f2dfac31d3b982c5

cheers

Re: [PATCH v2] powerpc: Add force enable of DAWR on P9 option

[Cédric Le Goater]

Hello Michael,


> --- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> +++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> @@ -822,18 +822,21 @@ END_FTR_SECTION_IFCLR(CPU_FTR_ARCH_207S)
>  	mtspr	SPRN_IAMR, r5
>  	mtspr	SPRN_PSPB, r6
>  	mtspr	SPRN_FSCR, r7
> -	ld	r5, VCPU_DAWR(r4)
> -	ld	r6, VCPU_DAWRX(r4)
> -	ld	r7, VCPU_CIABR(r4)
> -	ld	r8, VCPU_TAR(r4)
>  	/*
>  	 * Handle broken DAWR case by not writing it. This means we
>  	 * can still store the DAWR register for migration.
>  	 */
> -BEGIN_FTR_SECTION
> +	LOAD_REG_ADDR(r5, dawr_force_enable)
> +	lbz	r5, 0(r5)
> +	cmpdi	r5, 0
> +	beq	1f
> +	ld	r5, VCPU_DAWR(r4)
> +	ld	r6, VCPU_DAWRX(r4)
>  	mtspr	SPRN_DAWR, r5
>  	mtspr	SPRN_DAWRX, r6
> -END_FTR_SECTION_IFSET(CPU_FTR_DAWR)
> +1:
> +	ld	r7, VCPU_CIABR(r4)
> +	ld	r8, VCPU_TAR(r4)
>  	mtspr	SPRN_CIABR, r7
>  	mtspr	SPRN_TAR, r8
>  	ld	r5, VCPU_IC(r4)
> @@ -2513,11 +2516,11 @@ END_FTR_SECTION_IFSET(CPU_FTR_ARCH_207S)
>  	blr
>  
>  2:
> -BEGIN_FTR_SECTION
> -	/* POWER9 with disabled DAWR */
> +	LOAD_REG_ADDR(r11, dawr_force_enable)
> +	lbz	r11, 0(r11)
> +	cmpdi	r11, 0
>  	li	r3, H_HARDWARE
> -	blr
> -END_FTR_SECTION_IFCLR(CPU_FTR_DAWR)
> +	beqlr

Why is this a 'beqlr' ? Shouldn't it be a blr ? 

C.

>  	/* Emulate H_SET_DABR/X on P8 for the sake of compat mode guests */
>  	rlwimi	r5, r4, 5, DAWRX_DR | DAWRX_DW
>  	rlwimi	r5, r4, 2, DAWRX_WT
> 

Re: [PATCH v2] powerpc: Add force enable of DAWR on P9 option

[Michael Neuling]

> >  2:
> > -BEGIN_FTR_SECTION
> > -	/* POWER9 with disabled DAWR */
> > +	LOAD_REG_ADDR(r11, dawr_force_enable)
> > +	lbz	r11, 0(r11)
> > +	cmpdi	r11, 0
> >  	li	r3, H_HARDWARE
> > -	blr
> > -END_FTR_SECTION_IFCLR(CPU_FTR_DAWR)
> > +	beqlr
> 
> Why is this a 'beqlr' ? Shouldn't it be a blr ? 

I believe it's right and should be a beqlr.  It's to replace the FTR section to
make it dynamic based on the dawr_force_enable bit.

Mikey

Re: [PATCH v2] powerpc: Add force enable of DAWR on P9 option

[Cédric Le Goater]

On 11/06/2019 08:44, Michael Neuling wrote:
> 
>>>  2:
>>> -BEGIN_FTR_SECTION
>>> -	/* POWER9 with disabled DAWR */
>>> +	LOAD_REG_ADDR(r11, dawr_force_enable)
>>> +	lbz	r11, 0(r11)
>>> +	cmpdi	r11, 0
>>>  	li	r3, H_HARDWARE
>>> -	blr
>>> -END_FTR_SECTION_IFCLR(CPU_FTR_DAWR)
>>> +	beqlr
>>
>> Why is this a 'beqlr' ? Shouldn't it be a blr ? 
> 
> I believe it's right and should be a beqlr.  It's to replace the FTR section to
> make it dynamic based on the dawr_force_enable bit.

hmm, see the crash below on a L1 running a nested guest. r3 is set
to -1 (H_HARDWARE) but a vpcu pointer was expected. How can we fix
this ? 

C.


[   44.374746] BUG: Kernel NULL pointer dereference at 0x000013bf
[   44.374848] Faulting instruction address: 0xc00000000010b044
[   44.374906] Oops: Kernel access of bad area, sig: 11 [#1]
[   44.374951] LE PAGE_SIZE=64K MMU=Radix MMU=Hash SMP NR_CPUS=2048 NUMA pSeries
[   44.375018] Modules linked in: vhost_net vhost tap xt_CHECKSUM iptable_mangle xt_MASQUERADE iptable_nat nf_nat xt_conntrack nf_conntrack nf_defrag_ipv6 libcrc32c nf_defrag_ipv4 ipt_REJECT nf_reject_ipv4 xt_tcpudp bridge stp llc ebtable_filter ebtables ip6table_filter ip6_tables iptable_filter bpfilter vmx_crypto crct10dif_vpmsum crc32c_vpmsum kvm_hv kvm sch_fq_codel ip_tables x_tables autofs4 virtio_net net_failover virtio_scsi failover
[   44.375401] CPU: 8 PID: 1771 Comm: qemu-system-ppc Kdump: loaded Not tainted 5.2.0-rc4+ #3
[   44.375500] NIP:  c00000000010b044 LR: c0080000089dacf4 CTR: c00000000010aff4
[   44.375604] REGS: c00000179b397710 TRAP: 0300   Not tainted  (5.2.0-rc4+)
[   44.375691] MSR:  800000000280b033 <SF,VEC,VSX,EE,FP,ME,IR,DR,RI,LE>  CR: 42244842  XER: 00000000
[   44.375815] CFAR: c00000000010aff8 DAR: 00000000000013bf DSISR: 42000000 IRQMASK: 0 
[   44.375815] GPR00: c0080000089dd6bc c00000179b3979a0 c008000008a04300 ffffffffffffffff 
[   44.375815] GPR04: 0000000000000000 0000000000000003 000000002444b05d c0000017f11c45d0 
[   44.375815] GPR08: 078000003e018dfe 0000000000000028 0000000000000001 0000000000000075 
[   44.375815] GPR12: c00000000010aff4 c000000007ff6300 0000000000000000 0000000000000000 
[   44.375815] GPR16: 0000000000000000 c0000017f11d0000 00000000ffffffff c0000017f11ca7a8 
[   44.375815] GPR20: c0000017f11c42ec ffffffffffffffff 0000000000000000 000000000000000a 
[   44.375815] GPR24: fffffffffffffffc 0000000000000000 c0000017f11c0000 c000000001a77ed8 
[   44.375815] GPR28: c00000179af70000 fffffffffffffffc c0080000089ff170 c00000179ae88540 
[   44.376673] NIP [c00000000010b044] kvmppc_h_set_dabr+0x50/0x68
[   44.376754] LR [c0080000089dacf4] kvmppc_pseries_do_hcall+0xa3c/0xeb0 [kvm_hv]
[   44.376849] Call Trace:
[   44.376886] [c00000179b3979a0] [c0000017f11c0000] 0xc0000017f11c0000 (unreliable)
[   44.376982] [c00000179b397a10] [c0080000089dd6bc] kvmppc_vcpu_run_hv+0x694/0xec0 [kvm_hv]
[   44.377084] [c00000179b397ae0] [c0080000093f8bcc] kvmppc_vcpu_run+0x34/0x48 [kvm]
[   44.377185] [c00000179b397b00] [c0080000093f522c] kvm_arch_vcpu_ioctl_run+0x2f4/0x400 [kvm]
[   44.377286] [c00000179b397b90] [c0080000093e3618] kvm_vcpu_ioctl+0x460/0x850 [kvm]
[   44.377384] [c00000179b397d00] [c0000000004ba6c4] do_vfs_ioctl+0xe4/0xb40
[   44.377464] [c00000179b397db0] [c0000000004bb1e4] ksys_ioctl+0xc4/0x110
[   44.377547] [c00000179b397e00] [c0000000004bb258] sys_ioctl+0x28/0x80
[   44.377628] [c00000179b397e20] [c00000000000b888] system_call+0x5c/0x70
[   44.377712] Instruction dump:
[   44.377765] 4082fff4 4c00012c 38600000 4e800020 e96280c0 896b0000 2c2b0000 3860ffff 
[   44.377862] 4d820020 50852e74 508516f6 78840724 <f88313c0> f8a313c8 7c942ba6 7cbc2ba6 

Re: [PATCH v2] powerpc: Add force enable of DAWR on P9 option

[Michael Neuling]

On Tue, 2019-06-11 at 08:48 +0200, Cédric Le Goater wrote:
> On 11/06/2019 08:44, Michael Neuling wrote:
> > > >  2:
> > > > -BEGIN_FTR_SECTION
> > > > -	/* POWER9 with disabled DAWR */
> > > > +	LOAD_REG_ADDR(r11, dawr_force_enable)
> > > > +	lbz	r11, 0(r11)
> > > > +	cmpdi	r11, 0
> > > >  	li	r3, H_HARDWARE
> > > > -	blr
> > > > -END_FTR_SECTION_IFCLR(CPU_FTR_DAWR)
> > > > +	beqlr
> > > 
> > > Why is this a 'beqlr' ? Shouldn't it be a blr ? 
> > 
> > I believe it's right and should be a beqlr.  It's to replace the FTR section to
> > make it dynamic based on the dawr_force_enable bit.
> 
> hmm, see the crash below on a L1 running a nested guest. r3 is set
> to -1 (H_HARDWARE) but a vpcu pointer was expected. How can we fix
> this ? 
> 
> C.
> 
> 
> [   44.374746] BUG: Kernel NULL pointer dereference at 0x000013bf
> [   44.374848] Faulting instruction address: 0xc00000000010b044
> [   44.374906] Oops: Kernel access of bad area, sig: 11 [#1]
> [   44.374951] LE PAGE_SIZE=64K MMU=Radix MMU=Hash SMP NR_CPUS=2048 NUMA pSeries
> [   44.375018] Modules linked in: vhost_net vhost tap xt_CHECKSUM iptable_mangle xt_MASQUERADE iptable_nat nf_nat xt_conntrack nf_conntrack nf_defrag_ipv6 libcrc32c nf_defrag_ipv4 ipt_REJECT nf_reject_ipv4 xt_tcpudp bridge stp llc ebtable_filter ebtables ip6table_filter ip6_tables iptable_filter bpfilter vmx_crypto crct10dif_vpmsum crc32c_vpmsum kvm_hv kvm sch_fq_codel ip_tables x_tables autofs4 virtio_net net_failover virtio_scsi failover
> [   44.375401] CPU: 8 PID: 1771 Comm: qemu-system-ppc Kdump: loaded Not tainted 5.2.0-rc4+ #3
> [   44.375500] NIP:  c00000000010b044 LR: c0080000089dacf4 CTR: c00000000010aff4
> [   44.375604] REGS: c00000179b397710 TRAP: 0300   Not tainted  (5.2.0-rc4+)
> [   44.375691] MSR:  800000000280b033 <SF,VEC,VSX,EE,FP,ME,IR,DR,RI,LE>  CR: 42244842  XER: 00000000
> [   44.375815] CFAR: c00000000010aff8 DAR: 00000000000013bf DSISR: 42000000 IRQMASK: 0 
> [   44.375815] GPR00: c0080000089dd6bc c00000179b3979a0 c008000008a04300 ffffffffffffffff 
> [   44.375815] GPR04: 0000000000000000 0000000000000003 000000002444b05d c0000017f11c45d0 
> [   44.375815] GPR08: 078000003e018dfe 0000000000000028 0000000000000001 0000000000000075 
> [   44.375815] GPR12: c00000000010aff4 c000000007ff6300 0000000000000000 0000000000000000 
> [   44.375815] GPR16: 0000000000000000 c0000017f11d0000 00000000ffffffff c0000017f11ca7a8 
> [   44.375815] GPR20: c0000017f11c42ec ffffffffffffffff 0000000000000000 000000000000000a 
> [   44.375815] GPR24: fffffffffffffffc 0000000000000000 c0000017f11c0000 c000000001a77ed8 
> [   44.375815] GPR28: c00000179af70000 fffffffffffffffc c0080000089ff170 c00000179ae88540 
> [   44.376673] NIP [c00000000010b044] kvmppc_h_set_dabr+0x50/0x68
> [   44.376754] LR [c0080000089dacf4] kvmppc_pseries_do_hcall+0xa3c/0xeb0 [kvm_hv]
> [   44.376849] Call Trace:
> [   44.376886] [c00000179b3979a0] [c0000017f11c0000] 0xc0000017f11c0000 (unreliable)
> [   44.376982] [c00000179b397a10] [c0080000089dd6bc] kvmppc_vcpu_run_hv+0x694/0xec0 [kvm_hv]
> [   44.377084] [c00000179b397ae0] [c0080000093f8bcc] kvmppc_vcpu_run+0x34/0x48 [kvm]
> [   44.377185] [c00000179b397b00] [c0080000093f522c] kvm_arch_vcpu_ioctl_run+0x2f4/0x400 [kvm]
> [   44.377286] [c00000179b397b90] [c0080000093e3618] kvm_vcpu_ioctl+0x460/0x850 [kvm]
> [   44.377384] [c00000179b397d00] [c0000000004ba6c4] do_vfs_ioctl+0xe4/0xb40
> [   44.377464] [c00000179b397db0] [c0000000004bb1e4] ksys_ioctl+0xc4/0x110
> [   44.377547] [c00000179b397e00] [c0000000004bb258] sys_ioctl+0x28/0x80
> [   44.377628] [c00000179b397e20] [c00000000000b888] system_call+0x5c/0x70
> [   44.377712] Instruction dump:
> [   44.377765] 4082fff4 4c00012c 38600000 4e800020 e96280c0 896b0000 2c2b0000 3860ffff 
> [   44.377862] 4d820020 50852e74 508516f6 78840724 <f88313c0> f8a313c8 7c942ba6 7cbc2ba6 


Opps, it's because I corrupted r3 :-(

Does this fix it?


diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
index 139027c62d..f781ee1458 100644
--- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
+++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
@@ -2519,8 +2519,10 @@ END_FTR_SECTION_IFSET(CPU_FTR_ARCH_207S)
 	LOAD_REG_ADDR(r11, dawr_force_enable)
 	lbz	r11, 0(r11)
 	cmpdi	r11, 0
+	bne	3f
 	li	r3, H_HARDWARE
-	beqlr
+	blr
+3:
 	/* Emulate H_SET_DABR/X on P8 for the sake of compat mode guests */
 	rlwimi	r5, r4, 5, DAWRX_DR | DAWRX_DW
 	rlwimi	r5, r4, 2, DAWRX_WT

Re: [PATCH v2] powerpc: Add force enable of DAWR on P9 option

[Christophe Leroy]

Le 11/06/2019 à 09:24, Michael Neuling a écrit :
> On Tue, 2019-06-11 at 08:48 +0200, Cédric Le Goater wrote:
>> On 11/06/2019 08:44, Michael Neuling wrote:
>>>>>   2:
>>>>> -BEGIN_FTR_SECTION
>>>>> -	/* POWER9 with disabled DAWR */
>>>>> +	LOAD_REG_ADDR(r11, dawr_force_enable)
>>>>> +	lbz	r11, 0(r11)
>>>>> +	cmpdi	r11, 0
>>>>>   	li	r3, H_HARDWARE
>>>>> -	blr
>>>>> -END_FTR_SECTION_IFCLR(CPU_FTR_DAWR)
>>>>> +	beqlr
>>>>
>>>> Why is this a 'beqlr' ? Shouldn't it be a blr ?
>>>
>>> I believe it's right and should be a beqlr.  It's to replace the FTR section to
>>> make it dynamic based on the dawr_force_enable bit.
>>
>> hmm, see the crash below on a L1 running a nested guest. r3 is set
>> to -1 (H_HARDWARE) but a vpcu pointer was expected. How can we fix
>> this ?
>>
>> C.
>>
>>
>> [   44.374746] BUG: Kernel NULL pointer dereference at 0x000013bf
>> [   44.374848] Faulting instruction address: 0xc00000000010b044
>> [   44.374906] Oops: Kernel access of bad area, sig: 11 [#1]
>> [   44.374951] LE PAGE_SIZE=64K MMU=Radix MMU=Hash SMP NR_CPUS=2048 NUMA pSeries
>> [   44.375018] Modules linked in: vhost_net vhost tap xt_CHECKSUM iptable_mangle xt_MASQUERADE iptable_nat nf_nat xt_conntrack nf_conntrack nf_defrag_ipv6 libcrc32c nf_defrag_ipv4 ipt_REJECT nf_reject_ipv4 xt_tcpudp bridge stp llc ebtable_filter ebtables ip6table_filter ip6_tables iptable_filter bpfilter vmx_crypto crct10dif_vpmsum crc32c_vpmsum kvm_hv kvm sch_fq_codel ip_tables x_tables autofs4 virtio_net net_failover virtio_scsi failover
>> [   44.375401] CPU: 8 PID: 1771 Comm: qemu-system-ppc Kdump: loaded Not tainted 5.2.0-rc4+ #3
>> [   44.375500] NIP:  c00000000010b044 LR: c0080000089dacf4 CTR: c00000000010aff4
>> [   44.375604] REGS: c00000179b397710 TRAP: 0300   Not tainted  (5.2.0-rc4+)
>> [   44.375691] MSR:  800000000280b033 <SF,VEC,VSX,EE,FP,ME,IR,DR,RI,LE>  CR: 42244842  XER: 00000000
>> [   44.375815] CFAR: c00000000010aff8 DAR: 00000000000013bf DSISR: 42000000 IRQMASK: 0
>> [   44.375815] GPR00: c0080000089dd6bc c00000179b3979a0 c008000008a04300 ffffffffffffffff
>> [   44.375815] GPR04: 0000000000000000 0000000000000003 000000002444b05d c0000017f11c45d0
>> [   44.375815] GPR08: 078000003e018dfe 0000000000000028 0000000000000001 0000000000000075
>> [   44.375815] GPR12: c00000000010aff4 c000000007ff6300 0000000000000000 0000000000000000
>> [   44.375815] GPR16: 0000000000000000 c0000017f11d0000 00000000ffffffff c0000017f11ca7a8
>> [   44.375815] GPR20: c0000017f11c42ec ffffffffffffffff 0000000000000000 000000000000000a
>> [   44.375815] GPR24: fffffffffffffffc 0000000000000000 c0000017f11c0000 c000000001a77ed8
>> [   44.375815] GPR28: c00000179af70000 fffffffffffffffc c0080000089ff170 c00000179ae88540
>> [   44.376673] NIP [c00000000010b044] kvmppc_h_set_dabr+0x50/0x68
>> [   44.376754] LR [c0080000089dacf4] kvmppc_pseries_do_hcall+0xa3c/0xeb0 [kvm_hv]
>> [   44.376849] Call Trace:
>> [   44.376886] [c00000179b3979a0] [c0000017f11c0000] 0xc0000017f11c0000 (unreliable)
>> [   44.376982] [c00000179b397a10] [c0080000089dd6bc] kvmppc_vcpu_run_hv+0x694/0xec0 [kvm_hv]
>> [   44.377084] [c00000179b397ae0] [c0080000093f8bcc] kvmppc_vcpu_run+0x34/0x48 [kvm]
>> [   44.377185] [c00000179b397b00] [c0080000093f522c] kvm_arch_vcpu_ioctl_run+0x2f4/0x400 [kvm]
>> [   44.377286] [c00000179b397b90] [c0080000093e3618] kvm_vcpu_ioctl+0x460/0x850 [kvm]
>> [   44.377384] [c00000179b397d00] [c0000000004ba6c4] do_vfs_ioctl+0xe4/0xb40
>> [   44.377464] [c00000179b397db0] [c0000000004bb1e4] ksys_ioctl+0xc4/0x110
>> [   44.377547] [c00000179b397e00] [c0000000004bb258] sys_ioctl+0x28/0x80
>> [   44.377628] [c00000179b397e20] [c00000000000b888] system_call+0x5c/0x70
>> [   44.377712] Instruction dump:
>> [   44.377765] 4082fff4 4c00012c 38600000 4e800020 e96280c0 896b0000 2c2b0000 3860ffff
>> [   44.377862] 4d820020 50852e74 508516f6 78840724 <f88313c0> f8a313c8 7c942ba6 7cbc2ba6
> 
> 
> Opps, it's because I corrupted r3 :-(
> 
> Does this fix it?
> 
> 
> diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> index 139027c62d..f781ee1458 100644
> --- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> +++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> @@ -2519,8 +2519,10 @@ END_FTR_SECTION_IFSET(CPU_FTR_ARCH_207S)
>   	LOAD_REG_ADDR(r11, dawr_force_enable)
>   	lbz	r11, 0(r11)
>   	cmpdi	r11, 0
> +	bne	3f
>   	li	r3, H_HARDWARE
> -	beqlr
> +	blr
> +3:

Or you could copy r3 into another unused volatile register and use it 
instead of r3 below.

Christophe


>   	/* Emulate H_SET_DABR/X on P8 for the sake of compat mode guests */
>   	rlwimi	r5, r4, 5, DAWRX_DR | DAWRX_DW
>   	rlwimi	r5, r4, 2, DAWRX_WT
> 

Re: [PATCH v2] powerpc: Add force enable of DAWR on P9 option

[Michael Neuling]

On Tue, 2019-06-11 at 09:51 +0200, Christophe Leroy wrote:
> 
> Le 11/06/2019 à 09:24, Michael Neuling a écrit :
> > On Tue, 2019-06-11 at 08:48 +0200, Cédric Le Goater wrote:
> > > On 11/06/2019 08:44, Michael Neuling wrote:
> > > > > >   2:
> > > > > > -BEGIN_FTR_SECTION
> > > > > > -	/* POWER9 with disabled DAWR */
> > > > > > +	LOAD_REG_ADDR(r11, dawr_force_enable)
> > > > > > +	lbz	r11, 0(r11)
> > > > > > +	cmpdi	r11, 0
> > > > > >   	li	r3, H_HARDWARE
> > > > > > -	blr
> > > > > > -END_FTR_SECTION_IFCLR(CPU_FTR_DAWR)
> > > > > > +	beqlr
> > > > > 
> > > > > Why is this a 'beqlr' ? Shouldn't it be a blr ?
> > > > 
> > > > I believe it's right and should be a beqlr.  It's to replace the FTR
> > > > section to
> > > > make it dynamic based on the dawr_force_enable bit.
> > > 
> > > hmm, see the crash below on a L1 running a nested guest. r3 is set
> > > to -1 (H_HARDWARE) but a vpcu pointer was expected. How can we fix
> > > this ?
> > > 
> > > C.
> > > 
> > > 
> > > [   44.374746] BUG: Kernel NULL pointer dereference at 0x000013bf
> > > [   44.374848] Faulting instruction address: 0xc00000000010b044
> > > [   44.374906] Oops: Kernel access of bad area, sig: 11 [#1]
> > > [   44.374951] LE PAGE_SIZE=64K MMU=Radix MMU=Hash SMP NR_CPUS=2048 NUMA
> > > pSeries
> > > [   44.375018] Modules linked in: vhost_net vhost tap xt_CHECKSUM
> > > iptable_mangle xt_MASQUERADE iptable_nat nf_nat xt_conntrack nf_conntrack
> > > nf_defrag_ipv6 libcrc32c nf_defrag_ipv4 ipt_REJECT nf_reject_ipv4
> > > xt_tcpudp bridge stp llc ebtable_filter ebtables ip6table_filter
> > > ip6_tables iptable_filter bpfilter vmx_crypto crct10dif_vpmsum
> > > crc32c_vpmsum kvm_hv kvm sch_fq_codel ip_tables x_tables autofs4
> > > virtio_net net_failover virtio_scsi failover
> > > [   44.375401] CPU: 8 PID: 1771 Comm: qemu-system-ppc Kdump: loaded Not
> > > tainted 5.2.0-rc4+ #3
> > > [   44.375500] NIP:  c00000000010b044 LR: c0080000089dacf4 CTR:
> > > c00000000010aff4
> > > [   44.375604] REGS: c00000179b397710 TRAP: 0300   Not tainted  (5.2.0-
> > > rc4+)
> > > [   44.375691] MSR:  800000000280b033
> > > <SF,VEC,VSX,EE,FP,ME,IR,DR,RI,LE>  CR: 42244842  XER: 00000000
> > > [   44.375815] CFAR: c00000000010aff8 DAR: 00000000000013bf DSISR:
> > > 42000000 IRQMASK: 0
> > > [   44.375815] GPR00: c0080000089dd6bc c00000179b3979a0 c008000008a04300
> > > ffffffffffffffff
> > > [   44.375815] GPR04: 0000000000000000 0000000000000003 000000002444b05d
> > > c0000017f11c45d0
> > > [   44.375815] GPR08: 078000003e018dfe 0000000000000028 0000000000000001
> > > 0000000000000075
> > > [   44.375815] GPR12: c00000000010aff4 c000000007ff6300 0000000000000000
> > > 0000000000000000
> > > [   44.375815] GPR16: 0000000000000000 c0000017f11d0000 00000000ffffffff
> > > c0000017f11ca7a8
> > > [   44.375815] GPR20: c0000017f11c42ec ffffffffffffffff 0000000000000000
> > > 000000000000000a
> > > [   44.375815] GPR24: fffffffffffffffc 0000000000000000 c0000017f11c0000
> > > c000000001a77ed8
> > > [   44.375815] GPR28: c00000179af70000 fffffffffffffffc c0080000089ff170
> > > c00000179ae88540
> > > [   44.376673] NIP [c00000000010b044] kvmppc_h_set_dabr+0x50/0x68
> > > [   44.376754] LR [c0080000089dacf4] kvmppc_pseries_do_hcall+0xa3c/0xeb0
> > > [kvm_hv]
> > > [   44.376849] Call Trace:
> > > [   44.376886] [c00000179b3979a0] [c0000017f11c0000] 0xc0000017f11c0000
> > > (unreliable)
> > > [   44.376982] [c00000179b397a10] [c0080000089dd6bc]
> > > kvmppc_vcpu_run_hv+0x694/0xec0 [kvm_hv]
> > > [   44.377084] [c00000179b397ae0] [c0080000093f8bcc]
> > > kvmppc_vcpu_run+0x34/0x48 [kvm]
> > > [   44.377185] [c00000179b397b00] [c0080000093f522c]
> > > kvm_arch_vcpu_ioctl_run+0x2f4/0x400 [kvm]
> > > [   44.377286] [c00000179b397b90] [c0080000093e3618]
> > > kvm_vcpu_ioctl+0x460/0x850 [kvm]
> > > [   44.377384] [c00000179b397d00] [c0000000004ba6c4]
> > > do_vfs_ioctl+0xe4/0xb40
> > > [   44.377464] [c00000179b397db0] [c0000000004bb1e4] ksys_ioctl+0xc4/0x110
> > > [   44.377547] [c00000179b397e00] [c0000000004bb258] sys_ioctl+0x28/0x80
> > > [   44.377628] [c00000179b397e20] [c00000000000b888] system_call+0x5c/0x70
> > > [   44.377712] Instruction dump:
> > > [   44.377765] 4082fff4 4c00012c 38600000 4e800020 e96280c0 896b0000
> > > 2c2b0000 3860ffff
> > > [   44.377862] 4d820020 50852e74 508516f6 78840724 <f88313c0> f8a313c8
> > > 7c942ba6 7cbc2ba6
> > 
> > Opps, it's because I corrupted r3 :-(
> > 
> > Does this fix it?
> > 
> > 
> > diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> > b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> > index 139027c62d..f781ee1458 100644
> > --- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> > +++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> > @@ -2519,8 +2519,10 @@ END_FTR_SECTION_IFSET(CPU_FTR_ARCH_207S)
> >   	LOAD_REG_ADDR(r11, dawr_force_enable)
> >   	lbz	r11, 0(r11)
> >   	cmpdi	r11, 0
> > +	bne	3f
> >   	li	r3, H_HARDWARE
> > -	beqlr
> > +	blr
> > +3:
> 
> Or you could copy r3 into another unused volatile register and use it 
> instead of r3 below.

r3 is the vcpu pointer passed in. Changing to a different register will make the
code harder to follow IMHO.

Plus this is a much clearer fix.

So I don't think I'll do that.

Mikey

> 
> Christophe
> 
> 
> >   	/* Emulate H_SET_DABR/X on P8 for the sake of compat mode guests */
> >   	rlwimi	r5, r4, 5, DAWRX_DR | DAWRX_DW
> >   	rlwimi	r5, r4, 2, DAWRX_WT
> >