Re: [PATCH 02/13] KVM: arm64: Clarify ESR_ELx_ERET_ISS_ERET*

From: Marc Zyngier <maz@kernel.org>
To: Joey Gouly <joey.gouly@arm.com>
Cc: kvmarm@lists.linux.dev, kvm@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	James Morse <james.morse@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Oliver Upton <oliver.upton@linux.dev>,
	Zenghui Yu <yuzenghui@huawei.com>, Will Deacon <will@kernel.org>,
	Catalin Marinas <catalin.marinas@arm.com>
Subject: Re: [PATCH 02/13] KVM: arm64: Clarify ESR_ELx_ERET_ISS_ERET*
Date: Tue, 20 Feb 2024 12:29:30 +0000	[thread overview]
Message-ID: <861q9748ut.wl-maz@kernel.org> (raw)
In-Reply-To: <20240220113127.GB16168@e124191.cambridge.arm.com>

On Tue, 20 Feb 2024 11:31:27 +0000,
Joey Gouly <joey.gouly@arm.com> wrote:
> 
> On Mon, Feb 19, 2024 at 09:20:03AM +0000, Marc Zyngier wrote:
> > The ESR_ELx_ERET_ISS_ERET* macros are a bit confusing:
> > 
> > - ESR_ELx_ERET_ISS_ERET really indicates that we have trapped an
> >   ERETA* instruction, as opposed to an ERET
> > 
> > - ESR_ELx_ERET_ISS_ERETA reallu indicates that we have trapped
> >   an ERETAB instruction, as opposed to an ERETAA.
> > 
> > Repaint the two helpers such as:
> > 
> > - ESR_ELx_ERET_ISS_ERET becomes ESR_ELx_ERET_ISS_ERETA
> > 
> > - ESR_ELx_ERET_ISS_ERETA becomes ESR_ELx_ERET_ISS_ERETAB
> > 
> > At the same time, use BIT() instead of raw values.
> > 
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> 
> I'm somewhat against this, as the original names are what the Arm
> ARM specifies.

I don't disagree, but that doesn't make the ARM ARM right! ;-)

> 
> > ---
> >  arch/arm64/include/asm/esr.h | 4 ++--
> >  arch/arm64/kvm/handle_exit.c | 2 +-
> >  2 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
> > index 353fe08546cf..72c7810ccf2c 100644
> > --- a/arch/arm64/include/asm/esr.h
> > +++ b/arch/arm64/include/asm/esr.h
> > @@ -290,8 +290,8 @@
> >  		 ESR_ELx_SYS64_ISS_OP2_SHIFT))
> >  
> >  /* ISS field definitions for ERET/ERETAA/ERETAB trapping */
> > -#define ESR_ELx_ERET_ISS_ERET		0x2
> > -#define ESR_ELx_ERET_ISS_ERETA		0x1
> > +#define ESR_ELx_ERET_ISS_ERETA		BIT(1)
> > +#define ESR_ELx_ERET_ISS_ERETAB		BIT(0)
> >  
> >  /*
> >   * ISS field definitions for floating-point exception traps
> > diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> > index 617ae6dea5d5..0646c623d1da 100644
> > --- a/arch/arm64/kvm/handle_exit.c
> > +++ b/arch/arm64/kvm/handle_exit.c
> > @@ -219,7 +219,7 @@ static int kvm_handle_ptrauth(struct kvm_vcpu *vcpu)
> >  
> >  static int kvm_handle_eret(struct kvm_vcpu *vcpu)
> >  {
> > -	if (kvm_vcpu_get_esr(vcpu) & ESR_ELx_ERET_ISS_ERET)
> > +	if (kvm_vcpu_get_esr(vcpu) & ESR_ELx_ERET_ISS_ERETA)
> 
> If this part is confusing due to the name, maybe introduce a function in esr.h
> esr_is_pac_eret() (name pending bikeshedding)?

That's indeed a better option. Now for the bikeshed aspect:

- esr_iss_is_eretax(): check for ESR_ELx_ERET_ISS_ERET being set

- esr_iss_is_eretab(): check for ESR_ELx_ERET_ISS_ERETA being set

Thoughts?

	M.

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