Re: [PATCH v2 2/3] KVM: arm64: Disable SPE Profiling Buffer when running in guest context

[Will Deacon <will@kernel.org>]

On Tue, Mar 03, 2026 at 09:48:06AM +0000, Suzuki K Poulose wrote:
> On 27/02/2026 21:21, Will Deacon wrote:
> > diff --git a/arch/arm64/kvm/hyp/nvhe/debug-sr.c b/arch/arm64/kvm/hyp/nvhe/debug-sr.c
> > index 3dbdee1148d3..75158a9cd06a 100644
> > --- a/arch/arm64/kvm/hyp/nvhe/debug-sr.c
> > +++ b/arch/arm64/kvm/hyp/nvhe/debug-sr.c
> > @@ -14,20 +14,20 @@
> >   #include <asm/kvm_hyp.h>
> >   #include <asm/kvm_mmu.h>
> > -static void __debug_save_spe(u64 *pmscr_el1)
> > +static void __debug_save_spe(void)
> >   {
> > -	u64 reg;
> > +	u64 *pmscr_el1, *pmblimitr_el1;
> > -	/* Clear pmscr in case of early return */
> > -	*pmscr_el1 = 0;
> > +	pmscr_el1 = host_data_ptr(host_debug_state.pmscr_el1);
> > +	pmblimitr_el1 = host_data_ptr(host_debug_state.pmblimitr_el1);
> >   	/*
> >   	 * At this point, we know that this CPU implements
> >   	 * SPE and is available to the host.
> >   	 * Check if the host is actually using it ?
> >   	 */
> > -	reg = read_sysreg_s(SYS_PMBLIMITR_EL1);
> > -	if (!(reg & BIT(PMBLIMITR_EL1_E_SHIFT)))
> > +	*pmblimitr_el1 = read_sysreg_s(SYS_PMBLIMITR_EL1);
> > +	if (!(*pmblimitr_el1 & BIT(PMBLIMITR_EL1_E_SHIFT)))
> >   		return;
> >   	/* Yes; save the control register and disable data generation */
> > @@ -37,18 +37,29 @@ static void __debug_save_spe(u64 *pmscr_el1)
> >   	/* Now drain all buffered data to memory */
> >   	psb_csync();
> > +	dsb(nsh);
> > +
> > +	/* And disable the profiling buffer */
> > +	write_sysreg_s(0, SYS_PMBLIMITR_EL1);
> > +	isb();
> >   }
> > -static void __debug_restore_spe(u64 pmscr_el1)
> > +static void __debug_restore_spe(void)
> >   {
> > -	if (!pmscr_el1)
> > +	u64 pmblimitr_el1 = *host_data_ptr(host_debug_state.pmblimitr_el1);
> > +
> > +	if (!(pmblimitr_el1 & BIT(PMBLIMITR_EL1_E_SHIFT)))
> >   		return;
> >   	/* The host page table is installed, but not yet synchronised */
> >   	isb();
> 
> minor nit: This seems buried deep down in a helper (with no context of what
> else could have happened since the host context has been restored)
> and for now it looks correct,  but is prone to inadvertent changes
> causing issues or making this obsolete. With the isb() following LIMITR,
> wouldn't that be  sufficient ?

I'm just inherting this from the existing upstream code -- see the isb()
in the existing implementation of __debug_restore_spe().

The isb() is needed to ensure that SPE can't start making out-of-context
translation table walks (which can occur once PMBLIMITR_EL1.E is set)
before the stage-2 MMU is restored back to the host configuration (e.g.
by clearing HCR_EL2.VM for nVHE or by restoring VTCR and VTTBR for
pKVM). We want to predicate it on SPE being enabled, otherwise it's
unconditional overhead, so I don't think we can move it.

Will