Re: [PATCH 5/5] KVM: arm64: nVHE: Remove unneeded isb() when modifying PMSCR_EL1

From: Alexandru Elisei <alexandru.elisei@arm.com>
To: Will Deacon <will@kernel.org>
Cc: maz@kernel.org, kvmarm@lists.cs.columbia.edu,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 5/5] KVM: arm64: nVHE: Remove unneeded isb() when modifying PMSCR_EL1
Date: Thu, 15 Jul 2021 10:31:29 +0100	[thread overview]
Message-ID: <17eead71-5db4-403c-eff0-e45d9b9f7a31@arm.com> (raw)
In-Reply-To: <20210714182036.GA32475@willie-the-truck>

Hi Will,

On 7/14/21 7:20 PM, Will Deacon wrote:
> On Wed, Jul 14, 2021 at 10:56:01AM +0100, Alexandru Elisei wrote:
>> According to ARM DDI 0487G.a, page D9-2930, profiling is disabled when
>> the PE is executing at a higher exception level than the profiling
>> buffer owning exception level. This is also confirmed by the pseudocode
>> for the StatisticalProfilingEnabled() function.
>>
>> During the world switch and before activating guest traps, KVM executes
>> at EL2 with the buffer owning exception level being EL1 (MDCR_EL2.E2PB =
>> 0b11). As a result, profiling is already disabled when draining the
>> buffer, making the isb() after the write to PMSCR_EL1 unnecessary.
>>
>> CC: Will Deacon <will@kernel.org>
>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
>> ---
>>  arch/arm64/kvm/hyp/nvhe/debug-sr.c | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/arch/arm64/kvm/hyp/nvhe/debug-sr.c b/arch/arm64/kvm/hyp/nvhe/debug-sr.c
>> index 7d3f25868cae..fdf0e0ba17e9 100644
>> --- a/arch/arm64/kvm/hyp/nvhe/debug-sr.c
>> +++ b/arch/arm64/kvm/hyp/nvhe/debug-sr.c
>> @@ -33,7 +33,6 @@ static void __debug_save_spe(u64 *pmscr_el1)
>>  	/* Yes; save the control register and disable data generation */
>>  	*pmscr_el1 = read_sysreg_s(SYS_PMSCR_EL1);
>>  	write_sysreg_s(0, SYS_PMSCR_EL1);
>> -	isb();
> Hmm, but we still need an ISB somewhere between clearing pmscr_el1 and
> mdcr_el2.e2pb, right? Where does that occur?

Yes, we do need an ISB to make sure we don't start profiling using the EL2&0
translation regime, but with a buffer pointer programmed by the host at EL1 which
is most likely not even mapped at EL2.

When I wrote the patch, I reasoned that the ISB in
__sysreg_restore_state_nvhe->__sysreg_restore_el1_state and the isb from
__load_stage2 will make sure that PMSCR_EL1 is cleared before the change to the
buffer owning regime.

As I was double checking that just now, I realized that *both* ISBs are executed
only if the system has ARM64_WORKAROUND_SPECULATIVE_AT. No ISB gets executed when
the workaround is not needed. We could make the ISB here depend on the system not
having the workaround, but it looks to me like there's little to be gained from
that (just one less ISB when the workaround is applied), at the expense of making
the code even more difficult to reason about.

My preference would be to drop this patch.

Thanks,

Alex

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm