Re: [PATCH 1/6] KVM: s390: selftests: Add kvm_exit test for instruction intercepts

[Aaron Brown <aaronmbr@linux.ibm.com>]

On 11/12/26 8:45 AM, Janosch Frank wrote:
> On 11/10/26 12:16 PM, Aaron M. Brown wrote:
[...]
>> +++ b/tools/testing/selftests/kvm/include/s390/kvm_lifecycle.h
>> @@ -0,0 +1,35 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Definition of structs for KVM lifecycle tests.
>> + *
>> + * Copyright IBM Corp. 2026
>> + *
>> + * Authors:
>> + *  Nitya Khamar (nkhamar@linux.ibm.com)
>> + *  Aaron Malik Brown (aaronmbr@linux.ibm.com)
>> + */
>> +#ifndef SELFTEST_KVM_LIFECYCLE_H
>> +#define SELFTEST_KVM_LIFECYCLE_H
>> +
>> +#include "kvm_util.h"
>> +#include <stdbool.h>
>> +
>> +struct kvm_lifecycle_testdef {
>> +    const char *name;
>> +    bool (*test)(void);
>> +};
>> +
>> +struct kvm_parameters {
>> +    struct kvm_vm *vm;
>> +    struct kvm_vcpu *vcpu;
>> +    void *guest_code;
>> +    struct kvm_run *run;
>> +    unsigned int interrupt_type;
>> +    struct kvm_s390_irq *irq;
>> +    struct kvm_s390_irq_state *irq_state;
>> +    struct {
>> +        bool immediate_exit_condition;
>> +    } manual_parameters;
>> +};
> 
> In this patch I don't see any benefit in having this struct.
> I'm hoping there's some use for it in later patches.

Yeah I may have added it a bit prematurely, as most of these fields are 
only used by the ucontrol tests later in the series, so patch 1 doesn't 
need them. I'll drop the struct from this patch and use local variables
in the next version, thanks

[...]
>> +
>> +static struct kvm_parameters create_vm_kvm_exit(struct kvm_parameters 
>> parameters)
> 
> Why aren't we using a pointer to the argument?
> 
>> +{
>> +    parameters.vm = vm_create_with_one_vcpu(&parameters.vcpu,
>> +                        parameters.guest_code);
>> +    return parameters;
>> +}
>> +
>> +static void cleanup_kvm(struct kvm_parameters *parameters)
>> +{
>> +    kvm_vm_free(parameters->vm);
>> +}
> 
> These two function generate >= 4 lines for a single line of useful code.
> 

Good point, I'll remove these helpers as well and just call
vm_create_with_one_vcpu() and kvm_vm_free() directly
>> +
>> +static void compare_regs(struct kvm_regs *regs, struct kvm_sync_regs 
>> *sregs)
>> +{
>> +    int register_number;
>> +
>> +    for (register_number = 0; register_number < NUM_GPRS; 
>> register_number++)
>> +        TEST_ASSERT_EQ(regs->gprs[register_number], sregs- 
>> >gprs[register_number]);
>> +}
>> +
>> +static void guestcode_instruction_intercept(void)
>> +{
>> +    asm volatile("diag 0,0, 0x500\n" :::);
>> +    GUEST_DONE();
>> +}
>> +
>> +static bool test_instruction_intercept(void)
>> +{
>> +    struct kvm_parameters param = {
>> +        .guest_code = guestcode_instruction_intercept,
>> +        .interrupt_type = KVM_S390_INT_EXTERNAL_CALL,
>> +        .manual_parameters.immediate_exit_condition = false,
>> +    };
>> +    struct kvm_s390_irq_state irq_state = {};
>> +    struct kvm_s390_irq irq = {};
>> +    struct kvm_regs regs;
>> +    int interrupts;
>> +
>> +    ksft_print_msg("Testing Instruction Intercept Exit\n");
>> +    param = create_vm_kvm_exit(param);
>> +    vcpu_run(param.vcpu);
>> +    param.run = param.vcpu->run;
>> +    irq.type = param.interrupt_type;
>> +    irq.u.extcall.code = param.vcpu->id;
>> +    irq_state.len = sizeof(irq);
>> +    irq_state.buf = (unsigned long)&irq;
>> +    param.irq = &irq;
>> +    param.irq_state = &irq_state;
>> +    TEST_ASSERT_KVM_EXIT_REASON(param.vcpu, KVM_EXIT_S390_SIEIC);
>> +    TEST_ASSERT(param.run->s390_sieic.icptcode == ICPT_INST,
>> +            "Failed with Unexpected icptcode of 0x%x\n",
>> +            param.run->s390_sieic.icptcode);
>> +    interrupts = __vcpu_ioctl(param.vcpu, KVM_S390_SET_IRQ_STATE,
>> +                  param.irq_state);
> 
> Why are we doing two things at the same time?
> What does the IRQ code add to the test?

You're right, we should focus on only  testing the exit, moreover 
irq_injection has gotten its own test file now, ill drop this
in v2 >> +    TEST_ASSERT(interrupts >= 0, "Cannot Inject Interrupt 
(%d)", errno);
> 
> That should be a == 0, no?
> Documentations says "0 on success".
> 

Indeed, this can also go away in v2, lets keep it simple
>> +    irq_state.buf = (unsigned long)&irq;
>> +    irq_state.len = sizeof(irq);
>> +    interrupts = __vcpu_ioctl(param.vcpu, KVM_S390_GET_IRQ_STATE,
>> +                  &irq_state);
>> +    TEST_ASSERT(interrupts > 0,
>> +            "Expected pending interrupt after injection, got %d", 
>> interrupts);
> 
> Here it's fine since the rc returns the number of bytes KVM copied.
> But we could actually check for a number being at least len(irq_state)

Good point, I will note that for possibly extending the irq tests in the 
future! >> +    TEST_ASSERT_EQ(irq.type, KVM_S390_INT_EXTERNAL_CALL);
>> +    TEST_ASSERT_EQ(irq.u.extcall.code, param.vcpu->id);
>> +    vcpu_run(param.vcpu);
>> +    TEST_ASSERT_KVM_EXIT_REASON(param.vcpu, KVM_EXIT_S390_SIEIC);
>> +    TEST_ASSERT(param.run->s390_sieic.icptcode == ICPT_INST,
>> +            "Expected ICPT_INST from GUEST_DONE after inject, got 
>> 0x%x\n",
>> +            param.run->s390_sieic.icptcode);
>> +    vcpu_regs_get(param.vcpu, &regs);
>> +    compare_regs(&regs, &param.run->s.regs);
>> +    cleanup_kvm(&param);
>> +    return true;
>> +}
>> +
>> +static void guestcode_diagnose_hypercall(void)
>> +{
>> +    asm volatile("diag 0,0, 0x501\n" :::);
>> +}
>> +
>> +static bool test_diagnose_code(void)
>> +{
>> +    struct kvm_parameters param = {
>> +        .guest_code = guestcode_diagnose_hypercall,
>> +        .manual_parameters.immediate_exit_condition = false,
>> +    };
>> +    ksft_print_msg("Testing Diagnosis Hypercall Exit\n");
> 
> Instruction is called diagnose according to the POP.

I will update that accordingly, thanks !

>> +    param = create_vm_kvm_exit(param);
>> +    vcpu_run(param.vcpu);
>> +    param.run = param.vcpu->run;
>> +    TEST_ASSERT_KVM_EXIT_REASON(param.vcpu, KVM_EXIT_S390_SIEIC);
>> +    TEST_ASSERT(param.run->s390_sieic.icptcode == ICPT_INST,
>> +            "Failed with Unexpected Exit Reason of %u and icptcode of 
>> 0x%x\n",
>> +            param.run->exit_reason, param.run->s390_sieic.icptcode);
>> +    TEST_ASSERT(param.run->s390_sieic.ipa >> 8 == IPA_DIAG,
>> +            "Failed with Unexpected Opcode of %u\n",
>> +            param.run->s390_sieic.ipa >> 8);
>> +    TEST_ASSERT(param.run->s390_sieic.ipb >> 16 == DIAG_ASSEMBLY_CODE,
>> +            "Failed with Unexpected Diagnose Code of %u\n",
>> +            param.run->s390_sieic.ipb >> 16);
>> +    cleanup_kvm(&param);
>> +    return true;
>> +}
>> +
>> +static void guestcode_diagnose_conditional(void)
>> +{
>> +    asm volatile("lghi %%r3,0x3333\n"
>> +        "lghi %%r5, 0x555\n"
>> +        "lghi %%r6, 0x3333\n"
>> +        "cgrjne %%r6, %%r3, 1f\n"
>> +        "diag 0,0, 0x501\n"
>> +        "1:\n"
>> +        :
>> +        :
>> +        : "r3", "r5", "r6"
>> +    );
>> +    GUEST_DONE();
>> +}
>> +
>> +static bool test_diagnose_conditional_code(void)
>> +{
>> +    struct kvm_parameters param = {
>> +        .guest_code = guestcode_diagnose_conditional,
>> +        .manual_parameters.immediate_exit_condition = false,
>> +    };
>> +    struct kvm_regs regs;
>> +
>> +    ksft_print_msg("Testing Diagnosis Hypercall for Conditional Code 
>> Exit\n");
> 
> What is this testing?

It is only testing that r3-r6 survive the intercept, so I think I can 
merge it into the DIAG 0x501 test for v2 >> +    param = 
create_vm_kvm_exit(param);
>> +    vcpu_run(param.vcpu);
>> +    param.run = param.vcpu->run;
>> +    TEST_ASSERT_KVM_EXIT_REASON(param.vcpu, KVM_EXIT_S390_SIEIC);
>> +    TEST_ASSERT(param.run->s390_sieic.icptcode == ICPT_INST,
>> +            "Failed with Unexpected Exit Reason of %u and icptcode of 
>> 0x%x\n",
>> +            param.run->exit_reason, param.run->s390_sieic.icptcode);
>> +    TEST_ASSERT(param.run->s390_sieic.ipa >> 8 == IPA_DIAG,
>> +            "Failed with Unexpected Opcode of %u\n",
>> +            param.run->s390_sieic.ipa >> 8);
>> +    TEST_ASSERT(param.run->s390_sieic.ipb >> 16 == DIAG_ASSEMBLY_CODE,
> 
> Which diag assembly code?
> There's a huge number of them and this test already uses 2.
This is for the virtio notify hypercall (500) in particular, I will be 
more explicit with the variable names in v2 ! >> +            "Failed 
with Unexpected Diagnose Code of %u\n",
>> +            param.run->s390_sieic.ipb >> 16);
>> +    vcpu_regs_get(param.vcpu, &regs);
>> +    TEST_ASSERT_EQ(regs.gprs[3], 0x3333);
>> +    TEST_ASSERT_EQ(regs.gprs[5], 0x555);
>> +    TEST_ASSERT_EQ(regs.gprs[6], 0x3333);
>> +    cleanup_kvm(&param);
>> +    return true;
>> +}
>> +
>> +static void test(void)
>> +{
>> +    int i;
>> +    struct kvm_lifecycle_testdef testlist[] = {
>> +        {
>> +            .name = "Testing Instruction Intercepts",
>> +            .test = test_instruction_intercept,
>> +        },
>> +        {
>> +            .name = "Testing DIAG Code",
>> +            .test = test_diagnose_code,
>> +        },
>> +        {
>> +            .name = "Testing Conditional DIAG Code",
>> +            .test = test_diagnose_conditional_code,
>> +        },
>> +    };
>> +
>> +    ksft_print_header();
>> +    ksft_set_plan(ARRAY_SIZE(testlist));
>> +
>> +    for (i = 0; i < ARRAY_SIZE(testlist); i++) {
>> +        if (testlist[i].test())
>> +            ksft_test_result_pass("%s\n", testlist[i].name);
>> +        else
>> +            ksft_test_result_skip("%s\n", testlist[i].name);
>> +    }
>> +}
>> +
>> +int main(void)
>> +{
>> +    TEST_REQUIRE(kvm_has_cap(KVM_CAP_USER_MEMORY2));
>> +    TEST_REQUIRE(kvm_has_cap(KVM_CAP_SYNC_REGS));
>> +    test();
>> +    ksft_finished();
>> +}
> 
thanks for the constructive comments Janosch, I will get to updating this,

Aaron