Re: [risu PATCH v2 1/4] s390x: Add basic s390x support to the C code

[Thomas Huth <thuth@redhat.com>]

On 12/09/2023 18.50, Peter Maydell wrote:
> On Tue, 5 Sept 2023 at 12:50, Thomas Huth <thuth@redhat.com> wrote:
>>
>> With these changes, it is now possible to compile the "risu" binary
>> for s390x hosts.
>>
>> Acked-by: Ilya Leoshkevich <iii@linux.ibm.com>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
> 
> 
> 
>> +/* reginfo_init: initialize with a ucontext */
>> +void reginfo_init(struct reginfo *ri, ucontext_t *uc)
>> +{
>> +    int i;
>> +
>> +    memset(ri, 0, sizeof(*ri));
>> +
>> +    ri->faulting_insn = *((uint32_t *) uc->uc_mcontext.psw.addr);
> 
> Here we extract the faulting instruction, assuming it to
> be a 32-bit insn starting at uc_mcontext.psw.addr...
> 
>> +    ri->psw_mask = uc->uc_mcontext.psw.mask;
>> +    ri->psw_addr = uc->uc_mcontext.psw.addr - image_start_address;
>> +
>> +    for (i = 0; i < 16; i++) {
>> +        ri->gregs[i] = uc->uc_mcontext.gregs[i];
>> +    }
>> +
>> +    memcpy(&ri->fpregs, &uc->uc_mcontext.fpregs, sizeof(fpregset_t));
>> +}
> 
>> +void advance_pc(void *vuc)
>> +{
>> +    /*
>> +     * Note: The PSW address already points to the next instruction
>> +     * after we get a SIGILL, so we must not advance it here!
>> +     */
>> +    // ucontext_t *uc = (ucontext_t *) vuc;
>> +    // uc->uc_mcontext.psw.addr += 4;
> 
> ...but here we say that psw.addr points to the instruction
> *after* the faulting one.
> 
> These don't seem like they can both be correct?

Drat, you're right, thanks for the hint! The code in reginfo_init() is 
wrong, it takes the instruction after the illegal one. It was just working 
(wrongly, but without crashing) by accident here since the OP_TESTEND 
instruction was preceeded by an OP_COMPARE instruction :-/

To get the address (and thus opcode) of the faulting instruction, I have to 
use siginfo_t->si_addr instead of using the ucontext_t structure. I will fix 
it in the next version.

  Thomas