Re: [PATCH] accel/tcg/user-exec: Fix read-modify-write of code on s390 hosts

[Richard Henderson <richard.henderson@linaro.org>]

On 8/3/21 9:54 AM, Ilya Leoshkevich wrote:
>       /* ??? On linux, the non-rt signal handler has 4 (!) arguments instead
> -       of the normal 2 arguments.  The 3rd argument contains the "int_code"
> -       from the hardware which does in fact contain the is_write value.
> +       of the normal 2 arguments.  The 4th argument contains the "Translation-
> +       Exception Identification for DAT Exceptions" from the hardware (aka
> +       "int_parm_long"), which does in fact contain the is_write value.
>          The rt signal handler, as far as I can tell, does not give this value
> -       at all.  Not that we could get to it from here even if it were.  */
> -    /* ??? This is not even close to complete, since it ignores all
> -       of the read-modify-write instructions.  */
> +       at all.  Not that we could get to it from here even if it were.
> +       So fall back to parsing instructions.  Treat read-modify-write ones as
> +       writes, which is not fully correct, but for tracking self-modifying code
> +       this is better than treating them as reads.  Checking si_addr page flags
> +       might be a viable improvement, albeit a racy one.  */
> +    /* ??? This is not even close to complete.  */

You should have gotten a checkpatch warning here.
Just convert the comment to

   /*
    * this style
    */

>       pinsn = (uint16_t *)pc;
>       switch (pinsn[0] >> 8) {
>       case 0x50: /* ST */
>       case 0x42: /* STC */
>       case 0x40: /* STH */
> +    case 0xba: /* CS */
> +    case 0xbb: /* CDS */
> +    case 0xc8: /* CSST */

CSST is format SSF; you're not checking enough bits to distinguish from LOAD PAIR DISJOINT.

Otherwise, looks good.

r~