powerpc: Disable DAWR on POWER9

Message ID 20180316025456.24036-1-mikey@neuling.org
State New
Headers show
Series
  • powerpc: Disable DAWR on POWER9
Related show

Commit Message

Michael Neuling March 16, 2018, 2:54 a.m.
Using the DAWR on POWER9 can cause xstops, hence we need to disable
it.

The current CPU_FTR for DAWR is a bit messy. Despite having
CPU_FTR_DAWR, currently we assume DAWR exists in the KVM code based on
CPU_FTR_ARCH_207. In other places we assume DAWR exists if
CPU_FTR_DAWR is set.

This attempts to clear up the situation by always using CPU_FTR_DAWR
before setting the DAWR (to a non-zero value).

DAWR has 5 different ways of being set from userspace. ptrace,
h_set_mode, h_set_mode(DAWR), h_set_dabr(), kvmppc_set_one_reg() and
xmon.

For ptrace, we now return an error if run on a POWER9. GDB gives this
error when you attempt to run a program with a breakpoint set on
POWER9:
  "Unexpected error setting breakpoint or watchpoint: No such device."

h_set_mode() and h_set_dabr() will now return an error to the guest
when on a POWER9 host. Current Linux guests ignore this error, so they
will silently not get the DAWR (sigh). The same error codes are being
used by POWERVM in this case.

kvmppc_set_one_reg() will store the value in the vcpu but won't
actually set it on POWER9 hardware. This is done so we don't break
migration from P8 to P9, at the cost of silently losing the DAWR on
the migration. This is not ideal but hopefully the best overall
solution. This approach has been acked by paulus.

For xmon, the 'bd' command will return an error on P9.

Thanks to Pedro Franco de Carvalho for the initial version of this.

Signed-off-by: Michael Neuling <mikey@neuling.org>
---
 arch/powerpc/include/asm/cputable.h     |  5 ++---
 arch/powerpc/include/asm/debug.h        |  1 +
 arch/powerpc/include/asm/hvcall.h       |  1 +
 arch/powerpc/kernel/dt_cpu_ftrs.c       |  1 -
 arch/powerpc/kernel/hw_breakpoint.c     |  3 +++
 arch/powerpc/kernel/process.c           | 11 +++++++++++
 arch/powerpc/kernel/ptrace.c            | 13 +++++++++++--
 arch/powerpc/kvm/book3s_hv.c            |  2 ++
 arch/powerpc/kvm/book3s_hv_rmhandlers.S | 12 ++++++++++++
 arch/powerpc/xmon/xmon.c                |  5 +++++
 10 files changed, 48 insertions(+), 6 deletions(-)

Comments

Nicholas Piggin March 16, 2018, 3:43 a.m. | #1
On Fri, 16 Mar 2018 13:54:56 +1100
Michael Neuling <mikey@neuling.org> wrote:


> diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
> index 945e2c29ad..dfb77b43f1 100644
> --- a/arch/powerpc/kernel/dt_cpu_ftrs.c
> +++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
> @@ -62,7 +62,6 @@ struct dt_cpu_feature {
>  	    CPU_FTR_COHERENT_ICACHE | \
>  	    CPU_FTR_STCX_CHECKS_ADDRESS |\
>  	    CPU_FTR_POPCNTB | CPU_FTR_POPCNTD | \
> -	    CPU_FTR_DAWR | \
>  	    CPU_FTR_ARCH_206 |\
>  	    CPU_FTR_ARCH_207S)

This may be better to clear in cpufeatures_cpu_quirks(). This is supposed
to work on POWER8 as well.

Thanks,
Nick

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michael Neuling March 16, 2018, 5:26 a.m. | #2
On Fri, 2018-03-16 at 13:43 +1000, Nicholas Piggin wrote:
> On Fri, 16 Mar 2018 13:54:56 +1100
> Michael Neuling <mikey@neuling.org> wrote:
> 
> 
> > diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c
> > b/arch/powerpc/kernel/dt_cpu_ftrs.c
> > index 945e2c29ad..dfb77b43f1 100644
> > --- a/arch/powerpc/kernel/dt_cpu_ftrs.c
> > +++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
> > @@ -62,7 +62,6 @@ struct dt_cpu_feature {
> >  	    CPU_FTR_COHERENT_ICACHE | \
> >  	    CPU_FTR_STCX_CHECKS_ADDRESS |\
> >  	    CPU_FTR_POPCNTB | CPU_FTR_POPCNTD | \
> > -	    CPU_FTR_DAWR | \
> >  	    CPU_FTR_ARCH_206 |\
> >  	    CPU_FTR_ARCH_207S)
> 
> This may be better to clear in cpufeatures_cpu_quirks(). 

OK, I wasn't sure about this bit... thanks for catching.  I'll move to there.

> This is supposed to work on POWER8 as well.

Arrh, yeah.

Mikey
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nicholas Piggin March 16, 2018, 7:17 a.m. | #3
On Fri, 16 Mar 2018 16:26:33 +1100
Michael Neuling <mikey@neuling.org> wrote:

> On Fri, 2018-03-16 at 13:43 +1000, Nicholas Piggin wrote:
> > On Fri, 16 Mar 2018 13:54:56 +1100
> > Michael Neuling <mikey@neuling.org> wrote:
> > 
> >   
> > > diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c
> > > b/arch/powerpc/kernel/dt_cpu_ftrs.c
> > > index 945e2c29ad..dfb77b43f1 100644
> > > --- a/arch/powerpc/kernel/dt_cpu_ftrs.c
> > > +++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
> > > @@ -62,7 +62,6 @@ struct dt_cpu_feature {
> > >  	    CPU_FTR_COHERENT_ICACHE | \
> > >  	    CPU_FTR_STCX_CHECKS_ADDRESS |\
> > >  	    CPU_FTR_POPCNTB | CPU_FTR_POPCNTD | \
> > > -	    CPU_FTR_DAWR | \
> > >  	    CPU_FTR_ARCH_206 |\
> > >  	    CPU_FTR_ARCH_207S)  
> > 
> > This may be better to clear in cpufeatures_cpu_quirks().   
> 
> OK, I wasn't sure about this bit... thanks for catching.  I'll move to there.
> 
> > This is supposed to work on POWER8 as well.  
> 
> Arrh, yeah.

We *could* move DAWR out of the base feature set. Firmare is not
actually being released for POWER8 in production... Then we can add
a new dt property for DAWR. Existing POWER9 firmware would do the
right thing for new kernels.

I mean DAWR is fairly fundamental so making it a quirk isn't unreasonable
either, just means we have to patch kernels with PVRs.

Thanks,
Nick
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pedro Franco de Carvalho March 16, 2018, 8:53 p.m. | #4
Michael Neuling <mikey@neuling.org> writes:

> For ptrace, we now return an error if run on a POWER9. GDB gives this
> error when you attempt to run a program with a breakpoint set on
> POWER9:
>   "Unexpected error setting breakpoint or watchpoint: No such device."

Thanks a lot for implementing this!

Would it also make sense to have PPC_PTRACE_GETHWDBGINFO return
dbginfo.num_data_bps = 0 if !breakpoint_available()? In this case GDB
would work in a friendlier way, because it would transparently switch to
software watchpoints.

Not sure what to do about .features in this case. I assume currently it
only has PPC_DEBUG_FEATURE_DATA_BP_RANGE set and not the DAWR feature
flag (since the cpu feature is disabled).

Thanks!
Pedro

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michael Neuling March 19, 2018, 4:30 a.m. | #5
On Fri, 2018-03-16 at 17:53 -0300, Pedro Franco de Carvalho wrote:
> Michael Neuling <mikey@neuling.org> writes:
> 
> > For ptrace, we now return an error if run on a POWER9. GDB gives this
> > error when you attempt to run a program with a breakpoint set on
> > POWER9:
> >   "Unexpected error setting breakpoint or watchpoint: No such device."
> 
> Thanks a lot for implementing this!
> 
> Would it also make sense to have PPC_PTRACE_GETHWDBGINFO return
> dbginfo.num_data_bps = 0 if !breakpoint_available()? In this case GDB
> would work in a friendlier way, because it would transparently switch to
> software watchpoints.

If I do that, it looks like GDB falls back to software emulation (very sloooow)
of the watchpoint rather than failing with the error I gave before. 

That's probably the better solution, so I'll do that... Thanks!

> Not sure what to do about .features in this case. I assume currently it
> only has PPC_DEBUG_FEATURE_DATA_BP_RANGE set and not the DAWR feature
> flag (since the cpu feature is disabled).

Yeah, currently you'll get PPC_DEBUG_FEATURE_DATA_BP_RANGE and no
PPC_DEBUG_FEATURE_DATA_BP_DAWR. 

If I set .features = 0 when !breakpoint_enabled() (as well as setting
num_data_bps = 0 above), GDB seems to not like it when I run the program on P9:

    (gdb) r
    Starting program: /bin/true 
    Warning:
    Could not insert hardware watchpoint 1.
    Could not insert hardware breakpoints:
    You may have requested too many hardware breakpoints/watchpoints.
    (gdb)

So I think I'll leave this bits as I had in version 1 of the patch.

Thanks for the catch.

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

Patch

diff --git a/arch/powerpc/include/asm/cputable.h b/arch/powerpc/include/asm/cputable.h
index a2c5c95882..79a206bcd4 100644
--- a/arch/powerpc/include/asm/cputable.h
+++ b/arch/powerpc/include/asm/cputable.h
@@ -463,9 +463,8 @@  static inline void cpu_feature_keys_init(void) { }
 	    CPU_FTR_DSCR | CPU_FTR_SAO  | \
 	    CPU_FTR_STCX_CHECKS_ADDRESS | CPU_FTR_POPCNTB | CPU_FTR_POPCNTD | \
 	    CPU_FTR_CFAR | CPU_FTR_HVMODE | CPU_FTR_VMX_COPY | \
-	    CPU_FTR_DBELL | CPU_FTR_HAS_PPR | CPU_FTR_DAWR | \
-	    CPU_FTR_ARCH_207S | CPU_FTR_TM_COMP | CPU_FTR_ARCH_300 | \
-	    CPU_FTR_PKEY)
+	    CPU_FTR_DBELL | CPU_FTR_HAS_PPR | CPU_FTR_ARCH_207S | \
+	    CPU_FTR_TM_COMP | CPU_FTR_ARCH_300 | CPU_FTR_PKEY)
 #define CPU_FTRS_POWER9_DD1 ((CPU_FTRS_POWER9 | CPU_FTR_POWER9_DD1) & \
 			     (~CPU_FTR_SAO))
 #define CPU_FTRS_POWER9_DD2_0 CPU_FTRS_POWER9
diff --git a/arch/powerpc/include/asm/debug.h b/arch/powerpc/include/asm/debug.h
index fc97404de0..36634a22f3 100644
--- a/arch/powerpc/include/asm/debug.h
+++ b/arch/powerpc/include/asm/debug.h
@@ -47,6 +47,7 @@  static inline int debugger_fault_handler(struct pt_regs *regs) { return 0; }
 
 void set_breakpoint(struct arch_hw_breakpoint *brk);
 void __set_breakpoint(struct arch_hw_breakpoint *brk);
+bool breakpoint_available(void);
 #ifdef CONFIG_PPC_ADV_DEBUG_REGS
 extern void do_send_trap(struct pt_regs *regs, unsigned long address,
 			 unsigned long error_code, int brkpt);
diff --git a/arch/powerpc/include/asm/hvcall.h b/arch/powerpc/include/asm/hvcall.h
index eca3f9c689..e87d465af4 100644
--- a/arch/powerpc/include/asm/hvcall.h
+++ b/arch/powerpc/include/asm/hvcall.h
@@ -88,6 +88,7 @@ 
 #define H_P8		-61
 #define H_P9		-62
 #define H_TOO_BIG	-64
+#define H_UNSUPPORTED	-67
 #define H_OVERLAP	-68
 #define H_INTERRUPT	-69
 #define H_BAD_DATA	-70
diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
index 945e2c29ad..dfb77b43f1 100644
--- a/arch/powerpc/kernel/dt_cpu_ftrs.c
+++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
@@ -62,7 +62,6 @@  struct dt_cpu_feature {
 	    CPU_FTR_COHERENT_ICACHE | \
 	    CPU_FTR_STCX_CHECKS_ADDRESS |\
 	    CPU_FTR_POPCNTB | CPU_FTR_POPCNTD | \
-	    CPU_FTR_DAWR | \
 	    CPU_FTR_ARCH_206 |\
 	    CPU_FTR_ARCH_207S)
 
diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
index 53b9c1dfd7..bee3f702c5 100644
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -33,6 +33,7 @@ 
 #include <asm/hw_breakpoint.h>
 #include <asm/processor.h>
 #include <asm/sstep.h>
+#include <asm/debug.h>
 #include <linux/uaccess.h>
 
 /*
@@ -171,6 +172,8 @@  int arch_validate_hwbkpt_settings(struct perf_event *bp)
 	 * HW_BREAKPOINT_ALIGN by rounding off to the lower address, the
 	 * 'symbolsize' should satisfy the check below.
 	 */
+	if (!breakpoint_available())
+		return -ENODEV;
 	length_max = 8; /* DABR */
 	if (cpu_has_feature(CPU_FTR_DAWR)) {
 		length_max = 512 ; /* 64 doublewords */
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 1738c4127b..c51d7133bf 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -827,6 +827,17 @@  void set_breakpoint(struct arch_hw_breakpoint *brk)
 	preempt_enable();
 }
 
+/* Check if DAWR or DABR hardware */
+bool breakpoint_available(void)
+{
+	if (cpu_has_feature(CPU_FTR_DAWR))
+		return true; /* POWER8 DAWR */
+	if (!cpu_has_feature(CPU_FTR_ARCH_207S))
+		/* DABR: Everything but POWER8 and POWER9 */
+		return true;
+	return false;
+}
+
 #ifdef CONFIG_PPC64
 DEFINE_PER_CPU(struct cpu_usage, cpu_usage_array);
 #endif
diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index ca72d7391d..f4c4f9882c 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -41,6 +41,7 @@ 
 #include <asm/switch_to.h>
 #include <asm/tm.h>
 #include <asm/asm-prototypes.h>
+#include <asm/debug.h>
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/syscalls.h>
@@ -2378,6 +2379,7 @@  static int ptrace_set_debugreg(struct task_struct *task, unsigned long addr,
 	struct perf_event_attr attr;
 #endif /* CONFIG_HAVE_HW_BREAKPOINT */
 #ifndef CONFIG_PPC_ADV_DEBUG_REGS
+	bool set_bp = true;
 	struct arch_hw_breakpoint hw_brk;
 #endif
 
@@ -2411,9 +2413,10 @@  static int ptrace_set_debugreg(struct task_struct *task, unsigned long addr,
 	hw_brk.address = data & (~HW_BRK_TYPE_DABR);
 	hw_brk.type = (data & HW_BRK_TYPE_DABR) | HW_BRK_TYPE_PRIV_ALL;
 	hw_brk.len = 8;
+	set_bp = (data) && (hw_brk.type & HW_BRK_TYPE_RDWR);
 #ifdef CONFIG_HAVE_HW_BREAKPOINT
 	bp = thread->ptrace_bps[0];
-	if ((!data) || !(hw_brk.type & HW_BRK_TYPE_RDWR)) {
+	if (!set_bp) {
 		if (bp) {
 			unregister_hw_breakpoint(bp);
 			thread->ptrace_bps[0] = NULL;
@@ -2450,7 +2453,10 @@  static int ptrace_set_debugreg(struct task_struct *task, unsigned long addr,
 		return PTR_ERR(bp);
 	}
 
-#endif /* CONFIG_HAVE_HW_BREAKPOINT */
+#else /* !CONFIG_HAVE_HW_BREAKPOINT */
+	if (set_bp && (!breakpoint_available()))
+		return -ENODEV;
+#endif /* !CONFIG_HAVE_HW_BREAKPOINT */
 	task->thread.hw_brk = hw_brk;
 #else /* CONFIG_PPC_ADV_DEBUG_REGS */
 	/* As described above, it was assumed 3 bits were passed with the data
@@ -2904,6 +2910,9 @@  static long ppc_set_hwdebug(struct task_struct *child,
 	if (child->thread.hw_brk.address)
 		return -ENOSPC;
 
+	if (!breakpoint_available())
+		return -ENODEV;
+
 	child->thread.hw_brk = brk;
 
 	return 1;
diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index 89707354c2..43ff667962 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -741,6 +741,8 @@  static int kvmppc_h_set_mode(struct kvm_vcpu *vcpu, unsigned long mflags,
 	case H_SET_MODE_RESOURCE_SET_DAWR:
 		if (!kvmppc_power8_compatible(vcpu))
 			return H_P2;
+		if (!breakpoint_available())
+			return H_P2;
 		if (mflags)
 			return H_UNSUPPORTED_FLAG_START;
 		if (value2 & DABRX_HYP)
diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
index f31f357b8c..1036aefe56 100644
--- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
+++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
@@ -886,8 +886,14 @@  END_FTR_SECTION_IFCLR(CPU_FTR_ARCH_207S)
 	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
 	mtspr	SPRN_DAWR, r5
 	mtspr	SPRN_DAWRX, r6
+END_FTR_SECTION_IFSET(CPU_FTR_DAWR)
 	mtspr	SPRN_CIABR, r7
 	mtspr	SPRN_TAR, r8
 	ld	r5, VCPU_IC(r4)
@@ -1834,6 +1840,10 @@  BEGIN_FTR_SECTION
 	ld	r6, STACK_SLOT_DAWR(r1)
 	ld	r7, STACK_SLOT_DAWRX(r1)
 	mtspr	SPRN_CIABR, r5
+	/*
+	 * If the DAWR doesn't work, it's ok to write these here as
+	 * this value should always be zero
+	*/
 	mtspr	SPRN_DAWR, r6
 	mtspr	SPRN_DAWRX, r7
 END_FTR_SECTION_IFSET(CPU_FTR_ARCH_207S)
@@ -2512,8 +2522,10 @@  END_FTR_SECTION_IFSET(CPU_FTR_ARCH_207S)
 	clrrdi	r4, r4, 3
 	std	r4, VCPU_DAWR(r3)
 	std	r5, VCPU_DAWRX(r3)
+BEGIN_FTR_SECTION
 	mtspr	SPRN_DAWR, r4
 	mtspr	SPRN_DAWRX, r5
+END_FTR_SECTION_IFSET(CPU_FTR_DAWR)
 	li	r3, 0
 	blr
 
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index 82e1a3ee6e..1f85b0ddd2 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -1297,6 +1297,11 @@  bpt_cmds(void)
 	static const char badaddr[] = "Only kernel addresses are permitted for breakpoints\n";
 	int mode;
 	case 'd':	/* bd - hardware data breakpoint */
+		if (!breakpoint_available()) {
+			printf("Hardware data breakpoint "
+			       "not supported on this cpu\n");
+			break;
+		}
 		mode = 7;
 		cmd = inchar();
 		if (cmd == 'r')