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

[Alexander Graf <agraf@suse.de>]

On 18.06.14 06:39, Bharat.Bhushan@freescale.com wrote:
>
>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf@suse.de]
>> Sent: Tuesday, June 17, 2014 4:14 PM
>> To: Bhushan Bharat-R65777; qemu-ppc@nongnu.org; qemu-devel@nongnu.org
>> Subject: Re: [PATCH 3/3 v2] ppc debug: Add debug stub support
>>
>>
>> On 17.06.14 12:40, Bharat.Bhushan@freescale.com wrote:
>>>> -----Original Message-----
>>>> From: Alexander Graf [mailto:agraf@suse.de]
>>>> Sent: Tuesday, June 17, 2014 3:20 PM
>>>> To: Bhushan Bharat-R65777; qemu-ppc@nongnu.org; qemu-devel@nongnu.org
>>>> Subject: Re: [PATCH 3/3 v2] ppc debug: Add debug stub support
>>>>
>>>>
>>>> On 17.06.14 11:14, Bharat.Bhushan@freescale.com wrote:
>>>>>> -----Original Message-----
>>>>>> From: Alexander Graf [mailto:agraf@suse.de]
>>>>>> Sent: Tuesday, June 17, 2014 1:46 PM
>>>>>> To: Bhushan Bharat-R65777; qemu-ppc@nongnu.org;
>>>>>> qemu-devel@nongnu.org
>>>>>> 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 <Bharat.Bhushan@freescale.com>
>>>>>>> ---
>>>>>>> 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