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

[address@hidden]

> -----Original Message-----
> From: Alexander Graf [mailto:address@hidden
> Sent: Tuesday, June 24, 2014 5:02 PM
> To: Bhushan Bharat-R65777; address@hidden; address@hidden
> Subject: Re: [PATCH 3/3 v2] ppc debug: Add debug stub support
> 
> 
> 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.

V3 version of path is based on this :)

Thanks
-Bharat

> 
> 
> 
> Alex