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

From: Ingo Molnar
Date: Tue Mar 10 2009 - 11:19:18 EST



* Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:

> 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.

I very well know which file this is, you need to read my reply
again.

These are very generic-sounding fields and they should not be
hidden via pointless wrappers by the generic code. Dont let the
tail wag the dog. If there's architecture weirdness that does
not fit the generic code, then _that_ architecture should have
the ugliness - not the generic code. (note that these accessors
are used by the generic code so the uglification spreads there)

> > > + 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.

Well it's introduced because we have a priority-sorted list of
breakpoints not an array. A list needs to be maintained and when
updated it's reloaded. I was thinking about possibly getting rid
of that list complication and go back to the simpler array. But
it's hard because the lifetime of a kernel space breakpoint
spans context-switches so there has to be separation.

Ingo
--
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/