Re: [PATCH v2 12/16] KVM: arm64: Implement AT S1PIE support

[Marc Zyngier <maz@kernel.org>]

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.

-- 
Without deviation from the norm, progress is not possible.