I noticed that __set_breakpoint in process.c calls set_dabr if
CPU_FTR_DAWR is not set. One case which could cause this to happen is if
perf events are not configured (and CONFIG_HW_BREAKPOINT is not set),
and someone calls ptrace with SET_DEBUGREG and data with a non-zero
address and 0 in the RDWR bits. In this case set_bp will be false,
thread.hw_brk will be set, and __switch_to will call __set_breakpoint,
which will call set_dabr. I'm not sure what happens if mtspr with DARN
is called on a P8.
With respect to setting .features, from the V1 discussion:
> 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.
This would make existing GDB releases work nicely, but it should
probably be fixed in GDB.
When .features is 0, GDB ignores the rest of the info returned by
GETHWDBGINFO and assumes there will be at least one watchpoint register,
which leads to the behavior you described, and doesn't seem correct
(on the GDB side).
Thanks!
Pedro
Michael Neuling <mikey@xxxxxxxxxxx> writes:
> 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 advertise zero breakpoints on POWER9 via the
> PPC_PTRACE_GETHWDBGINFO call. This results in GDB falling back to
> software emulation of the watchpoint (which is slow).
>
> 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@xxxxxxxxxxx>
> ---
> v2:
> Move to CPU feature quirk from Nick's
> dbginfo.num_data_bps = 0 if !breakpoint_available() from Pedro
> ---
> 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 | 3 +++
> arch/powerpc/kernel/hw_breakpoint.c | 3 +++
> arch/powerpc/kernel/process.c | 11 +++++++++++
> arch/powerpc/kernel/ptrace.c | 18 +++++++++++++++---
> arch/powerpc/kvm/book3s_hv.c | 2 ++
> arch/powerpc/kvm/book3s_hv_rmhandlers.S | 12 ++++++++++++
> arch/powerpc/xmon/xmon.c | 5 +++++
> 10 files changed, 55 insertions(+), 6 deletions(-)
>
> 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..941a5fbd8d 100644
> --- a/arch/powerpc/kernel/dt_cpu_ftrs.c
> +++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
> @@ -709,6 +709,9 @@ static __init void cpufeatures_cpu_quirks(void)
> cur_cpu_spec->cpu_features |= CPU_FTR_POWER9_DD1;
> else if ((version & 0xffffefff) == 0x004e0201)
> cur_cpu_spec->cpu_features |= CPU_FTR_POWER9_DD2_1;
> +
> + if ((version & 0xffff0000) == 0x004e0000)
> + cur_cpu_spec->cpu_features &= ~(CPU_FTR_DAWR);
> }
>
> static void __init cpufeatures_setup_finished(void)
> 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..92e03418fd 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;
> @@ -3052,7 +3061,10 @@ long arch_ptrace(struct task_struct *child, long request,
> #endif
> #else /* !CONFIG_PPC_ADV_DEBUG_REGS */
> dbginfo.num_instruction_bps = 0;
> - dbginfo.num_data_bps = 1;
> + if (breakpoint_available())
> + dbginfo.num_data_bps = 1;
> + else
> + dbginfo.num_data_bps = 0;
> dbginfo.num_condition_regs = 0;
> #ifdef CONFIG_PPC64
> dbginfo.data_bp_alignment = 8;
> 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')
> --
> 2.14.1
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
