diff mbox series

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

Message ID 20210714095601.184854-6-alexandru.elisei@arm.com (mailing list archive)
State New, archived
Headers show
Series KVM: arm64: Cleanups and one optimization | expand

Commit Message

Alexandru Elisei July 14, 2021, 9:56 a.m. UTC
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(-)

Comments

Will Deacon July 14, 2021, 6:20 p.m. UTC | #1
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?

Will
Alexandru Elisei July 15, 2021, 9:31 a.m. UTC | #2
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
diff mbox series

Patch

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();
 
 	/* Now drain all buffered data to memory */
 	psb_csync();