Re: [PATCH v3 17/23] KVM: s390: arm64: Integrate sysreg into the host

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] KVM: s390: arm64: hcrx_el2 not updated after finalizing ID
  regs
- [Medium] KVM: s390: arm64: preemptible smp_processor_id() in
  kvm_handle_sys_reg()
- [Medium] KVM: s390: arm64: missing reset of ctxt.elr_el1, spsr_el1,
  and sp_el1
--

commit 0aa20b7c35188a2ab3cdf479482a3360763c4252
Author: Steffen Eiden <seiden@linux.ibm.com>

KVM: s390: arm64: Integrate sysreg into the host

This commit integrates the arm64 system register management infrastructure
into the s390 host by initializing and resetting system registers on vCPU
creation and reset, finalizing them on the first vCPU run, handling
ESR_ELx_EC_SYS64 exits, and exposing them via KVM_CAP_ONE_REG.

> diff --git a/arch/s390/kvm/arm64/arm.c b/arch/s390/kvm/arm64/arm.c
> index ba6235629c7d9..62fd71af8968c 100644
> --- a/arch/s390/kvm/arm64/arm.c
> +++ b/arch/s390/kvm/arm64/arm.c

[ ... ]

> @@ -271,6 +280,10 @@ int kvm_arch_vcpu_run_pid_change(struct kvm_vcpu *vcpu)
>  	if (likely(READ_ONCE(vcpu->pid)))
>  		return 0;
>  
> +	ret = kvm_vcpu_finalize_sys_regs(vcpu);
> +	if (ret)
> +		return ret;
> +
>  	scoped_guard(mutex, &kvm->arch.config_lock)
>  		set_bit(KVM_ARCH_FLAG_HAS_RAN_ONCE, &kvm->arch.flags);
>  

[Severity: Medium]
Should kvm_arch_vcpu_run_pid_change() recompute vcpu->arch.hcrx_el2 (for
example via vcpu_reset_hcrx()) after finalizing ID registers?

vcpu_reset_hcrx() sets HCRX_EL2_MSCEn from ID_AA64ISAR2_EL1 during
kvm_reset_vcpu() at KVM_ARM_VCPU_INIT time.  Because KVM_SET_ONE_REG is
only permitted after KVM_ARM_VCPU_INIT, if userspace clears FEAT_MOPS in
SYS_ID_AA64ISAR2_EL1 before the first KVM_RUN, vcpu->arch.hcrx_el2 retains
the stale HCRX_EL2_MSCEn bit and kvm_arch_vcpu_ioctl_run() leaves MOPS
instructions enabled in hardware for the guest.

[ ... ]

> diff --git a/arch/s390/kvm/arm64/handle_exit.c b/arch/s390/kvm/arm64/handle_exit.c
> index e0c7773c2165d..1653163ed6d65 100644
> --- a/arch/s390/kvm/arm64/handle_exit.c
> +++ b/arch/s390/kvm/arm64/handle_exit.c
> @@ -62,6 +62,7 @@ static int kvm_handle_unknown_ec(struct kvm_vcpu *vcpu)
>  static exit_handle_fn arm_exit_handlers[] = {
>  	[0 ... ESR_ELx_EC_MAX]	= kvm_handle_unknown_ec,
>  	[ESR_ELx_EC_HVC64]	= handle_hvc,
> +	[ESR_ELx_EC_SYS64]	= kvm_handle_sys_reg,
>  	[ESR_ELx_EC_IABT_LOW]	= kvm_handle_guest_abort,
>  	[ESR_ELx_EC_DABT_LOW]	= kvm_handle_guest_abort,
>  };

[Severity: Medium]
Does calling kvm_handle_sys_reg() from handle_exit() invoke
_vcpu_read_sys_reg() and _vcpu_write_sys_reg() in preemptible context while
the vCPU is loaded?

In kvm_arch_vcpu_ioctl_run(), handle_exit() runs after local_irq_enable()
with preemption enabled and vcpu_is_loaded(vcpu) true.  When a guest traps
on a save-area system register such as OSLSR_EL1 or SCXTNUM_EL0/EL1,
kvm_handle_sys_reg() -> perform_access() calls vcpu_read_sys_reg() or
vcpu_write_sys_reg():

arch/s390/include/asm/kvm_host_arm64.h:_vcpu_read_sys_reg()
	KVM_BUG_ON(vcpu_is_loaded(C) && smp_processor_id() != (C)->cpu, (C)->kvm);

Under CONFIG_DEBUG_PREEMPT=y this triggers a "using smp_processor_id() in
preemptible code" splat, and if the vCPU thread is preempted and migrated
between smp_processor_id() and reading (C)->cpu, KVM_BUG_ON() fires and
terminates the VM.

> diff --git a/arch/s390/kvm/arm64/reset.c b/arch/s390/kvm/arm64/reset.c
> index e4cdce1dbf9d2..0339901f5fd08 100644
> --- a/arch/s390/kvm/arm64/reset.c
> +++ b/arch/s390/kvm/arm64/reset.c

[ ... ]

> @@ -43,9 +80,14 @@ void kvm_reset_vcpu(struct kvm_vcpu *vcpu)
>  		vcpu_put(vcpu);
>  
>  	kvm_reset_vcpu_core(vcpu);
> +	kvm_reset_sys_regs(vcpu);
>  
>  	/* Reset special registers */
>  	vcpu_reset_hcr(vcpu);
> +	vcpu_reset_hcrx(vcpu);
> +	vcpu_reset_cptr(vcpu);
> +	vcpu_reset_cnthctl(vcpu);
> +	vcpu_reset_icc(vcpu);

[Severity: Medium]
Does kvm_reset_vcpu() leave vcpu->arch.ctxt.elr_el1,
vcpu->arch.ctxt.spsr_el1, and vcpu->arch.sae_block.sp_el1 unreset when a
vCPU is reset after running?

Neither kvm_reset_vcpu_core() nor kvm_reset_sys_regs() resets those fields
because sys_reg_descs[] has no entries for ELR_EL1, SPSR_EL1, or SP_EL1.
On the next guest entry after a vCPU reset, kvm_arch_vcpu_ioctl_run()
writes the stale ctxt.elr_el1 and ctxt.spsr_el1 values back into
SYS_ELR_EL1 and SYS_SPSR_EL1 in the hardware save area.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-sysreg-v3-0-75e95980fb10@linux.ibm.com?part=17