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

[Marc Zyngier]

On Tue, 18 Aug 2026 15:09:01 +0100,
Steffen Eiden <seiden@xxxxxxxxxxxxx> wrote:
> 
> On Thu, Aug 13, 2026 at 02:19:48PM +0100, Marc Zyngier wrote:
> > 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)

Exactly. Which is why I was expecting that the s390 code would
populate the copy of the ESR_EL2 register in the vcpu structure, and
let the arm64 code do the talking. But evidently, that's not what is
happening (or at least not that way).

> 
> 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.

Not a show-stopper, but I think we'll eventually want to work more at
the architecture level rather than solely at the "code sharing" level.

I expect it would make things a bit simpler to follow.

	M.

-- 
Without deviation from the norm, progress is not possible.