diff mbox series

[09/10] KVM: arm64: Handle PIR{,E0}_EL2 traps

Message ID 20240813144738.2048302-10-maz@kernel.org (mailing list archive)
State New, archived
Headers show
Series KVM: arm64: Add EL2 support to FEAT_S1PIE | expand

Commit Message

Marc Zyngier Aug. 13, 2024, 2:47 p.m. UTC 
Add the FEAT_S1PIE EL2 registers the sysreg descriptor array so that
they can be handled as a trap.

Access to these registers is conditionned on ID_AA64MMFR3_EL1.S1PIE
being advertised.

Similarly to other other changes, PIRE0_EL2 is guaranteed to trap
thanks to the D22677 update to the architecture..

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/sys_regs.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Comments

Joey Gouly Aug. 13, 2024, 3:24 p.m. UTC | #1 
On Tue, Aug 13, 2024 at 03:47:37PM +0100, Marc Zyngier wrote:
> Add the FEAT_S1PIE EL2 registers the sysreg descriptor array so that
> they can be handled as a trap.
> 
> Access to these registers is conditionned on ID_AA64MMFR3_EL1.S1PIE
> being advertised.
> 
> Similarly to other other changes, PIRE0_EL2 is guaranteed to trap
> thanks to the D22677 update to the architecture..
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm64/kvm/sys_regs.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 52250db3c122..a5f604e24e05 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -346,6 +346,18 @@ static bool access_rw(struct kvm_vcpu *vcpu,
>  	return true;
>  }
>  
> +static bool check_s1pie_access_rw(struct kvm_vcpu *vcpu,
> +				  struct sys_reg_params *p,
> +				  const struct sys_reg_desc *r)
> +{
> +	if (!kvm_has_feat(vcpu->kvm, ID_AA64MMFR3_EL1, S1PIE, IMP)) {
> +		kvm_inject_undefined(vcpu);
> +		return false;
> +	}
> +
> +	return access_rw(vcpu, p, r);
> +}
> +
>  /*
>   * See note at ARMv7 ARM B1.14.4 (TL;DR: S/W ops are not easily virtualized).
>   */
> @@ -2827,6 +2839,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>  	EL2_REG(HPFAR_EL2, access_rw, reset_val, 0),
>  
>  	EL2_REG(MAIR_EL2, access_rw, reset_val, 0),
> +	EL2_REG(PIRE0_EL2, check_s1pie_access_rw, reset_val, 0),
> +	EL2_REG(PIR_EL2, check_s1pie_access_rw, reset_val, 0),
>  	EL2_REG(AMAIR_EL2, access_rw, reset_val, 0),
>  
>  	EL2_REG(VBAR_EL2, access_rw, reset_val, 0),

I think we should also use this for PIR_EL1 / PIRE0_EL1? We have NULL for their access field.

	{ SYS_DESC(SYS_PIR_EL1), NULL, reset_unknown, PIR_EL1 },
Marc Zyngier Aug. 13, 2024, 3:45 p.m. UTC | #2 
On Tue, 13 Aug 2024 16:24:52 +0100,
Joey Gouly <joey.gouly@arm.com> wrote:
> 
> On Tue, Aug 13, 2024 at 03:47:37PM +0100, Marc Zyngier wrote:
> > Add the FEAT_S1PIE EL2 registers the sysreg descriptor array so that
> > they can be handled as a trap.
> > 
> > Access to these registers is conditionned on ID_AA64MMFR3_EL1.S1PIE
> > being advertised.
> > 
> > Similarly to other other changes, PIRE0_EL2 is guaranteed to trap
> > thanks to the D22677 update to the architecture..
> > 
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> >  arch/arm64/kvm/sys_regs.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> > 
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index 52250db3c122..a5f604e24e05 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -346,6 +346,18 @@ static bool access_rw(struct kvm_vcpu *vcpu,
> >  	return true;
> >  }
> >  
> > +static bool check_s1pie_access_rw(struct kvm_vcpu *vcpu,
> > +				  struct sys_reg_params *p,
> > +				  const struct sys_reg_desc *r)
> > +{
> > +	if (!kvm_has_feat(vcpu->kvm, ID_AA64MMFR3_EL1, S1PIE, IMP)) {
> > +		kvm_inject_undefined(vcpu);
> > +		return false;
> > +	}
> > +
> > +	return access_rw(vcpu, p, r);
> > +}
> > +
> >  /*
> >   * See note at ARMv7 ARM B1.14.4 (TL;DR: S/W ops are not easily virtualized).
> >   */
> > @@ -2827,6 +2839,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> >  	EL2_REG(HPFAR_EL2, access_rw, reset_val, 0),
> >  
> >  	EL2_REG(MAIR_EL2, access_rw, reset_val, 0),
> > +	EL2_REG(PIRE0_EL2, check_s1pie_access_rw, reset_val, 0),
> > +	EL2_REG(PIR_EL2, check_s1pie_access_rw, reset_val, 0),
> >  	EL2_REG(AMAIR_EL2, access_rw, reset_val, 0),
> >  
> >  	EL2_REG(VBAR_EL2, access_rw, reset_val, 0),
> 
> I think we should also use this for PIR_EL1 / PIRE0_EL1? We have NULL for their access field.
> 
> 	{ SYS_DESC(SYS_PIR_EL1), NULL, reset_unknown, PIR_EL1 },

I don't think we need this. In general, the EL1 FEAT_S1PIE registers
are directly accessed by the VM, and do not trap.

However, if the VM has been configured to not expose S1PIE, then we
set the corresponding FGU bits in kvm_calculate_traps():

	if (!kvm_has_feat(kvm, ID_AA64MMFR3_EL1, S1PIE, IMP))
		kvm->arch.fgu[HFGxTR_GROUP] |= (HFGxTR_EL2_nPIRE0_EL1 |
						HFGxTR_EL2_nPIR_EL1);

The effect of this is that we don't even make to the sysreg array, and
inject an UNDEF directly from the point of decoding the trap (see the
beginning of triage_sysreg_trap()).

For EL2 registers, there is no concept of FGT since they always trap,
so no architectural trick we can play to shortcut the handling.
Therefore we make it to the handler and have to triage things there.

Does it make sense?

	M.
Joey Gouly Aug. 13, 2024, 4:19 p.m. UTC | #3 
On Tue, Aug 13, 2024 at 04:45:46PM +0100, Marc Zyngier wrote:
> On Tue, 13 Aug 2024 16:24:52 +0100,
> Joey Gouly <joey.gouly@arm.com> wrote:
> > 
> > On Tue, Aug 13, 2024 at 03:47:37PM +0100, Marc Zyngier wrote:
> > > Add the FEAT_S1PIE EL2 registers the sysreg descriptor array so that
> > > they can be handled as a trap.
> > > 
> > > Access to these registers is conditionned on ID_AA64MMFR3_EL1.S1PIE
> > > being advertised.
> > > 
> > > Similarly to other other changes, PIRE0_EL2 is guaranteed to trap
> > > thanks to the D22677 update to the architecture..
> > > 
> > > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > > ---
> > >  arch/arm64/kvm/sys_regs.c | 14 ++++++++++++++
> > >  1 file changed, 14 insertions(+)
> > > 
> > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > > index 52250db3c122..a5f604e24e05 100644
> > > --- a/arch/arm64/kvm/sys_regs.c
> > > +++ b/arch/arm64/kvm/sys_regs.c
> > > @@ -346,6 +346,18 @@ static bool access_rw(struct kvm_vcpu *vcpu,
> > >  	return true;
> > >  }
> > >  
> > > +static bool check_s1pie_access_rw(struct kvm_vcpu *vcpu,
> > > +				  struct sys_reg_params *p,
> > > +				  const struct sys_reg_desc *r)
> > > +{
> > > +	if (!kvm_has_feat(vcpu->kvm, ID_AA64MMFR3_EL1, S1PIE, IMP)) {
> > > +		kvm_inject_undefined(vcpu);
> > > +		return false;
> > > +	}
> > > +
> > > +	return access_rw(vcpu, p, r);
> > > +}
> > > +
> > >  /*
> > >   * See note at ARMv7 ARM B1.14.4 (TL;DR: S/W ops are not easily virtualized).
> > >   */
> > > @@ -2827,6 +2839,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> > >  	EL2_REG(HPFAR_EL2, access_rw, reset_val, 0),
> > >  
> > >  	EL2_REG(MAIR_EL2, access_rw, reset_val, 0),
> > > +	EL2_REG(PIRE0_EL2, check_s1pie_access_rw, reset_val, 0),
> > > +	EL2_REG(PIR_EL2, check_s1pie_access_rw, reset_val, 0),
> > >  	EL2_REG(AMAIR_EL2, access_rw, reset_val, 0),
> > >  
> > >  	EL2_REG(VBAR_EL2, access_rw, reset_val, 0),
> > 
> > I think we should also use this for PIR_EL1 / PIRE0_EL1? We have NULL for their access field.
> > 
> > 	{ SYS_DESC(SYS_PIR_EL1), NULL, reset_unknown, PIR_EL1 },
> 
> I don't think we need this. In general, the EL1 FEAT_S1PIE registers
> are directly accessed by the VM, and do not trap.
> 
> However, if the VM has been configured to not expose S1PIE, then we
> set the corresponding FGU bits in kvm_calculate_traps():
> 
> 	if (!kvm_has_feat(kvm, ID_AA64MMFR3_EL1, S1PIE, IMP))
> 		kvm->arch.fgu[HFGxTR_GROUP] |= (HFGxTR_EL2_nPIRE0_EL1 |
> 						HFGxTR_EL2_nPIR_EL1);
> 
> The effect of this is that we don't even make to the sysreg array, and
> inject an UNDEF directly from the point of decoding the trap (see the
> beginning of triage_sysreg_trap()).
> 
> For EL2 registers, there is no concept of FGT since they always trap,
> so no architectural trick we can play to shortcut the handling.
> Therefore we make it to the handler and have to triage things there.
> 
> Does it make sense?

Ah yes, forgot how that worked, thanks for the reminder!

There's another 'conditionned' typo in the commit message, but otherwise:

Reviewed-by: Joey Gouly <joey.gouly@arm.com>

Thanks,
Joey
diff mbox series

Patch

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 52250db3c122..a5f604e24e05 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -346,6 +346,18 @@  static bool access_rw(struct kvm_vcpu *vcpu,
 	return true;
 }
 
+static bool check_s1pie_access_rw(struct kvm_vcpu *vcpu,
+				  struct sys_reg_params *p,
+				  const struct sys_reg_desc *r)
+{
+	if (!kvm_has_feat(vcpu->kvm, ID_AA64MMFR3_EL1, S1PIE, IMP)) {
+		kvm_inject_undefined(vcpu);
+		return false;
+	}
+
+	return access_rw(vcpu, p, r);
+}
+
 /*
  * See note at ARMv7 ARM B1.14.4 (TL;DR: S/W ops are not easily virtualized).
  */
@@ -2827,6 +2839,8 @@  static const struct sys_reg_desc sys_reg_descs[] = {
 	EL2_REG(HPFAR_EL2, access_rw, reset_val, 0),
 
 	EL2_REG(MAIR_EL2, access_rw, reset_val, 0),
+	EL2_REG(PIRE0_EL2, check_s1pie_access_rw, reset_val, 0),
+	EL2_REG(PIR_EL2, check_s1pie_access_rw, reset_val, 0),
 	EL2_REG(AMAIR_EL2, access_rw, reset_val, 0),
 
 	EL2_REG(VBAR_EL2, access_rw, reset_val, 0),