Re: [PATCH 1/3] perf/x86/amd: AMD support for bp_len > HW_BREAKPOINT_LEN_8

[Jacob Shin <jacob.shin@amd.com>]

On Sat, Apr 27, 2013 at 05:05:10PM +0200, Oleg Nesterov wrote:
> On 04/26, Jacob Shin wrote:
> >
> > Implement hardware breakpoint address mask for AMD Family 16h and
> > above processors. CPUID feature bit indicates hardware support for
> > DRn_ADDR_MASK MSRs. These masks further qualify DRn/DR7 hardware
> > breakpoint addresses to allow matching of larger addresses ranges.
> 
> Imho, looks good.
> 
> Just one nit and one question below.
> 
> > @@ -163,29 +165,8 @@ void arch_uninstall_hw_breakpoint(struct perf_event *bp)
> >  	*dr7 &= ~__encode_dr7(i, info->len, info->type);
> ...
> > +	if (info->mask)
> > +		set_dr_addr_mask(0, i);
> 
> I agree we should clear addr_mask anyway.
> 
> But I am just curious, what if we do not? I mean what will the hardware
> do if this breakpoint was already disabled but the mask wasn't cleared?

Oh, it is fine if we don't and we are not using breakpoints, however I
was trying to account for per-thread events sharing the same DR
register, in that case we don't want previous event's mask value still
in the MSR.

So it was either clear on uninstall, or unconditionally set (even if
the new event's mask should be 0)

> 
> > @@ -314,11 +300,14 @@ int arch_validate_hwbkpt_settings(struct perf_event *bp)
> >  	if (ret)
> >  		return ret;
> >  
> > -	ret = -EINVAL;
> > -
> >  	switch (info->len) {
> >  	case X86_BREAKPOINT_LEN_1:
> >  		align = 0;
> > +		if (info->mask) {
> > +			if (!cpu_has_bpext)
> > +				return -EOPNOTSUPP;
> > +			align = info->mask;
> 
> OK. But it seems we need a CONFIG_CPU_SUP_AMD-dependant helper for
> this cpu_has_bpext check? (like we have the nop set_dr_addr_mask()
> variant if !CONFIG_CPU_SUP_AMD).
> 
> Suppose that the kernel was compiled without CONFIG_CPU_SUP_AMD.
> Then perf_event_open(attr => { .bp_len == 16 }) will succeed, but
> this breakpoint won't actually work as expected?

Well, on non-AMD, even if we have CONFIG_CPU_SUP_AMD=y or not,
cpu_has_bpext (x86 CPUID check) will fail.

On AMD, if we don't have CONFIG_CPU_SUP_AMD=y then we can't even boot.
So I think ... its fine.


> 
> Oleg.
> 
>