Re: [Qemu-ppc] [PATCH 3/3 v2] ppc debug: Add debug stub support

[Alexander Graf]

On 18.06.14 06:39, address@hidden wrote:
> > -----Original Message-----
> > From: Alexander Graf [mailto:address@hidden
> > Sent: Tuesday, June 17, 2014 4:14 PM
> > To: Bhushan Bharat-R65777; address@hidden; address@hidden
> > Subject: Re: [PATCH 3/3 v2] ppc debug: Add debug stub support
> >
> >
> > On 17.06.14 12:40, address@hidden wrote:
> > > > -----Original Message-----
> > > > From: Alexander Graf [mailto:address@hidden
> > > > Sent: Tuesday, June 17, 2014 3:20 PM
> > > > To: Bhushan Bharat-R65777; address@hidden; address@hidden
> > > > Subject: Re: [PATCH 3/3 v2] ppc debug: Add debug stub support
> > > >
> > > >
> > > > On 17.06.14 11:14, address@hidden wrote:
> > > > > > -----Original Message-----
> > > > > > From: Alexander Graf [mailto:address@hidden
> > > > > > Sent: Tuesday, June 17, 2014 1:46 PM
> > > > > > To: Bhushan Bharat-R65777; address@hidden;
> > > > > > address@hidden
> > > > > > Subject: Re: [PATCH 3/3 v2] ppc debug: Add debug stub support
> > > > > >
> > > > > >
> > > > > > On 17.06.14 09:08, Bharat Bhushan wrote:
> > > > > > > This patch adds software breakpoint, hardware breakpoint and
> > > > > > > hardware watchpoint support for ppc. If the debug interrupt is not
> > > > > > > handled then this is injected to guest.
> > > > > > >
> > > > > > > Signed-off-by: Bharat Bhushan <address@hidden>
> > > > > > > ---
> > > > > > > v1->v2:
> > > > > > >      - factored out e500 specific code based on exception model
> > > > > > POWERPC_EXCP_BOOKE.
> > > > > > >      - Not supporting ppc440
> > > > > > >
> > > > > > >      hw/ppc/e500.c        |   3 +
> > > > > > >      target-ppc/kvm.c     | 355
> > > > ++++++++++++++++++++++++++++++++++++++++++++++---
> > > > > > --
> > > > > > >      target-ppc/kvm_ppc.h |   1 +
> > > > > > >      3 files changed, 330 insertions(+), 29 deletions(-)
> > > > > > >
> > > > > > > diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c index a973c18..47caa84
> > > > > > > 100644
> > > > > > > --- a/hw/ppc/e500.c
> > > > > > > +++ b/hw/ppc/e500.c
> > > > > > > @@ -853,6 +853,9 @@ void ppce500_init(MachineState *machine,
> > > > > > > PPCE500Params
> > > > > > *params)
> > > > > > >          if (kvm_enabled()) {
> > > > > > >              kvmppc_init();
> > > > > > >          }
> > > > > > > +
> > > > > > > +    /* E500 supports 2 h/w breakpoints and 2 watchpoints */
> > > > > > > +    kvmppc_hw_breakpoint_init(2, 2);
> > > > > > This does not belong into the machine file.
> > > > > What about calling this from init_proc_e500() in target-ppc/translate_init.c
> > ?
> > > > I think it makes sense to leave it in KVM land. Why not do it lazily
> > > > on insert_hw_breakpoint?
> > > You mean setting in kvm_arch_insert_hw_breakpoint() when called first time;
> > something like:
> > >       static bool init = 0;
> > >
> > >       if (!init) {
> > >           if (env->excp_model == POWERPC_EXCP_BOOKE) {
> > >               max_hw_breakpoint = 2;
> > >               max_hw_watchpoint = 2;
> > >           } else
> > >            // Add for book3s max_hw_watchpoint = 1;
> > >          }
> > >          init = 1;
> > >       }
> > I would probably reuse max_hw_breakpoint as a hint whether it's initialized and
> > put all of this into a small function, but yes :).
> Ahh, we cannot do this in kvm_arch_insert_hw_breakpoint() as we can not get 
> "env" reference in this function. Prototype of this is:
> int kvm_arch_insert_hw_breakpoint(target_ulong addr, target_ulong len, int 
> type);

Just use first_cpu? :)

> I will suggest that we initialize this from kvm_arch_init_vcpu(). This way we 
> are still in KVM zone.

That works too, yes.



Alex