Re: [PATCH v4 07/27] arm64: Prepare sharing arm64 headers with s390

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

On Mon, Jul 06, 2026 at 10:22:41PM +0100, Marc Zyngier wrote:
> On Mon, 06 Jul 2026 09:52:07 +0100,
> Steffen Eiden <seiden@linux.ibm.com> wrote:
> > 
> > Prepare the sharing of arm64 headers with s390 by marking the shared
> > regions or add a comment that the whole file is shared.
> > The regions are marked with:
> > 
> >  #define ARM64_S390_COMMON
> > 
> >  /* insert shared definitions here */
> > 
> >  #endif /* ARM64_S390_COMMON */
> > 
> > The preprocessor symbol ARM64_S390_COMMON is always defined for arm64.
> > Reduce the include scope where possible and beneficial for s390.
> > 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         |  5 +++--
> >  arch/arm64/include/asm/kvm_arm.h     |  6 ++++--
> >  arch/arm64/include/asm/kvm_emulate.h | 19 ++++++++++++++++++
> >  arch/arm64/include/asm/kvm_host.h    | 28 +++++++++++++++++++++++++++
> >  arch/arm64/include/asm/kvm_mmu.h     |  6 ++++++
> >  arch/arm64/include/asm/ptrace.h      |  9 +++++++++
> >  arch/arm64/include/asm/sysreg.h      | 29 +++++++++++++++++++++++++---
> >  9 files changed, 98 insertions(+), 7 deletions(-)
> > 

...

> >  		vcpu_set_flag((v), e);					\
> >  	} while (0)
> >  
> > +#endif /* ARM64_S390_COMMON */
> > +
> 
> It'd be worth looking at an initial patch reorganising this file to
> minimise the number of such markers. This is equally valid for other
> files in this patch.
> 
> [...]
> 

So it is fine if I reorganize files a bit to reduce the number of
markes?
I feared it would not. I will reorganize the files a bit then if this
reduces the number of sections. Also if some thiings are required in a
later series I might alreaddy share the stuff if possible if that
reduces the number of regions.


> > diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
> > index 6eae7e7e2a68..b782cae771fe 100644
> > --- a/arch/arm64/include/asm/kvm_mmu.h
> > +++ b/arch/arm64/include/asm/kvm_mmu.h
> > @@ -142,12 +142,15 @@ static __always_inline unsigned long __kern_hyp_va(unsigned long v)
> >  
> >  extern u32 __hyp_va_bits;
> >  
> > +#ifdef ARM64_S390_COMMON
> >  /*
> >   * We currently support using a VM-specified IPA size. For backward
> >   * compatibility, the default IPA size is fixed to 40bits.
> >   */
> >  #define KVM_PHYS_SHIFT	(40)
> >  
> > +#endif /* ARM64_S390_COMMON */
> > +
> >  #define kvm_phys_shift(mmu)		VTCR_EL2_IPA((mmu)->vtcr)
> >  #define kvm_phys_size(mmu)		(_AC(1, ULL) << kvm_phys_shift(mmu))
> >  #define kvm_phys_mask(mmu)		(kvm_phys_size(mmu) - _AC(1, ULL))
> > @@ -178,9 +181,12 @@ void stage2_unmap_vm(struct kvm *kvm);
> >  int kvm_init_stage2_mmu(struct kvm *kvm, struct kvm_s2_mmu *mmu, unsigned long type);
> >  void kvm_uninit_stage2_mmu(struct kvm *kvm);
> >  void kvm_free_stage2_pgd(struct kvm_s2_mmu *mmu);
> > +#ifdef ARM64_S390_COMMON
> >  int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa,
> >  			  phys_addr_t pa, unsigned long size, bool writable);
> 
> This only serves a purpose for GICv2, and I really hope your HW will
> not implement this. You should probably find a way to avoid dragging
> stuff that doesn't make sense on s390.
> 

good catch, I added this as a mistake. It was a leftover from the early
days.


...

> > +#ifdef ARM64_S390_COMMON
> > +
> >  /*
> >   * Instructions for modifying PSTATE fields.
> >   * As per Arm ARM for v8-A, Section "C.5.1.3 op0 == 0b00, architectural hints,
> > @@ -91,8 +99,6 @@
> >   */
> >  #define pstate_field(op1, op2)		((op1) << Op1_shift | (op2) << Op2_shift)
> >  #define PSTATE_Imm_shift		CRm_shift
> > -#define ENCODE_PSTATE(x, r)		(0xd500401f | PSTATE_ ## r | ((!!x) << PSTATE_Imm_shift))
> > -#define SET_PSTATE(x, r)		__emit_inst(ENCODE_PSTATE(x, r))
> >  
> >  #define PSTATE_PAN			pstate_field(0, 4)
> >  #define PSTATE_UAO			pstate_field(0, 3)
> > @@ -100,6 +106,11 @@
> >  #define PSTATE_DIT			pstate_field(3, 2)
> >  #define PSTATE_TCO			pstate_field(3, 4)
> >  
> > +#endif /* ARM64_S390_COMMON */
> > +
> > +#define ENCODE_PSTATE(x, r)		(0xd500401f | PSTATE_ ## r | ((!!x) << PSTATE_Imm_shift))
> > +#define SET_PSTATE(x, r)		__emit_inst(ENCODE_PSTATE(x, r))
> > +
> >  #define SET_PSTATE_PAN(x)		SET_PSTATE((x), PAN)
> >  #define SET_PSTATE_UAO(x)		SET_PSTATE((x), UAO)
> >  #define SET_PSTATE_SSBS(x)		SET_PSTATE((x), SSBS)
> > @@ -123,6 +134,8 @@
> >  #define GSB_SYS_BARRIER_INSN		__SYS_BARRIER_INSN(1, 0, 12, 0, 0, 31)
> >  #define GSB_ACK_BARRIER_INSN		__SYS_BARRIER_INSN(1, 0, 12, 0, 1, 31)
> 
> What is the reason to avoid dragging these definitions? They seem
> extremely similar to the rest.
> 

They emit arm64 instructions, dont they?

> >
> > +#ifdef ARM64_S390_COMMON
> > +
> >  /* Data cache zero operations */
> >  #define SYS_DC_ISW			sys_insn(1, 0, 7, 6, 2)
> >  #define SYS_DC_IGSW			sys_insn(1, 0, 7, 6, 4)
> > @@ -835,6 +848,8 @@
> >  #define SCTLR_ELx_A	 (BIT(1))
> >  #define SCTLR_ELx_M	 (BIT(0))
> >  
> > +#endif /* ARM64_S390_COMMON */
> > +
> >  #ifdef CONFIG_CPU_BIG_ENDIAN
> >  #define ENDIAN_SET_EL2		SCTLR_ELx_EE
> >  #else
> > @@ -869,6 +884,8 @@
> >  	 SCTLR_EL1_LSMAOE | SCTLR_EL1_nTLSMD | SCTLR_EL1_EIS   | \
> >  	 SCTLR_EL1_TSCXT  | SCTLR_EL1_EOS)
> >  
> > +#ifdef ARM64_S390_COMMON
> > +
> >  /* MAIR_ELx memory attributes (used by Linux) */
> >  #define MAIR_ATTR_DEVICE_nGnRnE		UL(0x00)
> >  #define MAIR_ATTR_DEVICE_nGnRE		UL(0x04)
> > @@ -1105,6 +1122,8 @@
> >  #define GICV5_GICR_CDNMIA_TYPE_MASK	GENMASK_ULL(31, 29)
> >  #define GICV5_GICR_CDNMIA_ID_MASK	GENMASK_ULL(23, 0)
> >  
> > +#endif /* ARM64_S390_COMMON */
> > +
> >  #define gicr_insn(insn)			read_sysreg_s(GICV5_OP_GICR_##insn)
> >  #define gic_insn(v, insn)		write_sysreg_s(v, GICV5_OP_GIC_##insn)
> >  
> > @@ -1254,6 +1273,8 @@
> >  	par;								\
> >  })
> >  
> > +#ifdef ARM64_S390_COMMON
> > +
> >  #define SYS_FIELD_VALUE(reg, field, val)	reg##_##field##_##val
> >  
> >  #define SYS_FIELD_GET(reg, field, val)		\
> > @@ -1266,6 +1287,8 @@
> >  		 FIELD_PREP(reg##_##field##_MASK,	\
> >  			    SYS_FIELD_VALUE(reg, field, val))
> >  
> > -#endif
> > +#endif /* ARM64_S390_COMMON */
> > +
> > +#endif /* __ASSEMBLER__ */
> >  
> >  #endif	/* __ASM_SYSREG_H */
> 
> Overall, the fine-grained tagging is a bit hard to read, and there is
> no obvious rationale on why some bits are needed, and not others are
> not.
>

Sometimes the reason that it is not used is that they are not used yet
but will in a later series. I defenitely can add a few more function
that we need right now to reduce the scatter.

> It definitely looks more manageable than the previous iteration, but
> as I said above, it would benefit from a less scattergun approach.
That is great to hear!

I tried to move as little arm code as possible. This is probably the
other extreme compared to the first attempts where I moved everything.
I will try out moving a few functions/defenitions inside the
headers/functions to resuce the scattering.

	Steffen