| Message ID | 20240903153834.1909472-13-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Add EL2 support to FEAT_S1PIE | expand |
Hello Marc! On Tue, Sep 03, 2024 at 04:38:30PM +0100, Marc Zyngier wrote: > It doesn't take much effort to imple,emt S1PIE support in AT. > This 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 | 136 +++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 135 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/kvm/at.c b/arch/arm64/kvm/at.c > index 68f5b89598ec..bd7e1b32b049 100644 > --- a/arch/arm64/kvm/at.c > +++ b/arch/arm64/kvm/at.c > @@ -736,6 +736,23 @@ static u64 compute_par_s1(struct kvm_vcpu *vcpu, struct s1_walk_result *wr, > return par; > } > > +static bool s1pie_enabled(struct kvm_vcpu *vcpu, enum trans_regime regime) > +{ > + if (!kvm_has_feat(vcpu->kvm, ID_AA64MMFR3_EL1, S1PIE, IMP)) > + return false; > + > + switch (regime) { > + case TR_EL2: > + case TR_EL20: > + return __vcpu_sys_reg(vcpu, TCR2_EL2) & TCR2_EL2_PIE; > + case TR_EL10: > + return (__vcpu_sys_reg(vcpu, HCRX_EL2) & HCRX_EL2_TCR2En) && > + (__vcpu_sys_reg(vcpu, TCR2_EL1) & TCR2_EL1x_PIE); > + default: > + BUG(); > + } > +} > + > static bool pan3_enabled(struct kvm_vcpu *vcpu, enum trans_regime regime) > { > u64 sctlr; > @@ -743,6 +760,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 > @@ -826,12 +846,126 @@ static void compute_s1_hierarchical_permissions(struct kvm_vcpu *vcpu, > } > } > > +#define pi_idx(v, r, i) ((__vcpu_sys_reg((v), (r)) >> ((i) * 4)) & 0xf) > + > +#define set_priv_perms(p, r, w, x) \ > + do { \ > + (p)->pr = (r); \ > + (p)->pw = (w); \ > + (p)->px = (x); \ > + } while (0) > + > +#define set_unpriv_perms(p, r, w, x) \ > + do { \ > + (p)->ur = (r); \ > + (p)->uw = (w); \ > + (p)->ux = (x); \ > + } while (0) > + > +/* Similar to AArch64.S1IndirectBasePermissions(), without GCS */ > +#define set_perms(w, p, ip) \ > + do { \ > + switch ((ip)) { \ > + case 0b0000: \ > + set_ ## w ## _perms((p), false, false, false); \ > + break; \ > + case 0b0001: \ > + set_ ## w ## _perms((p), true , false, false); \ > + break; \ > + case 0b0010: \ > + set_ ## w ## _perms((p), false, false, true ); \ > + break; \ > + case 0b0011: \ > + set_ ## w ## _perms((p), true , false, true ); \ > + break; \ > + case 0b0100: \ > + set_ ## w ## _perms((p), false, false, false); \ > + break; \ > + case 0b0101: \ > + set_ ## w ## _perms((p), true , true , false); \ > + break; \ > + case 0b0110: \ > + set_ ## w ## _perms((p), true , true , true ); \ > + break; \ > + case 0b0111: \ > + set_ ## w ## _perms((p), true , true , true ); \ > + break; \ > + case 0b1000: \ > + set_ ## w ## _perms((p), true , false, false); \ > + break; \ > + case 0b1001: \ > + set_ ## w ## _perms((p), true , false, false); \ > + break; \ > + case 0b1010: \ > + set_ ## w ## _perms((p), true , false, true ); \ > + break; \ > + case 0b1011: \ > + set_ ## w ## _perms((p), false, false, false); \ > + break; \ > + case 0b1100: \ > + set_ ## w ## _perms((p), true , true , false); \ > + break; \ > + case 0b1101: \ > + set_ ## w ## _perms((p), false, false, false); \ > + break; \ > + case 0b1110: \ > + set_ ## w ## _perms((p), true , true , true ); \ > + break; \ > + case 0b1111: \ > + set_ ## w ## _perms((p), 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, > + struct s1_perms *s1p) > +{ > + u8 up, pp, idx; > + > + idx = (FIELD_GET(GENMASK(54, 53), wr->desc) << 2 | > + FIELD_GET(BIT(51), wr->desc) << 1 | > + FIELD_GET(BIT(6), wr->desc)); > + > + switch (wi->regime) { > + case TR_EL10: > + pp = pi_idx(vcpu, PIR_EL1, idx); > + up = pi_idx(vcpu, PIRE0_EL1, idx); > + break; > + case TR_EL20: > + pp = pi_idx(vcpu, PIR_EL2, idx); > + up = pi_idx(vcpu, PIRE0_EL2, idx); > + break; > + case TR_EL2: > + pp = pi_idx(vcpu, PIR_EL2, idx); > + up = 0; > + break; > + } > + > + set_perms(priv, s1p, pp); > + > + if (wi->regime != TR_EL2) > + set_perms(unpriv, s1p, up); > + else > + set_unpriv_perms(s1p, false, false, false); > + > + if (s1p->px && s1p->uw) { > + set_priv_perms(s1p, false, false, false); > + set_unpriv_perms(s1p, false, false, false); > + } > +} > + > static void compute_s1_permissions(struct kvm_vcpu *vcpu, u32 op, > struct s1_walk_info *wi, > struct s1_walk_result *wr, > struct s1_perms *s1p) > { > - compute_s1_direct_permissions(vcpu, wi, wr, s1p); > + if (!s1pie_enabled(vcpu, wi->regime)) > + compute_s1_direct_permissions(vcpu, wi, wr, s1p); > + else > + compute_s1_indirect_permissions(vcpu, wi, wr, s1p); > + > compute_s1_hierarchical_permissions(vcpu, wi, wr, s1p); Is this (and the previous patch to split this up) right? Looking at this from the ARM ARM (ARM DDI 0487K.a): R JHSVW If Indirect permissions are used, then hierarchical permissions are disabled and TCR_ELx.HPDn are RES 1. > > if (op == OP_AT_S1E1RP || op == OP_AT_S1E1WP) { > -- Thanks, Joey
Hi Joey, Thanks for having a look. On Thu, 05 Sep 2024 14:58:20 +0100, Joey Gouly <joey.gouly@arm.com> wrote: > > Hello Marc! > > > static void compute_s1_permissions(struct kvm_vcpu *vcpu, u32 op, > > struct s1_walk_info *wi, > > struct s1_walk_result *wr, > > struct s1_perms *s1p) > > { > > - compute_s1_direct_permissions(vcpu, wi, wr, s1p); > > + if (!s1pie_enabled(vcpu, wi->regime)) > > + compute_s1_direct_permissions(vcpu, wi, wr, s1p); > > + else > > + compute_s1_indirect_permissions(vcpu, wi, wr, s1p); > > + > > compute_s1_hierarchical_permissions(vcpu, wi, wr, s1p); > > Is this (and the previous patch to split this up) right? > > Looking at this from the ARM ARM (ARM DDI 0487K.a): > > R JHSVW If Indirect permissions are used, then hierarchical > permissions are disabled and TCR_ELx.HPDn are RES 1. Odd. I was convinced that it was when S1POE is enabled that HPs were disabled. But you are absolutely right, and it is once more proven that I can't read. Oh well. Not to worry, I've since found other issues with this series. I have forgotten the patch dealing with the fast path on another branch, and since decided that TCR2_EL2 needed extra care to cope with individual features being disabled. The rework is still useful, as I'm looking at POE as well, but I need to hoist the HP stuff up a notch. I'll repost things once I've sorted these things up. Thanks again, M.
On Thu, Sep 05, 2024 at 03:57:13PM +0100, Marc Zyngier wrote: > Hi Joey, > > Thanks for having a look. > > On Thu, 05 Sep 2024 14:58:20 +0100, > Joey Gouly <joey.gouly@arm.com> wrote: > > > > Hello Marc! > > > > > static void compute_s1_permissions(struct kvm_vcpu *vcpu, u32 op, > > > struct s1_walk_info *wi, > > > struct s1_walk_result *wr, > > > struct s1_perms *s1p) > > > { > > > - compute_s1_direct_permissions(vcpu, wi, wr, s1p); > > > + if (!s1pie_enabled(vcpu, wi->regime)) > > > + compute_s1_direct_permissions(vcpu, wi, wr, s1p); > > > + else > > > + compute_s1_indirect_permissions(vcpu, wi, wr, s1p); > > > + > > > compute_s1_hierarchical_permissions(vcpu, wi, wr, s1p); > > > > Is this (and the previous patch to split this up) right? > > > > Looking at this from the ARM ARM (ARM DDI 0487K.a): > > > > R JHSVW If Indirect permissions are used, then hierarchical > > permissions are disabled and TCR_ELx.HPDn are RES 1. > > Odd. I was convinced that it was when S1POE is enabled that HPs were > disabled. But you are absolutely right, and it is once more proven > that I can't read. Oh well. For POE there is: RBVXDG Hierarchical Permissions are disabled and the TCR_ELx.{HPD0, HPD1} bits are RES1 for stage 1 of a translation regime using VMSAv8-64 if one or more of POE and E0POE (for EL1&0, EL2&0) is enabled for that translation regime. > > Not to worry, I've since found other issues with this series. I have > forgotten the patch dealing with the fast path on another branch, and > since decided that TCR2_EL2 needed extra care to cope with individual > features being disabled. > > The rework is still useful, as I'm looking at POE as well, but I need > to hoist the HP stuff up a notch. > > I'll repost things once I've sorted these things up. I think the rest of this patch looked fine though. > > Thanks again, > > M. > > -- > Without deviation from the norm, progress is not possible.
On Thu, 05 Sep 2024 16:37:34 +0100, Joey Gouly <joey.gouly@arm.com> wrote: > > On Thu, Sep 05, 2024 at 03:57:13PM +0100, Marc Zyngier wrote: > > Hi Joey, > > > > Thanks for having a look. > > > > On Thu, 05 Sep 2024 14:58:20 +0100, > > Joey Gouly <joey.gouly@arm.com> wrote: > > > > > > Hello Marc! > > > > > > > static void compute_s1_permissions(struct kvm_vcpu *vcpu, u32 op, > > > > struct s1_walk_info *wi, > > > > struct s1_walk_result *wr, > > > > struct s1_perms *s1p) > > > > { > > > > - compute_s1_direct_permissions(vcpu, wi, wr, s1p); > > > > + if (!s1pie_enabled(vcpu, wi->regime)) > > > > + compute_s1_direct_permissions(vcpu, wi, wr, s1p); > > > > + else > > > > + compute_s1_indirect_permissions(vcpu, wi, wr, s1p); > > > > + > > > > compute_s1_hierarchical_permissions(vcpu, wi, wr, s1p); > > > > > > Is this (and the previous patch to split this up) right? > > > > > > Looking at this from the ARM ARM (ARM DDI 0487K.a): > > > > > > R JHSVW If Indirect permissions are used, then hierarchical > > > permissions are disabled and TCR_ELx.HPDn are RES 1. > > > > Odd. I was convinced that it was when S1POE is enabled that HPs were > > disabled. But you are absolutely right, and it is once more proven > > that I can't read. Oh well. > > For POE there is: > > RBVXDG Hierarchical Permissions are disabled and the > TCR_ELx.{HPD0, HPD1} bits are RES1 for stage 1 of a > translation regime using VMSAv8-64 if one or more of POE and > E0POE (for EL1&0, EL2&0) is enabled for that translation > regime. Right, this is the one I had at the back of my head. At the end of the day, it really looks like hierarchical permissions are as dead as the proverbial dodo, and their sole purpose left is to antagonise implementations. Hopefully someone realises that and eventually allows implementations to build TCR_ELx.HPD* as RES1. > > Not to worry, I've since found other issues with this series. I have > > forgotten the patch dealing with the fast path on another branch, and > > since decided that TCR2_EL2 needed extra care to cope with individual > > features being disabled. > > > > The rework is still useful, as I'm looking at POE as well, but I need > > to hoist the HP stuff up a notch. > > > > I'll repost things once I've sorted these things up. > > I think the rest of this patch looked fine though. Cool, thanks for having given it a look! M.
diff --git a/arch/arm64/kvm/at.c b/arch/arm64/kvm/at.c index 68f5b89598ec..bd7e1b32b049 100644 --- a/arch/arm64/kvm/at.c +++ b/arch/arm64/kvm/at.c @@ -736,6 +736,23 @@ static u64 compute_par_s1(struct kvm_vcpu *vcpu, struct s1_walk_result *wr, return par; } +static bool s1pie_enabled(struct kvm_vcpu *vcpu, enum trans_regime regime) +{ + if (!kvm_has_feat(vcpu->kvm, ID_AA64MMFR3_EL1, S1PIE, IMP)) + return false; + + switch (regime) { + case TR_EL2: + case TR_EL20: + return __vcpu_sys_reg(vcpu, TCR2_EL2) & TCR2_EL2_PIE; + case TR_EL10: + return (__vcpu_sys_reg(vcpu, HCRX_EL2) & HCRX_EL2_TCR2En) && + (__vcpu_sys_reg(vcpu, TCR2_EL1) & TCR2_EL1x_PIE); + default: + BUG(); + } +} + static bool pan3_enabled(struct kvm_vcpu *vcpu, enum trans_regime regime) { u64 sctlr; @@ -743,6 +760,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 @@ -826,12 +846,126 @@ static void compute_s1_hierarchical_permissions(struct kvm_vcpu *vcpu, } } +#define pi_idx(v, r, i) ((__vcpu_sys_reg((v), (r)) >> ((i) * 4)) & 0xf) + +#define set_priv_perms(p, r, w, x) \ + do { \ + (p)->pr = (r); \ + (p)->pw = (w); \ + (p)->px = (x); \ + } while (0) + +#define set_unpriv_perms(p, r, w, x) \ + do { \ + (p)->ur = (r); \ + (p)->uw = (w); \ + (p)->ux = (x); \ + } while (0) + +/* Similar to AArch64.S1IndirectBasePermissions(), without GCS */ +#define set_perms(w, p, ip) \ + do { \ + switch ((ip)) { \ + case 0b0000: \ + set_ ## w ## _perms((p), false, false, false); \ + break; \ + case 0b0001: \ + set_ ## w ## _perms((p), true , false, false); \ + break; \ + case 0b0010: \ + set_ ## w ## _perms((p), false, false, true ); \ + break; \ + case 0b0011: \ + set_ ## w ## _perms((p), true , false, true ); \ + break; \ + case 0b0100: \ + set_ ## w ## _perms((p), false, false, false); \ + break; \ + case 0b0101: \ + set_ ## w ## _perms((p), true , true , false); \ + break; \ + case 0b0110: \ + set_ ## w ## _perms((p), true , true , true ); \ + break; \ + case 0b0111: \ + set_ ## w ## _perms((p), true , true , true ); \ + break; \ + case 0b1000: \ + set_ ## w ## _perms((p), true , false, false); \ + break; \ + case 0b1001: \ + set_ ## w ## _perms((p), true , false, false); \ + break; \ + case 0b1010: \ + set_ ## w ## _perms((p), true , false, true ); \ + break; \ + case 0b1011: \ + set_ ## w ## _perms((p), false, false, false); \ + break; \ + case 0b1100: \ + set_ ## w ## _perms((p), true , true , false); \ + break; \ + case 0b1101: \ + set_ ## w ## _perms((p), false, false, false); \ + break; \ + case 0b1110: \ + set_ ## w ## _perms((p), true , true , true ); \ + break; \ + case 0b1111: \ + set_ ## w ## _perms((p), 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, + struct s1_perms *s1p) +{ + u8 up, pp, idx; + + idx = (FIELD_GET(GENMASK(54, 53), wr->desc) << 2 | + FIELD_GET(BIT(51), wr->desc) << 1 | + FIELD_GET(BIT(6), wr->desc)); + + switch (wi->regime) { + case TR_EL10: + pp = pi_idx(vcpu, PIR_EL1, idx); + up = pi_idx(vcpu, PIRE0_EL1, idx); + break; + case TR_EL20: + pp = pi_idx(vcpu, PIR_EL2, idx); + up = pi_idx(vcpu, PIRE0_EL2, idx); + break; + case TR_EL2: + pp = pi_idx(vcpu, PIR_EL2, idx); + up = 0; + break; + } + + set_perms(priv, s1p, pp); + + if (wi->regime != TR_EL2) + set_perms(unpriv, s1p, up); + else + set_unpriv_perms(s1p, false, false, false); + + if (s1p->px && s1p->uw) { + set_priv_perms(s1p, false, false, false); + set_unpriv_perms(s1p, false, false, false); + } +} + static void compute_s1_permissions(struct kvm_vcpu *vcpu, u32 op, struct s1_walk_info *wi, struct s1_walk_result *wr, struct s1_perms *s1p) { - compute_s1_direct_permissions(vcpu, wi, wr, s1p); + if (!s1pie_enabled(vcpu, wi->regime)) + compute_s1_direct_permissions(vcpu, wi, wr, s1p); + else + compute_s1_indirect_permissions(vcpu, wi, wr, s1p); + compute_s1_hierarchical_permissions(vcpu, wi, wr, s1p); if (op == OP_AT_S1E1RP || op == OP_AT_S1E1WP) {
It doesn't take much effort to imple,emt S1PIE support in AT. This 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 | 136 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 135 insertions(+), 1 deletion(-)