Re: [PATCH v2 07/20] KVM: arm64: Share more arm64 code with s390

[Marc Zyngier]

On Mon, 31 Aug 2026 15:55:22 +0100,
Steffen Eiden <seiden@xxxxxxxxxxxxx> wrote:
> 
> Mark functions that s390 can use to implement arm on s390 as shared
> functions.

More of what? For what purpose? What is the logic behind exposing
snippets of code to S390? This patch tags some PAuth stuff, a bunch of
SVE definitions, and a whole lot of sysreg helpers.

What is the rationale for this organisation? I can't see any, and it
just looks like a very random approach to the problem.

I want to see logic, a progression. Something I can follow.

> 
> Signed-off-by: Steffen Eiden <seiden@xxxxxxxxxxxxx>
> ---
>  arch/arm64/kvm/arm.c      |  3 +++
>  arch/arm64/kvm/guest.c    | 11 +++++++----
>  arch/arm64/kvm/sys_regs.c | 36 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 46 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 6b92a3c1d490..62e8b32ee74e 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -340,6 +340,7 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
>  	kvm_arm_teardown_hypercalls(kvm);
>  }
>  
> +#ifdef ARM64_S390_COMMON
>  static bool kvm_has_full_ptr_auth(void)
>  {
>  	bool apa, gpa, api, gpi, apa3, gpa3;
> @@ -374,6 +375,8 @@ static bool kvm_has_full_ptr_auth(void)
>  		(apa + api + apa3) == 1);
>  }
>  
> +#endif /* ARM64_S390_COMMON */
> +

This could have been done much earlier.

>  int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>  {
>  	int r;
> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> index 6ca5a9f357cd..e978b2fbeb76 100644
> --- a/arch/arm64/kvm/guest.c
> +++ b/arch/arm64/kvm/guest.c
> @@ -307,12 +307,12 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
>  	return err;
>  }
>  
> -#endif /* ARM64_S390_COMMON */
> -
>  #define vq_word(vq) (((vq) - SVE_VQ_MIN) / 64)
>  #define vq_mask(vq) ((u64)1 << ((vq) - SVE_VQ_MIN) % 64)
>  #define vq_present(vqs, vq) (!!((vqs)[vq_word(vq)] & vq_mask(vq)))
>  
> +#endif /* ARM64_S390_COMMON */
> +

Pointless churn.

>  static int get_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
>  {
>  	unsigned int max_vq, vq;
> @@ -383,6 +383,7 @@ static int set_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
>  	return 0;
>  }
>  
> +#ifdef ARM64_S390_COMMON
>  #define SVE_REG_SLICE_SHIFT	0
>  #define SVE_REG_SLICE_BITS	5
>  #define SVE_REG_ID_SHIFT	(SVE_REG_SLICE_SHIFT + SVE_REG_SLICE_BITS)
> @@ -399,6 +400,8 @@ static int set_sve_vls(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
>  #define KVM_SVE_ZREG_SIZE KVM_REG_SIZE(KVM_REG_ARM64_SVE_ZREG(0, 0))
>  #define KVM_SVE_PREG_SIZE KVM_REG_SIZE(KVM_REG_ARM64_SVE_PREG(0, 0))
>  
> +#endif /* ARM64_S390_COMMON */
> +
>  /*
>   * Number of register slices required to cover each whole SVE register.
>   * NOTE: Only the first slice every exists, for now.
> @@ -595,8 +598,6 @@ static unsigned long num_core_regs(const struct kvm_vcpu *vcpu)
>  	return copy_core_reg_indices(vcpu, NULL);
>  }
>  
> -#endif /* ARM64_S390_COMMON */
> -
>  static unsigned long num_sve_regs(const struct kvm_vcpu *vcpu)
>  {
>  	const unsigned int slices = vcpu_sve_slices(vcpu);
> @@ -658,6 +659,8 @@ static int copy_sve_reg_indices(const struct kvm_vcpu *vcpu,
>  	return num_regs;
>  }
>  
> +#endif /* ARM64_S390_COMMON */
> +
>  /**
>   * kvm_arm_num_regs - how many registers do we present via KVM_GET_ONE_REG
>   * @vcpu: the vCPU pointer
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 53d096803d9d..b167f9d06fd4 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -45,6 +45,7 @@
>   * 64bit interface.
>   */
>  
> +#ifdef ARM64_S390_COMMON
>  static u64 sys_reg_to_index(const struct sys_reg_desc *reg);
>  static int set_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>  		      u64 val);
> @@ -82,6 +83,8 @@ static bool write_to_read_only(struct kvm_vcpu *vcpu,
>  			"sys_reg write to read-only register");
>  }
>  
> +#endif /* ARM64_S390_COMMON */
> +
>  enum sr_loc_attr {
>  	SR_LOC_MEMORY	= 0,	  /* Register definitely in memory */
>  	SR_LOC_LOADED	= BIT(0), /* Register on CPU, unless it cannot */
> @@ -428,6 +431,7 @@ void vcpu_write_sys_reg(struct kvm_vcpu *vcpu, u64 val, enum vcpu_sysreg reg)
>  	__vcpu_assign_sys_reg(vcpu, reg, val);
>  }
>  
> +#ifdef ARM64_S390_COMMON
>  /* CSSELR values; used to index KVM_REG_ARM_DEMUX_ID_CCSIDR */
>  #define CSSELR_MAX 14
>  
> @@ -533,6 +537,8 @@ static bool access_rw(struct kvm_vcpu *vcpu,
>  	return true;
>  }
>  
> +#endif /* ARM64_S390_COMMON */
> +
>  /*
>   * See note at ARMv7 ARM B1.14.4 (TL;DR: S/W ops are not easily virtualized).
>   */
> @@ -800,6 +806,7 @@ static bool access_gicv5_ppi_enabler(struct kvm_vcpu *vcpu,
>  	return true;
>  }
>  
> +#ifdef ARM64_S390_COMMON
>  static bool trap_raz_wi(struct kvm_vcpu *vcpu,
>  			struct sys_reg_params *p,
>  			const struct sys_reg_desc *r)
> @@ -853,6 +860,8 @@ static bool trap_oslsr_el1(struct kvm_vcpu *vcpu,
>  	return true;
>  }
>  
> +#endif /* ARM64_S390_COMMON */
> +
>  static int set_oslsr_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>  			 u64 val)
>  {
> @@ -1011,6 +1020,7 @@ static u64 reset_actlr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
>  	return actlr;
>  }
>  
> +#ifdef ARM64_S390_COMMON
>  static u64 reset_mpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
>  {
>  	u64 mpidr = kvm_calculate_mpidr(vcpu);
> @@ -1019,6 +1029,8 @@ static u64 reset_mpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
>  	return mpidr;
>  }
>  
> +#endif /* ARM64_S390_COMMON */
> +
>  static unsigned int hidden_visibility(const struct kvm_vcpu *vcpu,
>  				      const struct sys_reg_desc *r)
>  {
> @@ -1922,6 +1934,7 @@ static u8 pmuver_to_perfmon(u8 pmuver)
>  	}
>  }
>  
> +#ifdef ARM64_S390_COMMON
>  static u64 sanitise_id_aa64pfr0_el1(const struct kvm_vcpu *vcpu, u64 val);
>  static u64 sanitise_id_aa64pfr1_el1(const struct kvm_vcpu *vcpu, u64 val);
>  static u64 sanitise_id_aa64pfr2_el1(const struct kvm_vcpu *vcpu, u64 val);
> @@ -2078,6 +2091,8 @@ static bool access_id_reg(struct kvm_vcpu *vcpu,
>  	return true;
>  }
>  
> +#endif /* ARM64_S390_COMMON */
> +
>  static unsigned int aa32_id_visibility(const struct kvm_vcpu *vcpu,
>  				       const struct sys_reg_desc *r)
>  {
> @@ -2127,6 +2142,7 @@ static unsigned int fp8_visibility(const struct kvm_vcpu *vcpu,
>  	return REG_HIDDEN;
>  }
>  
> +#ifdef ARM64_S390_COMMON
>  static u64 sanitise_id_aa64pfr0_el1(const struct kvm_vcpu *vcpu, u64 val)
>  {
>  	if (!vcpu_has_sve(vcpu))
> @@ -2288,6 +2304,8 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
>  	return set_id_reg(vcpu, rd, val);
>  }
>  
> +#endif /* ARM64_S390_COMMON */
> +
>  static u64 read_sanitised_id_dfr0_el1(struct kvm_vcpu *vcpu,
>  				      const struct sys_reg_desc *rd)
>  {
> @@ -2332,6 +2350,7 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
>  	return set_id_reg(vcpu, rd, val);
>  }
>  
> +#ifdef ARM64_S390_COMMON
>  static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
>  			       const struct sys_reg_desc *rd, u64 user_val)
>  {
> @@ -2661,6 +2680,8 @@ static bool access_ccsidr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>  	return true;
>  }
>  
> +#endif /* ARM64_S390_COMMON */
> +
>  static int get_raz_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>  		       u64 *val)
>  {
> @@ -2764,6 +2785,7 @@ static bool bad_redir_trap(struct kvm_vcpu *vcpu,
>  	SYS_REG_USER_FILTER(name, access_arch_timer, reset_val, 0, \
>  			    arch_timer_get_user, arch_timer_set_user, vis)
>  
> +#ifdef ARM64_S390_COMMON
>  /*
>   * Since reset() callback and field val are not used for idregs, they will be
>   * used for specific purposes for idregs.
> @@ -2840,6 +2862,8 @@ static bool bad_redir_trap(struct kvm_vcpu *vcpu,
>  	.val = 0,				\
>  }
>  
> +#endif /* ARM64_S390_COMMON */
> +
>  static bool access_sp_el1(struct kvm_vcpu *vcpu,
>  			  struct sys_reg_params *p,
>  			  const struct sys_reg_desc *r)
> @@ -3212,6 +3236,7 @@ static void init_imp_id_regs(void)
>  	boot_cpu_aidr_val = read_sysreg(aidr_el1);
>  }
>  
> +#ifdef ARM64_S390_COMMON
>  static u64 reset_imp_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
>  {
>  	switch (reg_to_encoding(r)) {
> @@ -3269,6 +3294,8 @@ static int set_imp_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r,
>  	.val = mask,					\
>  	}
>  
> +#endif /* ARM64_S390_COMMON */
> +
>  static u64 reset_mdcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
>  {
>  	__vcpu_assign_sys_reg(vcpu, r->reg, vcpu->kvm->arch.nr_pmu_counters);
> @@ -4833,6 +4860,7 @@ static const struct sys_reg_desc cp15_64_regs[] = {
>  	{ SYS_DESC(SYS_AARCH32_CNTVCTSS),     access_arch_timer },
>  };
>  
> +#ifdef ARM64_S390_COMMON
>  static bool check_sysreg_table(const struct sys_reg_desc *table, unsigned int n,
>  			       bool reset_check)
>  {
> @@ -4882,6 +4910,8 @@ static void perform_access(struct kvm_vcpu *vcpu,
>  		kvm_incr_pc(vcpu);
>  }
>  
> +#endif /* ARM64_S390_COMMON */
> +
>  /*
>   * emulate_cp --  tries to match a sys_reg access in a handling table, and
>   *                call the corresponding trap handler.
> @@ -5340,6 +5370,7 @@ void kvm_sys_regs_create_debugfs(struct kvm *kvm)
>  			    &sr_resx_fops);
>  }
>  
> +#ifdef ARM64_S390_COMMON
>  static void reset_vm_ftr_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *reg)
>  {
>  	u32 id = reg_to_encoding(reg);
> @@ -5360,6 +5391,8 @@ static void reset_vcpu_ftr_id_reg(struct kvm_vcpu *vcpu,
>  	reg->reset(vcpu, reg);
>  }
>  
> +#endif /* ARM64_S390_COMMON */
> +
>  /**
>   * kvm_reset_sys_regs - sets system registers to reset value
>   * @vcpu: The VCPU pointer
> @@ -5432,6 +5465,7 @@ int kvm_handle_sys_reg(struct kvm_vcpu *vcpu)
>  	return 1;
>  }
>  
> +#ifdef ARM64_S390_COMMON
>  /******************************************************************************
>   * Userspace API
>   *****************************************************************************/
> @@ -5753,6 +5787,8 @@ int kvm_arm_copy_sys_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
>  	return write_demux_regids(uindices);
>  }
>  
> +#endif /* ARM64_S390_COMMON */
> +
>  #define KVM_ARM_FEATURE_ID_RANGE_INDEX(r)			\
>  	KVM_ARM_FEATURE_ID_RANGE_IDX(sys_reg_Op0(r),		\
>  		sys_reg_Op1(r),					\

The way you pick and chose random helpers feels *VERY* wrong. I really
want to understand the rationale for not using most of it, *including*
the sysreg arrays as a whole. Everything in this file is (or at leats
aims to be) architectural, and should be directly reused.

It should only be a matter of providing the required indirection to
access the S390 representation of the arm64 state.

	M.

-- 
Jazz isn't dead. It just smells funny.