Re: [PATCH v2] KVM: arm64: Enable S1PIE for hVHE

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2636 bytes --]

On Wed, Sep 09, 2026 at 10:44:26AM +0100, Marc Zyngier wrote:
> Mark Brown <broonie@kernel.org> wrote:

> > hVHE.  Enable FEAT_S1PIE with hVHE only, hVHE is used for protected VMs
> > but there is no real use case for nVHE mode on hardware with this
> > feature.

> That's not the reason. The reason is that there is no nVHE-only
> hardware with PIE, and that on VHE-capable HW, nVHE and hVHE are
> strictly equivalent. Therefore there is no need to add support for HW
> that does not exist.

Right, that's the reason why there is no real use case for nVHE on this
hardware - even where S1PIE capable hardware can run nVHE there is
always a preferable mode that delivers the same functionality.  I'll
make this more explicit.

> > This should have no practical impact other than causing any unexpected
> > encodings to map to no permissions instead of their default
> > meanings.

> What default meanings?

The meanings that are defined when S1PIE is not enabled.  I will make
this more explicit, or given your comments further down about not
sharing code possibly just renumber so it's not a thing any more.

> > +#define KVM_HYP_PIR_IDX(uxn, pxn, dbm, ap1) (((uxn) << 3) | ((pxn) << 2) | \
> > +					     ((dbm) << 1) | (ap1))
> > +

> This is not what these bits are called. They are just PIIndex[] bits.

I was trying to make it easier to map thing between non-S1PIE and S1PIE
encodings, I'll rename and add a comment to help people follow when
updating.

> > +alternative_if ARM64_HAS_S1PIE
> > +	/* S1PIE is only enabled with TCR2_EL2.PIE if we are running hVHE */
> > +	mov_q	x1, KVM_HVHE_PIR_EL2
> > +	msr	REG_PIR_EL2, x1
> > +	msr	REG_PIRE0_EL2, xzr
> > +alternative_else_nop_endif

> > +alternative_if ARM64_HAS_TCR2
> > +	ldr	x1, [x0, #NVHE_INIT_TCR2_EL2]
> > +	msr	REG_TCR2_EL2, x1
> > +alternative_else_nop_endif

> S1PIE implies TCR2. Why the additional alternatives?

That is true but TCR2 does not imply S1PIE, I wrote things this way so
that TCR2 is initialised even if we end up on a system where that is
present but S1PIE is not (or S1PIE is present but has been disabled by a
command line override).  This is during startup so it seemed reasonable
to write things in a straightforward and easy to read fashion.

> TBH, I think this is completely going the wrong way. Why can't we
> write this as a discrete enumeration of the permission combination we
> support (all 3 of them), and map that to the correct index?

I was deliberately following a similar pattern to that used for the host
kernel, intended to minimise code changes.  I will rework so we have an
alternative path for S1PIE rather than trying to share.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]