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

[Cornelia Huck]

On Thu,  1 Mar 2018 17:53:56 -0500
"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.
> 
> Cc: David Hildenbrand 
> Cc: Cornelia Huck 
> Cc: Alexander Graf 
> Cc: qemu-s3...@nongnu.org
> Signed-off-by: Emilio G. Cota 
> ---
>  target/s390x/translate.c | 162 
> +++
>  1 file changed, 80 insertions(+), 82 deletions(-)

Acked-by: Cornelia Huck 

Re: [Qemu-devel] [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 


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