Re: [PATCH v6 10/33] arm64: Share arm64 headers with s390

[Steffen Eiden <seiden@linux.ibm.com>]

On Thu, Aug 13, 2026 at 02:19:48PM +0100, Marc Zyngier wrote:
> On Wed, 12 Aug 2026 16:36:06 +0100,
> Steffen Eiden <seiden@linux.ibm.com> wrote:
> > 
> > Allow sharing of arm64 headers with s390 by marking the shared regions
> > or add a comment that the whole file is shared.
> > 
> > The shared regions are marked with:
> > 
> >  #ifdef ARM64_S390_COMMON
> > 
> >  /* insert shared definitions here */
> > 
> >  #endif /* ARM64_S390_COMMON */
> > 
> > The preprocessor symbol ARM64_S390_COMMON is always defined for arm64.
> > s390 later will generate its own view of arm64 headers based on the
> > content inside the ifdef/endif block.
> > 
> > No functional change.
> > 
> > Signed-off-by: Steffen Eiden <seiden@linux.ibm.com>
> > ---
> >  arch/arm64/Makefile                  |  2 ++
> >  arch/arm64/include/asm/brk-imm.h     |  1 +
> >  arch/arm64/include/asm/esr.h         |  1 +
> >  arch/arm64/include/asm/kvm_arm.h     |  1 +
> >  arch/arm64/include/asm/kvm_emulate.h | 18 ++++++++++++++++++
> >  arch/arm64/include/asm/kvm_host.h    | 20 ++++++++++++++++++++
> >  arch/arm64/include/asm/ptrace.h      |  4 ++++
> >  arch/arm64/include/asm/sysreg.h      | 15 +++++++++++++++
> >  8 files changed, 62 insertions(+)
> > 

...

> > diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> > index a34a9417ff4d..09f04a5686a1 100644
> > --- a/arch/arm64/include/asm/kvm_emulate.h
> > +++ b/arch/arm64/include/asm/kvm_emulate.h
> > @@ -47,6 +47,7 @@ void kvm_skip_instr32(struct kvm_vcpu *vcpu);
> >  int kvm_inject_dabt_excl_atomic(struct kvm_vcpu *vcpu, u64 addr);
> >  int kvm_inject_serror_esr(struct kvm_vcpu *vcpu, u64 esr);
> >  
> > +#ifdef ARM64_S390_COMMON
> >  void kvm_inject_undefined(struct kvm_vcpu *vcpu);
> >  void kvm_inject_sync(struct kvm_vcpu *vcpu, u64 esr);
> >  int kvm_inject_sea(struct kvm_vcpu *vcpu, bool iabt, u64 addr);
> > @@ -62,6 +63,8 @@ static inline int kvm_inject_sea_iabt(struct kvm_vcpu *vcpu, u64 addr)
> >  	return kvm_inject_sea(vcpu, true, addr);
> >  }
> >  
> > +#endif /* ARM64_S390_COMMON */
> > +
> >  static inline int kvm_inject_serror(struct kvm_vcpu *vcpu)
> >  {
> >  	/*
> > @@ -185,6 +188,7 @@ static inline void vcpu_set_thumb(struct kvm_vcpu *vcpu)
> >  	*vcpu_cpsr(vcpu) |= PSR_AA32_T_BIT;
> >  }
> >  
> > +#ifdef ARM64_S390_COMMON
> >  /*
> >   * vcpu_get_reg and vcpu_set_reg should always be passed a register number
> >   * coming from a read of ESR_EL2. Otherwise, it may give the wrong result on
> > @@ -203,6 +207,8 @@ static __always_inline void vcpu_set_reg(struct kvm_vcpu *vcpu, u8 reg_num,
> >  		vcpu_gp_regs(vcpu)[reg_num] = val;
> >  }
> >  
> > +#endif /* ARM64_S390_COMMON */
> > +
> >  static inline bool vcpu_is_el2_ctxt(const struct kvm_cpu_context *ctxt)
> >  {
> >  	switch (ctxt->regs.pstate & (PSR_MODE32_BIT | PSR_MODE_MASK)) {
> > @@ -405,6 +411,7 @@ static inline u64 kvm_vcpu_get_disr(const struct kvm_vcpu *vcpu)
> >  	return vcpu->arch.fault.disr_el1;
> >  }
> >  
> > +#ifdef ARM64_S390_COMMON
> >  static inline u32 kvm_vcpu_hvc_get_imm(const struct kvm_vcpu *vcpu)
> >  {
> >  	return kvm_vcpu_get_esr(vcpu) & ESR_ELx_xVC_IMM_MASK;
> > @@ -482,6 +489,8 @@ static __always_inline u8 kvm_vcpu_trap_get_fault(const struct kvm_vcpu *vcpu)
> >  	return kvm_vcpu_get_esr(vcpu) & ESR_ELx_FSC;
> >  }
> >  
> > +#endif /* ARM64_S390_COMMON */
> > +
> >  static inline
> >  bool kvm_vcpu_trap_is_permission_fault(const struct kvm_vcpu *vcpu)
> >  {
> > @@ -522,6 +531,7 @@ static __always_inline int kvm_vcpu_sys_get_rt(struct kvm_vcpu *vcpu)
> >  	return ESR_ELx_SYS64_ISS_RT(esr);
> >  }
> >  
> > +#ifdef ARM64_S390_COMMON
> 
> I'm curious about the reasons for skipping a bunch of architectural
> helpers:
> 
> - kvm_vcpu_trap_is_permission_fault()
we need the s390 state for this function -> we overload it
the only use is in shared arm64 code for is_write_fault
> - kvm_vcpu_trap_is_translation_fault()
> - kvm_vcpu_trap_get_perm_fault_granule()
> - kvm_vcpu_abt_issea()
as s390 mm works differntly in comparison to ARMs (suprise :) )
we do not need those


> - kvm_vcpu_sys_get_rt()
I'll add using this function in the next series (sysreg enablement)
I can do that now - It reduces the churn to the next series.

> 
> I really think you should include anything that is exclusively derived
> from architectural state, specially that these helpers are strictly
> dependent on stuff you already include.
> 
> Any particular reason why this isn't the case?

These functions operate right at the border where the s390 architectural
state (host view) and the arm64 architectural state (guest view) meet.

Especially, for the memory management of the guest we need to use
s390 based guest memory management (gmap) as the host only knows s390
(+SAE & friends to actually run an arm64 guest)

The s390 host work here is to convert the s390-state into arm64 state
for the guest. e.g. convert a s390 translation fault into a data abort (if
applicable)

The code in the second series should make this more
clear. As we now are clear on the code-sharing concept I will sent a
respin of the second series using the new sharing approach ASAP.


> 
> >  static inline bool kvm_is_write_fault(struct kvm_vcpu *vcpu)
> >  {
> >  	if (kvm_vcpu_abt_iss1tw(vcpu)) {
> > @@ -546,6 +556,8 @@ static inline bool kvm_is_write_fault(struct kvm_vcpu *vcpu)
> >  	return kvm_vcpu_dabt_iswrite(vcpu);
> >  }
> >  
> > +#endif /* ARM64_S390_COMMON */
> > +
> >  static inline unsigned long kvm_vcpu_get_mpidr_aff(struct kvm_vcpu *vcpu)
> >  {
> >  	return __vcpu_sys_reg(vcpu, MPIDR_EL1) & MPIDR_HWID_BITMASK;
> > @@ -587,6 +599,7 @@ static inline bool kvm_vcpu_is_be(struct kvm_vcpu *vcpu)
> >  	return vcpu_read_sys_reg(vcpu, r) & bit;
> >  }
> >  
> > +#ifdef ARM64_S390_COMMON
> 
> Similarly, you have dropped
> 
> - kvm_vcpu_set_be()
> 
> - kvm_vcpu_is_be()
> 
> I suspect that your implementation is LE only (it'd better be, as
> we're dropping BE support right now), but it would make a lot more
> sense if you could rely on the guest architectural state (which must
> be accurate) rather than short-cutting things in your own
> implementation.

Yes, you are right we are LE only. Therefore, I defined the function to
false such that the compiler/linker can optimize-out unreachable code.
This is true for a couple of function-shortcuts.

I would like to keep it that way if this is no show stopper for you.


...

> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index 22770c1132cb..e6b3d42abb08 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -39,6 +39,7 @@
> >  
> >  #define KVM_MAX_VCPUS VGIC_V3_MAX_CPUS
> >  
> > +#ifdef ARM64_S390_COMMON
> >  #define KVM_VCPU_MAX_FEATURES 10
> >  #define KVM_VCPU_VALID_FEATURES	(BIT(KVM_VCPU_MAX_FEATURES) - 1)
> >  
> > @@ -86,6 +87,8 @@
> >  /* Unhandled SEAs are taken to userspace */
> >  #define KVM_ARCH_FLAG_EXIT_SEA				11
> >  
> > +#endif /* ARM64_S390_COMMON */
> > +
> >  #define KVM_DIRTY_LOG_MANUAL_CAPS   (KVM_DIRTY_LOG_MANUAL_PROTECT_ENABLE | \
> >  				     KVM_DIRTY_LOG_INITIALLY_SET)
> >  
> > @@ -836,6 +839,8 @@ extern s64 kvm_nvhe_sym(hyp_physvirt_offset);
> >  extern u64 kvm_nvhe_sym(hyp_cpu_logical_map)[NR_CPUS];
> >  #define hyp_cpu_logical_map CHOOSE_NVHE_SYM(hyp_cpu_logical_map)
> >  
> > +#ifdef ARM64_S390_COMMON
> > +
> >  struct vcpu_reset_state {
> >  	unsigned long	pc;
> >  	unsigned long	r0;
> > @@ -843,6 +848,8 @@ struct vcpu_reset_state {
> >  	bool		reset;
> >  };
> >  
> > +#endif /* ARM64_S390_COMMON */
> 
> Given that this is a standalone structure without any dependency,
> could you simply move it to a "nicer" place as part of the reshuffling
> patch?
> 

Yes, no problem.

I just wanted to limit the number of moves. I'll guess we will meet in
the middle somewhere :) 


...

> > @@ -1248,6 +1259,8 @@ int kvm_arm_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg);
> >  unsigned long kvm_arm_num_sys_reg_descs(struct kvm_vcpu *vcpu);
> >  int kvm_arm_copy_sys_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices);
> >  
> > +#endif /* ARM64_S390_COMMON */
> > +
> >  int __kvm_arm_vcpu_get_events(struct kvm_vcpu *vcpu,
> >  			      struct kvm_vcpu_events *events);
> >  
> > @@ -1326,6 +1339,7 @@ int __init populate_nv_trap_config(void);
> >  
> >  void kvm_calculate_traps(struct kvm_vcpu *vcpu);
> >  
> > +#ifdef ARM64_S390_COMMON
> >  /* MMIO helpers */
> >  void kvm_mmio_write_buf(void *buf, unsigned int len, unsigned long data);
> >  unsigned long kvm_mmio_read_buf(const void *buf, unsigned int len);
> > @@ -1333,6 +1347,8 @@ unsigned long kvm_mmio_read_buf(const void *buf, unsigned int len);
> >  int kvm_handle_mmio_return(struct kvm_vcpu *vcpu);
> >  int io_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa);
> >  
> > +#endif /* ARM64_S390_COMMON */
> 
> Similarly, it'd be marginally better if you moved some of these
> prototypes together, as below (untested).
> 

Will do.

	Steffen

...