Re: [PATCH 4/9] arm: Split breakpoint validation into "check" and "commit"

[Andy Lutomirski]

On Wed, May 9, 2018 at 4:33 AM Mark Rutland <mark.rutland@xxxxxxx> wrote:

> On Tue, May 08, 2018 at 12:13:23PM +0100, Mark Rutland wrote:
> > Hi Frederick,
> >
> > On Sun, May 06, 2018 at 09:19:50PM +0200, Frederic Weisbecker wrote:
> > > The breakpoint code mixes up attribute check and commit into a single
> > > code entity. Therefore the validation may return an error due to
> > > incorrect atributes while still leaving halfway modified architecture
> > > breakpoint struct.
> > >
> > > Prepare fox fixing this misdesign and separate both logics.
> >
> > Could you elaborate on what the problem is? I would have expected that
> > when arch_build_bp_info() returns an error code, we wouldn't
> > subsequently use the arch_hw_breakpoint information. Where does that
> > happen?

>  From digging, I now see that this is a problem when
> modify_user_hw_breakpoint() is called on an existing breakpoint. It
> would be nice to mention that in the commit message.

> > I also see that the check and commit hooks have to duplicate a
> > reasonable amount of logic, e.g. the switch on bp->attr.type. Can we
> > instead refactor the existing arch_build_bp_info() hooks to use a
> > temporary arch_hw_breakpoint, and then struct assign it after all the
> > error cases, > e.g.
> >
> > static int arch_build_bp_info(struct perf_event *bp)
> > {
> >       struct arch_hw_breakpoint hbp;
> >
> >       if (some_condition(bp))
> >               hbp->field = 0xf00;
> >
> >       switch (bp->attr.type) {
> >       case FOO:
> >               return -EINVAL;
> >       case BAR:
> >               hbp->other_field = 7;
> >               break;
> >       };
> >
> >       if (failure_case(foo))
> >               return err;
> >
> >       *counter_arch_bp(bp) = hbp;
> > }
> >
> > ... or is that also problematic?

> IIUC, this *would* work, but it is a little opaque.

> Perhaps we could explicitly pass the temporary arch_hw_breakpoint in,
> and have the core code struct-assign it after checking for errors?

Hmm, maybe.  OTOH, I'm not really convinced that arch_hw_breakpoint is even
needed.  x86 at least could probably just regenerate the DRn and DR7 bits
on the fly as needed rather than caching them with basically no loss in
performance.  I completely agree that reducing duplication would be nice.