Hi Alex,
On Mon, 12 Aug 2024 16:11:02 +0100,
Alexandru Elisei <alexandru.elisei@xxxxxxx> wrote:
>
> Hi Marc,
>
> On Sat, Aug 10, 2024 at 11:16:15AM +0100, Marc Zyngier wrote:
> > Hi Alex,
> >
> > @@ -136,12 +137,22 @@ static int setup_s1_walk(struct kvm_vcpu *vcpu, u32 op, struct s1_walk_info *wi,
> > va = (u64)sign_extend64(va, 55);
> >
> > /* Let's put the MMU disabled case aside immediately */
> > - if (!(sctlr & SCTLR_ELx_M) ||
> > - (__vcpu_sys_reg(vcpu, HCR_EL2) & HCR_DC)) {
> > + switch (wi->regime) {
> > + case TR_EL10:
> > + if (__vcpu_sys_reg(vcpu, HCR_EL2) & HCR_DC)
> > + wr->level = S1_MMU_DISABLED;
>
> In compute_translation_regime(), for AT instructions other than AT S1E2*, when
> {E2H,TGE} = {0,1}, regime is Regime_EL10. As far as I can tell, when regime is
> Regime_EL10 and TGE is set, stage 1 is disabled, according to
> AArch64.S1Enabled() and the decription of the TGE bit.
Grmbl... I really dislike E2H=0. May it die a painful death. How about
this on top?
diff --git a/arch/arm64/kvm/at.c b/arch/arm64/kvm/at.c
index 10017d990bc3..870e77266f80 100644
--- a/arch/arm64/kvm/at.c
+++ b/arch/arm64/kvm/at.c
@@ -139,7 +139,19 @@ static int setup_s1_walk(struct kvm_vcpu *vcpu, u32 op, struct s1_walk_info *wi,
/* Let's put the MMU disabled case aside immediately */
switch (wi->regime) {
case TR_EL10:
- if (__vcpu_sys_reg(vcpu, HCR_EL2) & HCR_DC)
+ /*
+ * If dealing with the EL1&0 translation regime, 3 things
+ * can disable the S1 translation:
+ *
+ * - HCR_EL2.DC = 0
+ * - HCR_EL2.{E2H,TGE} = {0,1}
+ * - SCTLR_EL1.M = 0
+ *
+ * The TGE part is interesting. If we have decided that this
+ * is EL1&0, then it means that either {E2H,TGE} == {1,0} or
+ * {0,x}, and we only need to test for TGE == 1.
+ */
+ if (__vcpu_sys_reg(vcpu, HCR_EL2) & (HCR_DC | HCR_TGE))
wr->level = S1_MMU_DISABLED;
fallthrough;
case TR_EL2:
[...]
>
> switch (desc & GENMASK_ULL(1, 0)) {
> case 0b00:
> case 0b10:
> goto transfault;
> case 0b01:
> /* Block mapping */
> break;
> default:
> if (level == 3)
> break;
> }
>
> Is this better? Perhaps slightly easier to match against the descriptor layouts,
> but I'm not sure it's an improvement over your suggestion. Up to you, no point
> in bikeshedding over it.
I think I'll leave it as is for now. I'm getting sick of this code...
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
