Hi Marc,
On Mon, Aug 12, 2024 at 06:58:24PM +0100, Marc Zyngier wrote:
> 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;
The condition looks good now.
> 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...
Agreed!
Thanks,
Alex
