| Message ID | 20180316025456.24036-1-mikey@neuling.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | powerpc: Disable DAWR on POWER9 | expand |
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
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
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
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
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
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')
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(-)