| Message ID | 20240219092014.783809-3-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM/arm64: Add NV support for ERET and PAuth | expand |
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. > --- > 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)? > return kvm_handle_ptrauth(vcpu); > > /* Thanks, Joey
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.
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@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? > 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
On Tue, 20 Feb 2024 13:23:50 +0000, Joey Gouly <joey.gouly@arm.com> wrote: > > 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@arm.com> wrote: > > > > > > 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(). It's not an either/or situation. We actually need both: - esr_iss_is_eretax() being true tells you that you need to authenticate the ERET - esr_iss_is_eretab() tells you that you need to use the A or B key Thanks, M.
On Tue, Feb 20, 2024 at 01:41:15PM +0000, Marc Zyngier wrote: > On Tue, 20 Feb 2024 13:23:50 +0000, > Joey Gouly <joey.gouly@arm.com> wrote: > > > > 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@arm.com> wrote: > > > > > > > > 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(). > > It's not an either/or situation. We actually need both: > > - esr_iss_is_eretax() being true tells you that you need to > authenticate the ERET > > - esr_iss_is_eretab() tells you that you need to use the A or B key Oh right, yes that makes sense (please add a brief comment like ^ above the functions) Thanks, Joey
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) return kvm_handle_ptrauth(vcpu); /*
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> --- arch/arm64/include/asm/esr.h | 4 ++-- arch/arm64/kvm/handle_exit.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)