Re: [PATCH v2] s390x/tcg: MVCL: Exit to main loop if requested

["Alex Bennée" <alex.bennee@linaro.org>]

David Hildenbrand <david@redhat.com> writes:

> On 02.10.19 21:34, Richard Henderson wrote:
>> On 10/2/19 9:47 AM, Richard Henderson wrote:
>>> There is still the special case of EXECUTE of MVCL, which I suspect must have
>>> some failure mode that we're not considering -- the setting and clearing of
>>> ex_value can't help.  I have a suspicion that we need to special case that
>>> within helper_ex, just so that ex_value doesn't enter into it.
>>
>> I had a walk and a think.  I now believe that we're ok:
>>
>> (1) TB with EXECUTE runs, at address Ae
>>
>>     - env->psw_addr stored with Ae.
>>     - helper_ex() runs, memory address Am computed
>>       from D2a(X2a,B2a) or from psw.addr+RI2.
>>     - env->ex_value stored with memory value modified by R1a
>>
>> (2) TB of executee runs,
>>
>>     - env->ex_value stored with 0.
>>     - helper_mvcl() runs, using and updating R1b, R1b+1, R2b, R2b+1.
>>
>> (3a) helper_mvcl() completes,
>>
>>      - TB of executee continues, psw.addr += ilen.
>>      - Next instruction is the one following EXECUTE.
>
> Right, and whenever we inject an interrupt (e.g., access exception or an
> asynchronous one), we have to use addr=ilen of EXECUTE, not of the
> execute target. And that is guaranteed to reside in env->psw_addr/rewind
> info
>
>>
>> (3b) helper_mvcl() exits to main loop,
>>
>>      - cpu_loop_exit_restore() unwinds psw.addr = Ae.
>>      - Next instruction is the EXECUTE itself...
>>      - goto 1.
>
> Sounds about right to me. Thanks for verifying.
>
>>
>> If we can agree that the result is undefined if registers R1a, X2a, B2a overlap
>> R1b, R1b+1, R2b, R2b+1, or if the memory address Am is modified by the
>> interrupted MVCL, then we're ok.
>
> So the Programming Note 5. for EXECUTE says:
>
> When an interruptible instruction is made the tar-
> get of an execute-type instruction, the program
> normally should not designate any register
> updated by the interruptible instruction as the R1,
> X2, or B2 register for EXECUTE or R1 register for
> EXECUTE RELATIVE LONG. Otherwise, on
> resumption of execution after an interruption, or if
> the instruction is refetched without an interrup-
> tion, the updated values of these registers will be
> used in the execution of the execute-type instruc-
> tion. Similarly, the program should normally not
> let the destination field in storage of an interrupt-
> ible instruction include the location of an execute-
> type instruction, since the new contents of the
> location may be interpreted when resuming exe-
> cution.
>
> So, if I read correctly
> - The updated register content *will* be used
> - The updated memory content *may* be used
>
> However, also "the program normally should not"/"the program should
> normally not" which to me sounds like "just don't do it", in which case
> we are fine.
>
> So shall we leave this patch as-is (adding a summary of what you
> explained to the description) or shall we somehow factor out the
> TCG-internal-thingy check?

It would be nice to avoid this internal detail in the guest specific
code but I can live with it for now.

Acked-by: Alex Bennée <alex.bennee@linaro.org>

--
Alex Bennée