Re: [PATCH v2 1/8] KVM: arm64: Remove VM-wide VNCR mapping counter

[Marc Zyngier <maz@kernel.org>]

On Fri, 07 Aug 2026 17:45:17 +0100,
"Lorenzo Stoakes (ARM)" <ljs@kernel.org> wrote:
> 
> Bear with me being verbose here, as this is both nascent review + learning
> :)

No worries.

> 
> On Thu, Aug 06, 2026 at 10:10:19AM +0100, Marc Zyngier wrote:
> > The global VNCR mapping counter is used to decide whether an L1
> > provided VNCR page is mapped in L0 on any CPU at the point of
> > dealing with a TLB invalidation. It is incremented when a mapping
> > is made in the fixmap, and decremented when unmapped.
> >
> > As it turns out, this tracking has several flaws:
> >
> > - we are trying to invalidate TLBs, and the mapping is only an
> >   opportunistic consequence of the TLB. Checking this counter to
> >   decide whether a TLB needs to be invalidated may result in missed
> >   invalidations.
> 
> Is it largely the self-invalidation mentioned below or are there other cases?

No. The self-invalidation is only an additional consequence outlining
that even the most basic requirements cannot be honoured.

The thing to realise is that is that we are dealing with two separate
"objects" when it comes to VNCR:

- a SW TLB, which represent the guest VA to guest IPA to host PA
  translations. This is the vncr_tlb structure, populated as we walk
  the S1/S2 page tables. This structure's lifetime is controlled by
  TLB invalidations from the guest, MMU notifiers from the host, and
  natural eviction (there is only one such structure per vcpu).

- a shadow page table that implements the translation at *runtime*, as
  described by vncr_tlb. This is the per-CPU fixmap mapping. It's
  lifetime is at most a vcpu_load/vcpu_put cycle, but it can also be
  torn down by TLBI and notifiers.

vncr_map_count only tracks the latter, not the former. Which means
that if the mapping is not live on a CPU at the point of TLBI on *any*
CPU, nothing will happen. That's a blatant violation of the
architecture, and it could lead to memory corruption in the guest.

The obvious fix is in patch 8, tracking the TLBs rather than the
mappings.

>
> >
> > - an L1 vcpu invalidating its own TLB (a very likely case) will not
> >   succeed in invalidating the VNCR pseudo TLB because that page is
> >   not mapped in L0 at this stage.
> 
> Ahh yes this is pretty compelling then!
> 
> >
> > Given that this tracking fails at delivering the minimum guarantees
> > that are required and is only a performance optimisation, remove it
> > completely.
> >
> > Fixes: 4ffa72ad8f37e ("KVM: arm64: nv: Add S1 TLB invalidation primitive for VNCR_EL2")
> > Reviewed-by: Yuan Yao <yaoyuan@linux.alibaba.com>
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> 
> The change LGTM, it neatly removes the described mechanism which is well
> evidenced.
> 
> Comments below that are largely me talking out loud as I learn things :)
> 
> Acked-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>
>
> > Cc: stable@vger.kernel.org
> > ---
> >  arch/arm64/include/asm/kvm_host.h | 3 ---
> >  arch/arm64/kvm/hyp/vhe/switch.c   | 3 +--
> >  arch/arm64/kvm/nested.c           | 3 ---
> >  3 files changed, 1 insertion(+), 8 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index bae2c4f92ef5c..ac16f96c878d6 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -411,9 +411,6 @@ struct kvm_arch {
> >  	/* Masks for VNCR-backed and general EL2 sysregs */
> >  	struct kvm_sysreg_masks	*sysreg_masks;
> >
> > -	/* Count the number of VNCR_EL2 currently mapped */
> > -	atomic_t vncr_map_count;
> > -
> >  	/*
> >  	 * For an untrusted host VM, 'pkvm.handle' is used to lookup
> >  	 * the associated pKVM instance in the hypervisor.
> > diff --git a/arch/arm64/kvm/hyp/vhe/switch.c b/arch/arm64/kvm/hyp/vhe/switch.c
> > index bbe9cebd3d9d5..c09b1d411c584 100644
> > --- a/arch/arm64/kvm/hyp/vhe/switch.c
> > +++ b/arch/arm64/kvm/hyp/vhe/switch.c
> > @@ -427,8 +427,7 @@ static bool kvm_hyp_handle_tlbi_el2(struct kvm_vcpu *vcpu, u64 *exit_code)
> >  	 * If we have to check for any VNCR mapping being invalidated,
> >  	 * go back to the slow path for further processing.
> >  	 */
> > -	if (vcpu_el2_e2h_is_set(vcpu) && vcpu_el2_tge_is_set(vcpu) &&
> > -	    atomic_read(&vcpu->kvm->arch.vncr_map_count))
> > +	if (vcpu_el2_e2h_is_set(vcpu) && vcpu_el2_tge_is_set(vcpu))
> >  		return false;
> 
> So this seems to be the crux of it - seems to be 'is there any possibility that
> we will need to check for VNCR mappings being invalidated?'

Yup. If the guest's state is HCR_EL2.{E2H,TGE}={1,1}, then this is the
guest hypervisor invalidating TLBs for itself (and not its guest), and
returning 'false' takes us on the slow path where we'll have to look
at the individual vcnt_tlb structures to find out if there is any overlap.

> 
> Checks:
> 
> * vcpu_el2_e2h_is_set() - is the guest host kernel (?)'s hcr_el2.e2h
>   enabled? From what I gather hcr_el2.e2h is what allows sysreg_EL1 ->
>   sysreg_EL2 for the host kernel to allow unmodified kernels to run in EL2.

Amongst other things, yes. Please see FEAT_VHE in the ARM ARM for a
description of what HCR_EL2.E2H==1 implies.

>
>   IOW - is the guest host kernel VHE?

Note that for a KVM guest, if FEAT_VHE is present, the FEAT_E2H0 is
not implemented, which means that E2H is RES1. As an additional
restriction, we only expose NV to VHE guests. This greatly simplifies
the scope of what we need to support, and conveniently hides a bunch
of architecture defects that cannot be otherwise mitigated.

> 
> * vcpu_el2_tge_is_set() - Similarly tests for the hcr_el2.tge bit - and this
>   seems to be is 'EL1 -> EL2 redirection on?' - IOW - is this a kernel running
>   in EL2?

Not quite. We know for sure this is running at virtual EL2 (we trapped
with HCR_EL2.NV set, for a start). In the TLBI case, HCR_EL2.TGE
controls whether the behaviour of a TLBI S1E1* instruction targets the
host (TGE==1) or the guest (TGE==0).

> 
> Actually I see in is_hyp_ctxt():
> 
> 	 * We are in a hypervisor context if the vcpu mode is EL2 or
> 	 * E2H and TGE bits are set. The latter means we are in the user space
> 	 * of the VHE kernel. ARMv8.1 ARM describes this as 'InHost'
> 
> So I _think_ the combination of the two is checking to see if you're the L0
> kernel that _could_ send TLBi's that need to be handled?

The combination of the two bits indicates: is this a VHE hypervisor
invalidating TLBs for itself. If yes, then we need to check the VNCR
SW TLBs to complete the job.

> Previously it seemed the logic was 'if there are no VNCR mappings present then
> we can optimise by short-circuiting the rest of the processing in
> kvm_hyp_handle_sysreg_vhe()'.

Exactly.

> 
> It seems that the hardware TLBi has been processed by now so it's actually more
> like - there's still work to be done maintaining the software TLB and that's
> done elsewhere.

Yup.

[...]

> I think this is all vaguely sane :)

You have been assimilated.

Thanks for reading thus far!

	M.

-- 
Jazz isn't dead. It just smells funny.