On Wed, Sep 11, 2024 at 05:10:04PM +0100, Marc Zyngier wrote:
> On Wed, 11 Sep 2024 16:51:28 +0100,
> Joey Gouly <joey.gouly@xxxxxxx> wrote:
> >
> > On Wed, Sep 11, 2024 at 04:38:45PM +0100, Marc Zyngier wrote:
> > > On Wed, 11 Sep 2024 15:15:13 +0100,
> > > Joey Gouly <joey.gouly@xxxxxxx> wrote:
> > > >
> > > > On Wed, Sep 11, 2024 at 02:51:45PM +0100, Marc Zyngier wrote:
> > >
> > > [...]
> > >
> > > > > +static void compute_s1_hierarchical_permissions(struct kvm_vcpu *vcpu,
> > > > > + struct s1_walk_info *wi,
> > > > > + struct s1_walk_result *wr,
> > > > > + struct s1_perms *s1p)
> > > > > +{
> > > >
> > > > How about:
> > > >
> > > > if (wi->hpd)
> > > > return;
> > >
> > > I was hoping not to add anything like this, because all the table bits
> > > are 0 (we simply don't collect them), and thus don't have any effect.
> >
> > I just thought it was more obvious that they wouldn't apply in this case, don't
> > feel super strongly about it.
>
> If you want it to be obvious, how about this instead?
>
> diff --git a/arch/arm64/kvm/at.c b/arch/arm64/kvm/at.c
> index 8a5e1c4682619..fb9de5fc2cc26 100644
> --- a/arch/arm64/kvm/at.c
> +++ b/arch/arm64/kvm/at.c
> @@ -985,7 +985,8 @@ static void compute_s1_permissions(struct kvm_vcpu *vcpu, u32 op,
> else
> compute_s1_indirect_permissions(vcpu, wi, wr, s1p);
>
> - compute_s1_hierarchical_permissions(vcpu, wi, wr, s1p);
> + if (!wi->hpd)
> + compute_s1_hierarchical_permissions(vcpu, wi, wr, s1p);
>
> if (op == OP_AT_S1E1RP || op == OP_AT_S1E1WP) {
> bool pan;
>
That's even better, if you're happy to include it.
>
> Thanks,
>
> M.
Thanks,
J.
>
> --
> Without deviation from the norm, progress is not possible.
