Re: [PATCH v2 6/6] selftests: KVM: Test OS lock behavior

[Marc Zyngier <maz@kernel.org>]

Hi Oliver,

On Tue, 02 Nov 2021 09:46:51 +0000,
Oliver Upton <oupton@google.com> wrote:
> 
> KVM now correctly handles the OS Lock for its guests. When set, KVM
> blocks all debug exceptions originating from the guest. Add test cases
> to the debug-exceptions test to assert that software breakpoint,
> hardware breakpoint, watchpoint, and single-step exceptions are in fact
> blocked.
> 
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  .../selftests/kvm/aarch64/debug-exceptions.c  | 58 ++++++++++++++++++-
>  1 file changed, 56 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> index e5e6c92b60da..6b6ff81cdd23 100644
> --- a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> +++ b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> @@ -23,7 +23,7 @@
>  #define SPSR_D		(1 << 9)
>  #define SPSR_SS		(1 << 21)
>  
> -extern unsigned char sw_bp, hw_bp, bp_svc, bp_brk, hw_wp, ss_start;
> +extern unsigned char sw_bp, hw_bp, hw_bp2, bp_svc, bp_brk, hw_wp, ss_start;
>  static volatile uint64_t sw_bp_addr, hw_bp_addr;
>  static volatile uint64_t wp_addr, wp_data_addr;
>  static volatile uint64_t svc_addr;
> @@ -47,6 +47,14 @@ static void reset_debug_state(void)
>  	isb();
>  }
>  
> +static void enable_os_lock(void)
> +{
> +	write_sysreg(oslar_el1, 1);
> +	isb();
> +
> +	GUEST_ASSERT(read_sysreg(oslsr_el1) & 2);
> +}
> +
>  static void install_wp(uint64_t addr)
>  {
>  	uint32_t wcr;
> @@ -99,6 +107,7 @@ static void guest_code(void)
>  	GUEST_SYNC(0);
>  
>  	/* Software-breakpoint */
> +	reset_debug_state();
>  	asm volatile("sw_bp: brk #0");
>  	GUEST_ASSERT_EQ(sw_bp_addr, PC(sw_bp));
>  
> @@ -152,6 +161,51 @@ static void guest_code(void)
>  	GUEST_ASSERT_EQ(ss_addr[1], PC(ss_start) + 4);
>  	GUEST_ASSERT_EQ(ss_addr[2], PC(ss_start) + 8);
>  
> +	GUEST_SYNC(6);
> +
> +	/* OS Lock blocking software-breakpoint */
> +	reset_debug_state();
> +	enable_os_lock();
> +	sw_bp_addr = 0;
> +	asm volatile("brk #0");
> +	GUEST_ASSERT_EQ(sw_bp_addr, 0);

I haven't had a change to properly review the series, but this one
definitely caught my eye. My expectations are that BRK is *not*
affected by the OS Lock. The ARMv8 ARM goes as far as saying:

<quote>
Breakpoint Instruction exceptions are enabled regardless of the state
of the OS Lock and the OS Double Lock.
</quote>

as well as:

<quote>
There is no enable control for Breakpoint Instruction exceptions. They
are always enabled, and cannot be masked.
</quote>

I wonder how your test succeeds, though.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.