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

[Suzuki K Poulose]

On 03/03/2026 14:39, Will Deacon wrote:
> 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().

Of course, it was. I should have added "not from your change".


Now we have:

__kvm_vcpu_run () {

   ..guest_exit..

//load_host_stage2() if required -> pkvm.

__sysreg_restore_state_nvhe() -> __sysreg_restore_el1_state () #- we 
restore "host MMU context" here

  ...
__debug_restore_host_buffers_nvhe() -> __debug_restore_spe(); #issue isb 
here to sync the  host context

}

and the isb() is placed in the spe restore code. All of this is correct
and I do understand why it is there. My comment was, this is not easily
relatable and prone to errors with inadvertent changes (e.g., moving the
spe around etc). But I agree that we can avoid the unconditional
overhead over the code readability.

Cheers
Suzuki

> 
> 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