Re: [PATCH v5 27/31] KVM: s390: arm64: Implement required functions

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Uninitialized spinlock `mp_state_lock` triggers guest-to-host kernel panic
- [Critical] Infinite VM-Exit Loop on Stubbed Exception Injection
- [High] Incomplete SMP System Shutdown in PSCI SYSTEM_OFF handler
- [Medium] KVM Feature Enumeration Violation (HVC trapping unconditionally enabled)
- [Low] Compilation Errors / Undefined Constants
--

--- Patch [27]: [PATCH v5 27/31] KVM: s390: arm64: Implement required functions ---
commit 9ebfb4dc7b6b58f4a7a2e0419672a287a40a3f1e
Author: Steffen Eiden <[email protected]>

KVM: s390: arm64: Implement required functions

This commit implements the mostly trivial functions required by the shared
arm64 KVM code for s390. It also adds a very basic smccc handler that
can stop a vcpu.

> diff --git a/arch/s390/include/arm64/kvm_emulate.h b/arch/s390/include/arm64/kvm_emulate.h
> new file mode 100644
> --- /dev/null
> +++ b/arch/s390/include/arm64/kvm_emulate.h

[ ... ]

> +static inline bool vcpu_mode_priv(const struct kvm_vcpu *vcpu)
> +{
> +	u32 mode = *vcpu_cpsr(vcpu) & PSR_MODE_MASK;
> +
> +	return mode != PSR_MODE_EL0t;
> +}

[Severity: Low]
Does s390 have definitions for PSR_MODE_MASK or PSR_MODE_EL0t? This file
includes the host's <asm/ptrace.h> instead of the newly created
<arm64/ptrace.h>, which might result in missing macros and build failures.

> +static inline void kvm_skip_instr(struct kvm_vcpu *vcpu)
> +{
> +	*vcpu_pc(vcpu) += 4;
> +	*vcpu_cpsr(vcpu) &= ~PSR_BTYPE_MASK;
> +
> +	/* advance the singlestep state machine */
> +	*vcpu_cpsr(vcpu) &= ~SPSR_EL1_SS;
> +}

[Severity: Low]
Is SPSR_EL1_SS defined anywhere in this tree? The ARM64 macro is usually
named DBG_SPSR_SS, so this looks like it might fail to compile. Additionally,
PSR_BTYPE_MASK would also require the <arm64/ptrace.h> header to be included.

[ ... ]

> diff --git a/arch/s390/kvm/arm64/handle_exit.c b/arch/s390/kvm/arm64/handle_exit.c
> new file mode 100644
> --- /dev/null
> +++ b/arch/s390/kvm/arm64/handle_exit.c

[ ... ]

> +static int kvm_smccc_call_handler(struct kvm_vcpu *vcpu)
> +{
> +	u32 func_id = vcpu_get_reg(vcpu, 0);
> +	u64 val = PSCI_RET_NOT_SUPPORTED;
> +	int ret = 1;
> +
> +	if (func_id == PSCI_0_2_FN_SYSTEM_OFF) {

[Severity: Medium]
Should the PSCI SYSTEM_OFF feature be unconditionally exposed to the guest?
According to KVM subsystem guidelines, any new guest-visible behavior must be
off by default and discoverable through standard capability enumeration
interfaces to ensure capability negotiation and live migration work properly.

> +		scoped_guard(spinlock, &vcpu->arch.mp_state_lock) {

[Severity: Critical]
Has the vcpu->arch.mp_state_lock been initialized for s390 vCPUs? If this is
a zero-allocated structure and the spinlock lacks the required magic number,
could an untrusted guest executing an HVC call trigger a host kernel panic on
kernels configured with CONFIG_DEBUG_SPINLOCK?

> +			WRITE_ONCE(vcpu->arch.mp_state.mp_state, KVM_MP_STATE_STOPPED);
> +		}
> +		kvm_make_all_cpus_request(vcpu->kvm, KVM_REQ_SLEEP);

[Severity: Low]
Is KVM_REQ_SLEEP defined for s390? This macro is typically specific to ARM64
and RISC-V, which might cause a compilation error here.

[Severity: High]
Will this properly stop all vCPUs in an SMP guest? The code updates the power
state to KVM_MP_STATE_STOPPED only for the currently executing vCPU. When the
broadcasted KVM_REQ_SLEEP request is processed by the other vCPUs, won't they
check their own unmodified states and immediately resume guest execution?

> +		memset(&vcpu->run->system_event, 0,
> +		       sizeof(vcpu->run->system_event));

[ ... ]

> +static int handle_hvc(struct kvm_vcpu *vcpu)
> +{
> +	vcpu->stat.hvc_exit_stat++;
> +	return kvm_smccc_call_handler(vcpu);
> +}
> +
> +exit_handle_fn arm_exit_handlers[] = {
> +	[0 ... ESR_ELx_EC_MAX]	= kvm_handle_unknown_ec,
> +	[ESR_ELx_EC_HVC64]	= handle_hvc,
> +};

[Severity: Medium]
Similar to the PSCI SYSTEM_OFF issue above, should HVC64 trapping be
unconditionally enabled and mapped to handle_hvc without any capability
checks? KVM guidelines require new emulated instructions and exit reasons to
be enumerable and default to off.

[ ... ]

> diff --git a/arch/s390/kvm/arm64/inject_fault.c b/arch/s390/kvm/arm64/inject_fault.c
> new file mode 100644
> --- /dev/null
> +++ b/arch/s390/kvm/arm64/inject_fault.c
> @@ -0,0 +1,21 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <arm64/kvm_emulate.h>
> +
> +/**
> + * kvm_inject_undefined - inject an undefined instruction into the guest
> + * @vcpu: The vCPU in which to inject the exception
> + *
> + * It is assumed that this code is called from the VCPU thread and that the
> + * VCPU therefore is not currently executing guest code.
> + */
> +void kvm_inject_undefined(struct kvm_vcpu *vcpu)
> +{
> +	/* Stub until s390 supports arm64 sysregs TODO sysregs*/
> +}

[Severity: Critical]
Since this is an empty stub, will it cause an infinite VM-exit loop? If
kvm_handle_unknown_ec() calls kvm_inject_undefined() to handle an unknown
instruction, it will immediately return 1 to KVM's run loop. Because this
stub fails to advance the guest PC or inject an actual fault, won't the guest
re-execute the exact same faulting instruction and immediately trap again,
leading to a 100% host CPU lockup?

> +
> +int kvm_inject_sea(struct kvm_vcpu *vcpu, bool iabt, u64 addr)
> +{
> +	/* Stub until s390 supports arm64 sysregs TODO sysregs*/
> +	return 1;
> +}

[Severity: Critical]
Similar to kvm_inject_undefined(), won't returning 1 from this empty stub
immediately resume the guest without altering its execution state, resulting
in an inescapable infinite loop if a synchronous external abort occurs?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=27