| Message ID | 20241023145345.1613824-22-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Add EL2 support to FEAT_S1PIE/S1POE | expand |
On Wed, Oct 23, 2024 at 03:53:29PM +0100, Marc Zyngier wrote: > It doesn't take much effort to implement S1PIE support in AT. > > It is only a matter of using the AArch64.S1IndirectBasePermissions() > encodings for the permission, ignoring GCS which has no impact on AT, > and enforce FEAT_PAN3 being enabled as this is a requirement of > FEAT_S1PIE. > > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > arch/arm64/kvm/at.c | 117 +++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 116 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/kvm/at.c b/arch/arm64/kvm/at.c > index f5bd750288ff5..3d93ed1795603 100644 > --- a/arch/arm64/kvm/at.c > +++ b/arch/arm64/kvm/at.c > @@ -781,6 +781,9 @@ static bool pan3_enabled(struct kvm_vcpu *vcpu, enum trans_regime regime) > if (!kvm_has_feat(vcpu->kvm, ID_AA64MMFR1_EL1, PAN, PAN3)) > return false; > > + if (s1pie_enabled(vcpu, regime)) > + return true; > + > if (regime == TR_EL10) > sctlr = vcpu_read_sys_reg(vcpu, SCTLR_EL1); > else > @@ -862,11 +865,123 @@ static void compute_s1_hierarchical_permissions(struct kvm_vcpu *vcpu, > } > } > > +#define perm_idx(v, r, i) ((vcpu_read_sys_reg((v), (r)) >> ((i) * 4)) & 0xf) > + > +#define set_priv_perms(wr, r, w, x) \ > + do { \ > + (wr)->pr = (r); \ > + (wr)->pw = (w); \ > + (wr)->px = (x); \ > + } while (0) > + > +#define set_unpriv_perms(wr, r, w, x) \ > + do { \ > + (wr)->ur = (r); \ > + (wr)->uw = (w); \ > + (wr)->ux = (x); \ > + } while (0) > + > +/* Similar to AArch64.S1IndirectBasePermissions(), without GCS */ > +#define set_perms(w, wr, ip) \ > + do { \ > + /* R_LLZDZ */ \ > + switch ((ip)) { \ > + case 0b0000: \ > + set_ ## w ## _perms((wr), false, false, false); \ > + break; \ > + case 0b0001: \ > + set_ ## w ## _perms((wr), true , false, false); \ > + break; \ > + case 0b0010: \ > + set_ ## w ## _perms((wr), false, false, true ); \ > + break; \ > + case 0b0011: \ > + set_ ## w ## _perms((wr), true , false, true ); \ > + break; \ > + case 0b0100: \ > + set_ ## w ## _perms((wr), false, false, false); \ > + break; \ > + case 0b0101: \ > + set_ ## w ## _perms((wr), true , true , false); \ > + break; \ > + case 0b0110: \ > + set_ ## w ## _perms((wr), true , true , true ); \ > + break; \ > + case 0b0111: \ > + set_ ## w ## _perms((wr), true , true , true ); \ > + break; \ > + case 0b1000: \ > + set_ ## w ## _perms((wr), true , false, false); \ > + break; \ > + case 0b1001: \ > + set_ ## w ## _perms((wr), true , false, false); \ > + break; \ > + case 0b1010: \ > + set_ ## w ## _perms((wr), true , false, true ); \ > + break; \ > + case 0b1011: \ > + set_ ## w ## _perms((wr), false, false, false); \ > + break; \ > + case 0b1100: \ > + set_ ## w ## _perms((wr), true , true , false); \ > + break; \ > + case 0b1101: \ > + set_ ## w ## _perms((wr), false, false, false); \ > + break; \ > + case 0b1110: \ > + set_ ## w ## _perms((wr), true , true , true ); \ > + break; \ > + case 0b1111: \ > + set_ ## w ## _perms((wr), false, false, false); \ > + break; \ > + } \ > + } while (0) > + > +static void compute_s1_indirect_permissions(struct kvm_vcpu *vcpu, > + struct s1_walk_info *wi, > + struct s1_walk_result *wr) > +{ > + u8 up, pp, idx; > + > + idx = pte_pi_index(wr->desc); > + > + switch (wi->regime) { > + case TR_EL10: > + pp = perm_idx(vcpu, PIR_EL1, idx); > + up = perm_idx(vcpu, PIRE0_EL1, idx); > + break; > + case TR_EL20: > + pp = perm_idx(vcpu, PIR_EL2, idx); > + up = perm_idx(vcpu, PIRE0_EL2, idx); > + break; > + case TR_EL2: > + pp = perm_idx(vcpu, PIR_EL2, idx); > + up = 0; > + break; > + } There seems to be inconsistent use of default: BUG(); when switching on wi->regime. > + > + set_perms(priv, wr, pp); > + > + if (wi->regime != TR_EL2) > + set_perms(unpriv, wr, up); > + else > + set_unpriv_perms(wr, false, false, false); When regime == TR_EL2, up == 0, so the if/else should do the same thing? Maybe you've done that intentionally to be more explicit. Either way: Reviewed-by: Joey Gouly <joey.gouly@arm.com> > + > + /* R_VFPJF */ > + if (wr->px && wr->uw) { > + set_priv_perms(wr, false, false, false); > + set_unpriv_perms(wr, false, false, false); > + } > +} > + > static void compute_s1_permissions(struct kvm_vcpu *vcpu, u32 op, > struct s1_walk_info *wi, > struct s1_walk_result *wr) > { > - compute_s1_direct_permissions(vcpu, wi, wr); > + if (!s1pie_enabled(vcpu, wi->regime)) > + compute_s1_direct_permissions(vcpu, wi, wr); > + else > + compute_s1_indirect_permissions(vcpu, wi, wr); > > if (!wi->hpd) > compute_s1_hierarchical_permissions(vcpu, wi, wr); Thanks, Joey
On Thu, 24 Oct 2024 14:59:25 +0100, Joey Gouly <joey.gouly@arm.com> wrote: > > On Wed, Oct 23, 2024 at 03:53:29PM +0100, Marc Zyngier wrote: > > It doesn't take much effort to implement S1PIE support in AT. > > > > It is only a matter of using the AArch64.S1IndirectBasePermissions() > > encodings for the permission, ignoring GCS which has no impact on AT, > > and enforce FEAT_PAN3 being enabled as this is a requirement of > > FEAT_S1PIE. > > > > Signed-off-by: Marc Zyngier <maz@kernel.org> > > --- > > arch/arm64/kvm/at.c | 117 +++++++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 116 insertions(+), 1 deletion(-) > > > > diff --git a/arch/arm64/kvm/at.c b/arch/arm64/kvm/at.c > > index f5bd750288ff5..3d93ed1795603 100644 > > --- a/arch/arm64/kvm/at.c > > +++ b/arch/arm64/kvm/at.c > > @@ -781,6 +781,9 @@ static bool pan3_enabled(struct kvm_vcpu *vcpu, enum trans_regime regime) > > if (!kvm_has_feat(vcpu->kvm, ID_AA64MMFR1_EL1, PAN, PAN3)) > > return false; > > > > + if (s1pie_enabled(vcpu, regime)) > > + return true; > > + > > if (regime == TR_EL10) > > sctlr = vcpu_read_sys_reg(vcpu, SCTLR_EL1); > > else > > @@ -862,11 +865,123 @@ static void compute_s1_hierarchical_permissions(struct kvm_vcpu *vcpu, > > } > > } > > > > +#define perm_idx(v, r, i) ((vcpu_read_sys_reg((v), (r)) >> ((i) * 4)) & 0xf) > > + > > +#define set_priv_perms(wr, r, w, x) \ > > + do { \ > > + (wr)->pr = (r); \ > > + (wr)->pw = (w); \ > > + (wr)->px = (x); \ > > + } while (0) > > + > > +#define set_unpriv_perms(wr, r, w, x) \ > > + do { \ > > + (wr)->ur = (r); \ > > + (wr)->uw = (w); \ > > + (wr)->ux = (x); \ > > + } while (0) > > + > > +/* Similar to AArch64.S1IndirectBasePermissions(), without GCS */ > > +#define set_perms(w, wr, ip) \ > > + do { \ > > + /* R_LLZDZ */ \ > > + switch ((ip)) { \ > > + case 0b0000: \ > > + set_ ## w ## _perms((wr), false, false, false); \ > > + break; \ > > + case 0b0001: \ > > + set_ ## w ## _perms((wr), true , false, false); \ > > + break; \ > > + case 0b0010: \ > > + set_ ## w ## _perms((wr), false, false, true ); \ > > + break; \ > > + case 0b0011: \ > > + set_ ## w ## _perms((wr), true , false, true ); \ > > + break; \ > > + case 0b0100: \ > > + set_ ## w ## _perms((wr), false, false, false); \ > > + break; \ > > + case 0b0101: \ > > + set_ ## w ## _perms((wr), true , true , false); \ > > + break; \ > > + case 0b0110: \ > > + set_ ## w ## _perms((wr), true , true , true ); \ > > + break; \ > > + case 0b0111: \ > > + set_ ## w ## _perms((wr), true , true , true ); \ > > + break; \ > > + case 0b1000: \ > > + set_ ## w ## _perms((wr), true , false, false); \ > > + break; \ > > + case 0b1001: \ > > + set_ ## w ## _perms((wr), true , false, false); \ > > + break; \ > > + case 0b1010: \ > > + set_ ## w ## _perms((wr), true , false, true ); \ > > + break; \ > > + case 0b1011: \ > > + set_ ## w ## _perms((wr), false, false, false); \ > > + break; \ > > + case 0b1100: \ > > + set_ ## w ## _perms((wr), true , true , false); \ > > + break; \ > > + case 0b1101: \ > > + set_ ## w ## _perms((wr), false, false, false); \ > > + break; \ > > + case 0b1110: \ > > + set_ ## w ## _perms((wr), true , true , true ); \ > > + break; \ > > + case 0b1111: \ > > + set_ ## w ## _perms((wr), false, false, false); \ > > + break; \ > > + } \ > > + } while (0) > > + > > +static void compute_s1_indirect_permissions(struct kvm_vcpu *vcpu, > > + struct s1_walk_info *wi, > > + struct s1_walk_result *wr) > > +{ > > + u8 up, pp, idx; > > + > > + idx = pte_pi_index(wr->desc); > > + > > + switch (wi->regime) { > > + case TR_EL10: > > + pp = perm_idx(vcpu, PIR_EL1, idx); > > + up = perm_idx(vcpu, PIRE0_EL1, idx); > > + break; > > + case TR_EL20: > > + pp = perm_idx(vcpu, PIR_EL2, idx); > > + up = perm_idx(vcpu, PIRE0_EL2, idx); > > + break; > > + case TR_EL2: > > + pp = perm_idx(vcpu, PIR_EL2, idx); > > + up = 0; > > + break; > > + } > > There seems to be inconsistent use of > > default: > BUG(); > > when switching on wi->regime. True. Maybe I should drop them all apart from the one in setup_s1_walk(). > > > + > > + set_perms(priv, wr, pp); > > + > > + if (wi->regime != TR_EL2) > > + set_perms(unpriv, wr, up); > > + else > > + set_unpriv_perms(wr, false, false, false); > > When regime == TR_EL2, up == 0, so the if/else should do the same thing? Maybe > you've done that intentionally to be more explicit. The reason for doing so was not to give the impression that we were actively using the unprivileged indirect permissions for TR_EL2. But maybe that's be just as clear with a comment. > > Either way: > > Reviewed-by: Joey Gouly <joey.gouly@arm.com> Thanks! M.
diff --git a/arch/arm64/kvm/at.c b/arch/arm64/kvm/at.c index f5bd750288ff5..3d93ed1795603 100644 --- a/arch/arm64/kvm/at.c +++ b/arch/arm64/kvm/at.c @@ -781,6 +781,9 @@ static bool pan3_enabled(struct kvm_vcpu *vcpu, enum trans_regime regime) if (!kvm_has_feat(vcpu->kvm, ID_AA64MMFR1_EL1, PAN, PAN3)) return false; + if (s1pie_enabled(vcpu, regime)) + return true; + if (regime == TR_EL10) sctlr = vcpu_read_sys_reg(vcpu, SCTLR_EL1); else @@ -862,11 +865,123 @@ static void compute_s1_hierarchical_permissions(struct kvm_vcpu *vcpu, } } +#define perm_idx(v, r, i) ((vcpu_read_sys_reg((v), (r)) >> ((i) * 4)) & 0xf) + +#define set_priv_perms(wr, r, w, x) \ + do { \ + (wr)->pr = (r); \ + (wr)->pw = (w); \ + (wr)->px = (x); \ + } while (0) + +#define set_unpriv_perms(wr, r, w, x) \ + do { \ + (wr)->ur = (r); \ + (wr)->uw = (w); \ + (wr)->ux = (x); \ + } while (0) + +/* Similar to AArch64.S1IndirectBasePermissions(), without GCS */ +#define set_perms(w, wr, ip) \ + do { \ + /* R_LLZDZ */ \ + switch ((ip)) { \ + case 0b0000: \ + set_ ## w ## _perms((wr), false, false, false); \ + break; \ + case 0b0001: \ + set_ ## w ## _perms((wr), true , false, false); \ + break; \ + case 0b0010: \ + set_ ## w ## _perms((wr), false, false, true ); \ + break; \ + case 0b0011: \ + set_ ## w ## _perms((wr), true , false, true ); \ + break; \ + case 0b0100: \ + set_ ## w ## _perms((wr), false, false, false); \ + break; \ + case 0b0101: \ + set_ ## w ## _perms((wr), true , true , false); \ + break; \ + case 0b0110: \ + set_ ## w ## _perms((wr), true , true , true ); \ + break; \ + case 0b0111: \ + set_ ## w ## _perms((wr), true , true , true ); \ + break; \ + case 0b1000: \ + set_ ## w ## _perms((wr), true , false, false); \ + break; \ + case 0b1001: \ + set_ ## w ## _perms((wr), true , false, false); \ + break; \ + case 0b1010: \ + set_ ## w ## _perms((wr), true , false, true ); \ + break; \ + case 0b1011: \ + set_ ## w ## _perms((wr), false, false, false); \ + break; \ + case 0b1100: \ + set_ ## w ## _perms((wr), true , true , false); \ + break; \ + case 0b1101: \ + set_ ## w ## _perms((wr), false, false, false); \ + break; \ + case 0b1110: \ + set_ ## w ## _perms((wr), true , true , true ); \ + break; \ + case 0b1111: \ + set_ ## w ## _perms((wr), false, false, false); \ + break; \ + } \ + } while (0) + +static void compute_s1_indirect_permissions(struct kvm_vcpu *vcpu, + struct s1_walk_info *wi, + struct s1_walk_result *wr) +{ + u8 up, pp, idx; + + idx = pte_pi_index(wr->desc); + + switch (wi->regime) { + case TR_EL10: + pp = perm_idx(vcpu, PIR_EL1, idx); + up = perm_idx(vcpu, PIRE0_EL1, idx); + break; + case TR_EL20: + pp = perm_idx(vcpu, PIR_EL2, idx); + up = perm_idx(vcpu, PIRE0_EL2, idx); + break; + case TR_EL2: + pp = perm_idx(vcpu, PIR_EL2, idx); + up = 0; + break; + } + + set_perms(priv, wr, pp); + + if (wi->regime != TR_EL2) + set_perms(unpriv, wr, up); + else + set_unpriv_perms(wr, false, false, false); + + /* R_VFPJF */ + if (wr->px && wr->uw) { + set_priv_perms(wr, false, false, false); + set_unpriv_perms(wr, false, false, false); + } +} + static void compute_s1_permissions(struct kvm_vcpu *vcpu, u32 op, struct s1_walk_info *wi, struct s1_walk_result *wr) { - compute_s1_direct_permissions(vcpu, wi, wr); + if (!s1pie_enabled(vcpu, wi->regime)) + compute_s1_direct_permissions(vcpu, wi, wr); + else + compute_s1_indirect_permissions(vcpu, wi, wr); if (!wi->hpd) compute_s1_hierarchical_permissions(vcpu, wi, wr);
It doesn't take much effort to implement S1PIE support in AT. It is only a matter of using the AArch64.S1IndirectBasePermissions() encodings for the permission, ignoring GCS which has no impact on AT, and enforce FEAT_PAN3 being enabled as this is a requirement of FEAT_S1PIE. Signed-off-by: Marc Zyngier <maz@kernel.org> --- arch/arm64/kvm/at.c | 117 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 116 insertions(+), 1 deletion(-)