Re: [PATCH] KVM: s390: Add capability that forwards operation exceptions

[Thomas Huth <thuth@redhat.com>]

On 31/10/2025 09.45, Janosch Frank wrote:
> On 10/30/25 08:10, Thomas Huth wrote:
>> On 29/10/2025 14.04, Janosch Frank wrote:
>>> Setting KVM_CAP_S390_USER_OPEREXEC will forward all operation
>>> exceptions to user space. This also includes the 0x0000 instructions
>>> managed by KVM_CAP_S390_USER_INSTR0. It's helpful if user space wants
>>> to emulate instructions which do not (yet) have an opcode.
>>>
>>> While we're at it refine the documentation for
>>> KVM_CAP_S390_USER_INSTR0.
>>>
>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ...
>>> +7.45 KVM_CAP_S390_USER_OPEREXEC
>>> +----------------------------
>>> +
>>> +:Architectures: s390
>>> +:Parameters: none
>>> +
>>> +When this capability is enabled KVM forwards all operation exceptions
>>> +that it doesn't handle itself to user space. This also includes the
>>> +0x0000 instructions managed by KVM_CAP_S390_USER_INSTR0. This is
>>> +helpful if user space wants to emulate instructions which do not (yet)
>>> +have an opcode.
>>
>> "which do not (yet) have an opcode" sounds a little bit weird. Maybe rather:
>> "which are not (yet) implemented in the current CPU" or so?
> 
> How about:
> ...which are not (yet) implemented in hardware.

Sounds good!


>>> @@ -921,6 +922,12 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm, struct 
>>> kvm_enable_cap *cap)
>>>            VM_EVENT(kvm, 3, "ENABLE: CAP_S390_CPU_TOPOLOGY %s",
>>>                 r ? "(not available)" : "(success)");
>>>            break;
>>> +    case KVM_CAP_S390_USER_OPEREXEC:
>>> +        VM_EVENT(kvm, 3, "%s", "ENABLE: CAP_S390_USER_OPEREXEC");
>>> +        kvm->arch.user_operexec = 1;
>>> +        icpt_operexc_on_all_vcpus(kvm);
>>
>> Maybe check cap->flags here and return with an error if any flag is set? ...
>> otherwise, if we ever add flags here, userspace cannot check whether the
>> kernel accepted a flag or not.
> 
> Check the top of the function :)

Ah, I missed that, so it should already be fine!


>>> + * Run all tests above.
>>> + *
>>> + * Enablement after VCPU has been added is automatically tested since
>>> + * we enable the capability after VCPU creation.
>>> + */
>>> +static struct testdef {
>>> +    const char *name;
>>> +    void (*test)(void);
>>> +} testlist[] = {
>>> +    { "instr0", test_user_instr0 },
>>> +    { "operexec", test_user_operexec },
>>> +    { "operexec_combined", test_user_operexec_combined},
>>> +};
>>> +
>>> +int main(int argc, char *argv[])
>>> +{
>>> +    int idx;
>>> +
>>> +    TEST_REQUIRE(kvm_has_cap(KVM_CAP_S390_USER_INSTR0));
>>> +
>>> +    ksft_print_header();
>>> +    ksft_set_plan(ARRAY_SIZE(testlist));
>>> +    for (idx = 0; idx < ARRAY_SIZE(testlist); idx++) {
>>> +        testlist[idx].test();
>>> +        ksft_test_result_pass("%s\n", testlist[idx].name);
>>> +    }
>>> +    ksft_finished();
>>> +}
>>
>> You could likely use the KVM_ONE_VCPU_TEST() macro and test_harness_run() to
>> get rid of the boilerplate code here.
> 
> Is there a general directive to use KVM_ONE_VCPU_TEST?

Certainly not from my side!
(but Sean might have a different opinion on this topic ;-))

> To be honest I prefer the look as is since it doesn't hide things behind 
> macros and 95% of our tests use it.

Fine for me, too.

  Thomas