On Tue, Feb 20, 2024 at 12:29:30PM +0000, Marc Zyngier wrote:
> On Tue, 20 Feb 2024 11:31:27 +0000,
> Joey Gouly <joey.gouly@xxxxxxx> 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@xxxxxxxxxx>
> >
> > 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?
>
I was trying to avoid the ERETA* confusion by suggesting 'pac_eret', but if I
were to pick between your options I'd pick esr_iss_is_eretax().
Thanks,
Joey
