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

[David Hildenbrand]

On 30.11.2017 12:01, Thomas Huth wrote:
> 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).

The good thing is, it won't break KVM but only TCG. And it has been
broken in TCG for years now without anybody noticing it.

> 
> 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?

Oh god no, no such strange calling conventions from the same 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.

I don't really like that. And there would be a possibility for a race.
So a no from my side.

> 
> 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?

I also thought about this, but adding two parameters to every call is
even uglier. Not talking about having to construct special structs to
pass through the data. Please not that the TPI handler I am about to
introduce will also require to revert stuff in case there was an exception.

Another option I thought about was checking in the caller if EXC_PGM has
been set. But missing to add such a check is even easier.

I'd prefer to keep it as is, but if there are other opinions/ideas,
please speak up.

Thanks for having a look!

> 
>  Thomas
> 


-- 

Thanks,

David / dhildenb