Re: [qemu-s390x] [PATCH v2 for-2.12 07/16] s390x: handle exceptions during s390_cpu_virt_mem_rw() correctly (TCG)

[Thomas Huth]

On 29.11.2017 21:26, David Hildenbrand wrote:
> s390_cpu_virt_mem_rw() must always return, so callers can react on
> an exception (e.g. see ioinst_handle_stcrw()).
> 
> However, for TCG we always have to exit the cpu loop (and restore the
> cpu state before that) if we injected a program interrupt. So let's
> introduce and use s390_cpu_virt_mem_handle_exc() in code that is not
> purely KVM.
> 
> Directly pass the retaddr we already have available in these functions.
> 
> Signed-off-by: David Hildenbrand <address@hidden>
> ---
>  hw/s390x/s390-pci-inst.c  |  7 +++++++
>  target/s390x/cpu.h        |  1 +
>  target/s390x/ioinst.c     | 20 +++++++++++++++++---
>  target/s390x/mmu_helper.c | 14 ++++++++++++++
>  4 files changed, 39 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
> index 8123705dfd..6f41083244 100644
> --- a/hw/s390x/s390-pci-inst.c
> +++ b/hw/s390x/s390-pci-inst.c
> @@ -163,6 +163,7 @@ int clp_service_call(S390CPU *cpu, uint8_t r2, uintptr_t 
> ra)
>      }
>  
>      if (s390_cpu_virt_mem_read(cpu, env->regs[r2], r2, buffer, 
> sizeof(*reqh))) {
> +        s390_cpu_virt_mem_handle_exc(cpu, ra);
>          return 0;
>      }
>      reqh = (ClpReqHdr *)buffer;
> @@ -174,6 +175,7 @@ int clp_service_call(S390CPU *cpu, uint8_t r2, uintptr_t 
> ra)
>  
>      if (s390_cpu_virt_mem_read(cpu, env->regs[r2], r2, buffer,
>                                 req_len + sizeof(*resh))) {
> +        s390_cpu_virt_mem_handle_exc(cpu, ra);
>          return 0;
>      }
>      resh = (ClpRspHdr *)(buffer + req_len);
> @@ -189,6 +191,7 @@ int clp_service_call(S390CPU *cpu, uint8_t r2, uintptr_t 
> ra)
>  
>      if (s390_cpu_virt_mem_read(cpu, env->regs[r2], r2, buffer,
>                                 req_len + res_len)) {
> +        s390_cpu_virt_mem_handle_exc(cpu, ra);
>          return 0;
>      }
[...]

Having to change all these spots is kind of ugly ... and I guess it will also
be quite error-prone in the future (if someone is developing new code under
KVM only and then forgets to add these handlers for TCG).

Can't you do that simply always at the end of s390_cpu_virt_mem_rw() instead,
when the function has been called in non-checking mode (i.e. hostbuf != NULL)?
Then we only need the extra-dance in places where we call the _check function?

> @@ -212,9 +214,12 @@ void ioinst_handle_stcrw(S390CPU *cpu, uint32_t ipb, 
> uintptr_t ra)
>  
>      if (s390_cpu_virt_mem_write(cpu, addr, ar, &crw, sizeof(crw)) == 0) {
>          setcc(cpu, cc);
> -    } else if (cc == 0) {
> -        /* Write failed: requeue CRW since STCRW is a suppressing 
> instruction */
> -        css_undo_stcrw(&crw);
> +    } else {
> +        if (cc == 0) {
> +            /* Write failed: requeue CRW since STCRW is suppressing */
> +            css_undo_stcrw(&crw);
> +        }
> +        s390_cpu_virt_mem_handle_exc(cpu, ra);
>      }
>  }

That hunk would then need to do _check first and in case there is a failure,
call css_undo_stcrw() before the s390_cpu_virt_mem_handle_exc() function, I 
think.

Alternatively, the s390_cpu_virt_mem_rw() function could get an additional 
parameter
that points to a callback function which is used for "clean-up" right before the
cpu_loop_exit ... and in this case the callback function would contain the
css_undo_stcrw().

What do you think?

 Thomas