Re: [patch 02/11] x86 architecture implementation of HardwareBreakpoint interfaces

[Alan Stern]

On Tue, 10 Mar 2009, Ingo Molnar wrote:

> * prasad@xxxxxxxxxxxxxxxxxx <prasad@xxxxxxxxxxxxxxxxxx> wrote:
> 
> > +/*
> > + * Handle debug exception notifications.
> > + */
> > +
> > +int __kprobes hw_breakpoint_handler(struct die_args *args)
> > +{
> > +	struct cpu_hw_breakpoint *chbi;
> > +	int i;
> > +	struct hw_breakpoint *bp;
> > +	struct thread_hw_breakpoint *thbi = NULL;
> > +
> > +	/* The DR6 value is stored in args->err */
> > +#define DR6	(args->err)
> 
> that's ugly - what's wrong with an old-fashioned "int db6 = 
> args->err" type of approach?

Yes, it is ugly.  It was a holdover from an earlier version, and in 
fact it's likely to change in the future to become even more ugly.  But 
for now, you're right -- a simple assignment would be better.

> > +++ linux-2.6-tip.hbkpt/arch/x86/include/asm/hw_breakpoint.h
> > @@ -0,0 +1,132 @@
> > +#ifndef	_I386_HW_BREAKPOINT_H
> > +#define	_I386_HW_BREAKPOINT_H
> > +
> > +#ifdef	__KERNEL__
> > +#define	__ARCH_HW_BREAKPOINT_H
> > +
> > +struct arch_hw_breakpoint {
> > +	char		*name; /* Contains name of the symbol to set bkpt */
> > +	unsigned long	address;
> > +	u8		len;
> > +	u8		type;
> > +} __attribute__((packed));
> 
> hm, why packed and why u8 ? We dont expose this to user-space, 
> do we? (if yes then 'unsigned long' is wrong and __KERNEL__ is 
> wrong as well)

I can't remember why this was made packed; there doesn't seem to be any 
important reason for it.  The structure is not exposed to userspace.  
The len and type fields are u8 because they contain values no larger 
than 255.

> > +#include <linux/kdebug.h>
> > +#include <asm-generic/hw_breakpoint.h>
> > +
> > +/* HW breakpoint accessor routines */
> > +static inline const void *hw_breakpoint_get_kaddress(struct hw_breakpoint *bp)
> > +{
> > +	return (const void *) bp->info.address;
> > +}
> > +
> > +static inline const void __user *hw_breakpoint_get_uaddress
> > +						(struct hw_breakpoint *bp)
> > +{
> > +	return (const void __user *) bp->info.address;
> > +}
> > +
> > +static inline unsigned hw_breakpoint_get_len(struct hw_breakpoint *bp)
> > +{
> > +	return bp->info.len;
> > +}
> > +
> > +static inline unsigned hw_breakpoint_get_type(struct hw_breakpoint *bp)
> > +{
> > +	return bp->info.type;
> > +}
> 
> why this redirection, why dont just use the structure as-is? If 
> there's any arch weirdness then that arch should have 
> arch-special accessors - not the generic code.

These _are_ the arch-specific accessors.  Look at the filename:
arch/x86/include/asm/hw_breakpoint.h.

> > +	int			num_installed;	/* Number of installed bps */
> > +	unsigned		gennum;		/* update-generation number */
> 
> i suspect the gennum we can get rid of if we get rid of the 
> notion of priorities, right?

No.  gennum has nothing to do with priorities.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/