Re: [PATCH v3] KVM: s390: vsie: Avoid injecting machine check on signal

[Janosch Frank <frankja@linux.ibm.com>]

On 3/16/26 15:47, Christian Borntraeger wrote:
> 
> 
> Am 16.03.26 um 14:57 schrieb Janosch Frank:
>> On 3/16/26 14:09, Christian Borntraeger wrote:
>>> The recent XFER_TO_GUEST_WORK change resulted in a situation, where the
>>> vsie code would interpret a signal during work as a machine check during
>>> SIE as both use the EINTR return code.
>>> The exit_reason of the sie64a function has nothing to do with the
>>> kvm_run exit_reason. Rename it and define a specific code for machine
>>> checks instead of abusing -EINTR.
>>> rename exit_reason into sie_return to avoid the naming conflict
>>> and change the code flow in vsie.c to have a separate variable for rc
>>> and sie_return.
>>>
>>> Fixes: 2bd1337a1295e ("KVM: s390: Use generic VIRT_XFER_TO_GUEST_WORK functions")
>>> Signed-off-by: Christian Borntraeger <borntraeger@linux.ibm.com>
>>> ---
>>> v1->v2:
>>> instead of a band-aid, make the machine check more direct
>>> v2->v3:
>>> avoid overloading the rc variable in vsie.c
>>>
>>>    arch/s390/include/asm/kvm_host.h   |  3 +++
>>>    arch/s390/include/asm/stacktrace.h |  2 +-
>>>    arch/s390/kernel/asm-offsets.c     |  2 +-
>>>    arch/s390/kernel/entry.S           |  4 ++--
>>>    arch/s390/kernel/nmi.c             |  4 ++--
>>>    arch/s390/kvm/kvm-s390.c           | 15 ++++++++-------
>>>    arch/s390/kvm/vsie.c               |  7 +++++--
>>>    7 files changed, 22 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
>>> index 64a50f0862aa..3039c88daa63 100644
>>> --- a/arch/s390/include/asm/kvm_host.h
>>> +++ b/arch/s390/include/asm/kvm_host.h
>>> @@ -710,6 +710,9 @@ void kvm_arch_crypto_clear_masks(struct kvm *kvm);
>>>    void kvm_arch_crypto_set_masks(struct kvm *kvm, unsigned long *apm,
>>>                       unsigned long *aqm, unsigned long *adm);
>>> +#define SIE64_RETURN_NORMAL    0
>>> +#define SIE64_RETURN_MCCK    1
>>> +
>>>    int __sie64a(phys_addr_t sie_block_phys, struct kvm_s390_sie_block *sie_block, u64 *rsa,
>>>             unsigned long gasce);
>>> diff --git a/arch/s390/include/asm/stacktrace.h b/arch/s390/include/asm/stacktrace.h
>>> index c9ae680a28af..ac3606c3babe 100644
>>> --- a/arch/s390/include/asm/stacktrace.h
>>> +++ b/arch/s390/include/asm/stacktrace.h
>>> @@ -62,7 +62,7 @@ struct stack_frame {
>>>            struct {
>>>                unsigned long sie_control_block;
>>>                unsigned long sie_savearea;
>>> -            unsigned long sie_reason;
>>> +            unsigned long sie_return;
>>>                unsigned long sie_flags;
>>>                unsigned long sie_control_block_phys;
>>>                unsigned long sie_guest_asce;
>>> diff --git a/arch/s390/kernel/asm-offsets.c b/arch/s390/kernel/asm-offsets.c
>>> index e1a5b5b54e4f..fbd26f3e9f96 100644
>>> --- a/arch/s390/kernel/asm-offsets.c
>>> +++ b/arch/s390/kernel/asm-offsets.c
>>> @@ -63,7 +63,7 @@ int main(void)
>>>        OFFSET(__SF_EMPTY, stack_frame, empty[0]);
>>>        OFFSET(__SF_SIE_CONTROL, stack_frame, sie_control_block);
>>>        OFFSET(__SF_SIE_SAVEAREA, stack_frame, sie_savearea);
>>> -    OFFSET(__SF_SIE_REASON, stack_frame, sie_reason);
>>> +    OFFSET(__SF_SIE_RETURN, stack_frame, sie_return);
>>>        OFFSET(__SF_SIE_FLAGS, stack_frame, sie_flags);
>>>        OFFSET(__SF_SIE_CONTROL_PHYS, stack_frame, sie_control_block_phys);
>>>        OFFSET(__SF_SIE_GUEST_ASCE, stack_frame, sie_guest_asce);
>>> diff --git a/arch/s390/kernel/entry.S b/arch/s390/kernel/entry.S
>>> index 4873fe9d891b..5817cb47b2d0 100644
>>> --- a/arch/s390/kernel/entry.S
>>> +++ b/arch/s390/kernel/entry.S
>>> @@ -200,7 +200,7 @@ SYM_FUNC_START(__sie64a)
>>>        stg    %r3,__SF_SIE_CONTROL(%r15)    # ...and virtual addresses
>>>        stg    %r4,__SF_SIE_SAVEAREA(%r15)    # save guest register save area
>>>        stg    %r5,__SF_SIE_GUEST_ASCE(%r15)    # save guest asce
>>> -    xc    __SF_SIE_REASON(8,%r15),__SF_SIE_REASON(%r15) # reason code = 0
>>> +    xc    __SF_SIE_RETURN(8,%r15),__SF_SIE_RETURN(%r15) # return code = 0
>>>        mvc    __SF_SIE_FLAGS(8,%r15),__TI_flags(%r14) # copy thread flags
>>>        lmg    %r0,%r13,0(%r4)            # load guest gprs 0-13
>>>        mvi    __TI_sie(%r14),1
>>> @@ -237,7 +237,7 @@ SYM_INNER_LABEL(sie_exit, SYM_L_GLOBAL)
>>>        xgr    %r4,%r4
>>>        xgr    %r5,%r5
>>>        lmg    %r6,%r14,__SF_GPRS(%r15)    # restore kernel registers
>>> -    lg    %r2,__SF_SIE_REASON(%r15)    # return exit reason code
>>> +    lg    %r2,__SF_SIE_RETURN(%r15)    # return sie return code
>>>        BR_EX    %r14
>>>    SYM_FUNC_END(__sie64a)
>>>    EXPORT_SYMBOL(__sie64a)
>>> diff --git a/arch/s390/kernel/nmi.c b/arch/s390/kernel/nmi.c
>>> index a55abbf65333..fc1f775e4517 100644
>>> --- a/arch/s390/kernel/nmi.c
>>> +++ b/arch/s390/kernel/nmi.c
>>> @@ -487,8 +487,8 @@ void notrace s390_do_machine_check(struct pt_regs *regs)
>>>        mcck_dam_code = (mci.val & MCIC_SUBCLASS_MASK);
>>>        if (test_cpu_flag(CIF_MCCK_GUEST) &&
>>>        (mcck_dam_code & MCCK_CODE_NO_GUEST) != mcck_dam_code) {
>>> -        /* Set exit reason code for host's later handling */
>>> -        *((long *)(regs->gprs[15] + __SF_SIE_REASON)) = -EINTR;
>>> +        /* Set sie return code for host's later handling */
>>> +        *((long *)(regs->gprs[15] + __SF_SIE_RETURN)) = SIE64_RETURN_MCCK;
>>
>> We don't need to cast this to long anymore, do we?
>> Since sie_return is ulong, and we if we stay with positive numbers ulong would make the most sense.
>>
>> Although I'd prefer a cast to stack_frame but I'm unsure if that would make it more readable if we stay with a one line change.
> something like
> -               *((long *)(regs->gprs[15] + __SF_SIE_RETURN)) = SIE64_RETURN_MCCK;
> +               ((struct stack_frame *) regs->gprs[15])->sie_return = SIE64_RETURN_MCCK;>
> What do you think?

Yep

> 
> This reminds me that I should probably also make the local variable in vsie.c unsigned long instead of int.
> 
>