Re: [Qemu-arm] [PATCH v3 1/2] cpu: Add callback to check architectural watchpoint match

[Peter Maydell]

Andreas -- can I nudge you to say your preferences on whether
QOM CPU methods should have wrapper functions, please?
I think this patchset is otherwise ready to apply.

thanks
-- PMM

On 2 February 2016 at 12:23, Peter Maydell <address@hidden> wrote:
> On 31 January 2016 at 16:15, Sergey Fedorov <address@hidden> wrote:
>> When QEMU watchpoint matches, that is not definitely an architectural
>> watchpoint match yet. If it is a stop-before-access watchpoint then that
>> is hardly possible to ignore it after throwing a TCG exception.
>>
>> A special callback is introduced to check for architectural watchpoint
>> match before raising a TCG exception.
>>
>> Signed-off-by: Sergey Fedorov <address@hidden>
>
> This looks OK to me, but there's one QOM CPU style question
> I'd like Andreas's view on.
>
>> @@ -2024,6 +2024,7 @@ static const MemoryRegionOps notdirty_mem_ops = {
>>  static void check_watchpoint(int offset, int len, MemTxAttrs attrs, int 
>> flags)
>>  {
>>      CPUState *cpu = current_cpu;
>> +    CPUClass *cc = CPU_GET_CLASS(cpu);
>>      CPUArchState *env = cpu->env_ptr;
>>      target_ulong pc, cs_base;
>>      target_ulong vaddr;
>> @@ -2049,6 +2050,11 @@ static void check_watchpoint(int offset, int len, 
>> MemTxAttrs attrs, int flags)
>>              wp->hitaddr = vaddr;
>>              wp->hitattrs = attrs;
>>              if (!cpu->watchpoint_hit) {
>> +                if (wp->flags & BP_CPU &&
>> +                    !cc->debug_check_watchpoint(cpu, wp)) {
>
> At least some of the QOM CPU methods have wrapper functions
> (eg cpu_set_pc(), cpu_unaligned_access()) that just bundle up
> the CPU_GET_CLASS and method invocation). Should new methods
> like the debug_check_watchpoint() introduced by this patch have
> that kind of wrapper function, or is it optional?
>
> (There also seems to be a mix of "implement default/common
> behaviour in a common method implementation" vs "implement
> it in the wrapper function if the method pointer is NULL".
> I think the former, as done in this patch, is probably nicer
> but again would defer to Andreas.)
>
> thanks
> -- PMM