Re: [qemu-s390x] [PATCHv1 12/14] target/s390x: convert to TranslatorOps

[David Hildenbrand]

On 01.03.2018 23:53, Emilio G. Cota wrote:
> Note: I looked into dropping dc->do_debug. However, I don't see
> an easy way to do it given that TOO_MANY is also valid
> when we just translate more than max_insns. Thus, the check
> for do_debug in "case DISAS_PC_CC_UPDATED" would still need
> additional state to know whether or not we came from
> breakpoint_check.

Looks much cleaner! The do_debug() thing is fine for now.


> -        if (unlikely(cpu_breakpoint_test(cs, dc.base.pc_next, BP_ANY))) {
> -            status = DISAS_PC_STALE;
> -            do_debug = true;
> -            /* The address covered by the breakpoint must be included in
> -               [tb->pc, tb->pc + tb->size) in order to for it to be
> -               properly cleared -- thus we increment the PC here so that
> -               the logic setting tb->size below does the right thing.  */
> -            dc.base.pc_next += 2;
> -            break;
> -        }
> +    dc->base.is_jmp = DISAS_PC_STALE;
> +    dc->do_debug = true;
> +    /* The address covered by the breakpoint must be included in
> +       [tb->pc, tb->pc + tb->size) in order to for it to be
> +       properly cleared -- thus we increment the PC here so that
> +       the logic setting tb->size below does the right thing.  */
> +    dc->base.pc_next += 2;

This comment is now wrong. ("the logic in translator_loop()" ?)

Reviewed-by: David Hildenbrand <address@hidden>


Gave this series a quick test:

kvm-unit-tests pass (you need to rebase to have all tests passing),
fedora 27 boots

-- 

Thanks,

David / dhildenb