Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 29 May 2023 09:33:14 +0100
Message-ID: <86edmzczat.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Akihiko Odaki <akihiko.odaki@daynix.com>
Cc:     James Morse <james.morse@arm.com>,
        Suzuki K Poulose <suzuki.poulose@arm.com>,
        Oliver Upton <oliver.upton@linux.dev>,
        Zenghui Yu <yuzenghui@huawei.com>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will@kernel.org>,
        linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] KVM: arm64: Populate fault info for watchpoint
In-Reply-To: <20230529024847.8671-1-akihiko.odaki@daynix.com>
References: <20230529024847.8671-1-akihiko.odaki@daynix.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/28.2
 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Mon, 29 May 2023 03:48:47 +0100,
Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> 
> When handling ESR_ELx_EC_WATCHPT_LOW, far_el2 member of struct
> kvm_vcpu_fault_info will be copied to far member of struct
> kvm_debug_exit_arch and exposed to the userspace. The userspace will
> see stale values from older faults if the fault info does not get
> populated.

Nice catch! Some bike-shedding below...

> 
> Fixes: 8fb2046180a0 ("KVM: arm64: Move early handlers to per-EC handlers")
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>  arch/arm64/kvm/hyp/include/hyp/switch.h | 2 +-
>  arch/arm64/kvm/hyp/nvhe/switch.c        | 6 ++++--
>  arch/arm64/kvm/hyp/vhe/switch.c         | 3 ++-
>  3 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h
> index 07d37ff88a3f..2eb0a1481ead 100644
> --- a/arch/arm64/kvm/hyp/include/hyp/switch.h
> +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h
> @@ -351,7 +351,7 @@ static bool kvm_hyp_handle_cp15_32(struct kvm_vcpu *vcpu, u64 *exit_code)
>  	return false;
>  }
>  
> -static bool kvm_hyp_handle_iabt_low(struct kvm_vcpu *vcpu, u64 *exit_code)
> +static bool kvm_hyp_handle_generic_fault(struct kvm_vcpu *vcpu, u64 *exit_code)
>  {
>  	if (!__populate_fault_info(vcpu))
>  		return true;
> diff --git a/arch/arm64/kvm/hyp/nvhe/switch.c b/arch/arm64/kvm/hyp/nvhe/switch.c
> index c2cb46ca4fb6..1b77866079dc 100644
> --- a/arch/arm64/kvm/hyp/nvhe/switch.c
> +++ b/arch/arm64/kvm/hyp/nvhe/switch.c
> @@ -184,8 +184,9 @@ static const exit_handler_fn hyp_exit_handlers[] = {
>  	[ESR_ELx_EC_SYS64]		= kvm_hyp_handle_sysreg,
>  	[ESR_ELx_EC_SVE]		= kvm_hyp_handle_fpsimd,
>  	[ESR_ELx_EC_FP_ASIMD]		= kvm_hyp_handle_fpsimd,
> -	[ESR_ELx_EC_IABT_LOW]		= kvm_hyp_handle_iabt_low,
> +	[ESR_ELx_EC_IABT_LOW]		= kvm_hyp_handle_generic_fault,
>  	[ESR_ELx_EC_DABT_LOW]		= kvm_hyp_handle_dabt_low,
> +	[ESR_ELx_EC_WATCHPT_LOW]	= kvm_hyp_handle_generic_fault,
>  	[ESR_ELx_EC_PAC]		= kvm_hyp_handle_ptrauth,
>  };

It is slightly odd to merge instruction abort with watchpoint faults,
as the later is solely concerned with data, and not instructions.

I'd rather we don't completely unify the handlers, even if they have
the same implementation. There will be various differences in handling
with NV which will result in diverging implementations.

So either duplicate the code (it wouldn't make a huge difference), or
add kvm_hyp_handle_iabt_low() and kvm_hyp_handle_watchpt_low() as
aliases of kvm_hyp_handle_generic_fault (which would probably be
better renamed as kvm_hyp_handle_memory_fault). And then use that as
the prologue to the dabt handler. Something like the patch below.

Thanks,

	M.

diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h
index 0a344184f26c..928983726a62 100644
--- a/arch/arm64/kvm/hyp/include/hyp/switch.h
+++ b/arch/arm64/kvm/hyp/include/hyp/switch.h
@@ -414,17 +414,21 @@ static bool kvm_hyp_handle_cp15_32(struct kvm_vcpu *vcpu, u64 *exit_code)
 	return false;
 }
 
-static bool kvm_hyp_handle_iabt_low(struct kvm_vcpu *vcpu, u64 *exit_code)
+static bool kvm_hyp_handle_memory_fault(struct kvm_vcpu *vcpu, u64 *exit_code)
 {
 	if (!__populate_fault_info(vcpu))
 		return true;
 
 	return false;
 }
+static bool kvm_hyp_handle_iabt_low(struct kvm_vcpu *vcpu, u64 *exit_code)
+	__alias(kvm_hyp_handle_memory_fault);
+static bool kvm_hyp_handle_watchpt_low(struct kvm_vcpu *vcpu, u64 *exit_code)
+	__alias(kvm_hyp_handle_memory_fault);
 
 static bool kvm_hyp_handle_dabt_low(struct kvm_vcpu *vcpu, u64 *exit_code)
 {
-	if (!__populate_fault_info(vcpu))
+	if (kvm_hyp_handle_memory_fault(vcpu, exit_code))
 		return true;
 
 	if (static_branch_unlikely(&vgic_v2_cpuif_trap)) {
diff --git a/arch/arm64/kvm/hyp/nvhe/switch.c b/arch/arm64/kvm/hyp/nvhe/switch.c
index 7730860552da..b357ba7c453d 100644
--- a/arch/arm64/kvm/hyp/nvhe/switch.c
+++ b/arch/arm64/kvm/hyp/nvhe/switch.c
@@ -186,6 +186,7 @@ static const exit_handler_fn hyp_exit_handlers[] = {
 	[ESR_ELx_EC_FP_ASIMD]		= kvm_hyp_handle_fpsimd,
 	[ESR_ELx_EC_IABT_LOW]		= kvm_hyp_handle_iabt_low,
 	[ESR_ELx_EC_DABT_LOW]		= kvm_hyp_handle_dabt_low,
+	[ESR_ELx_EC_WATCHPT_LOW]	= kvm_hyp_handle_watchpt_low,
 	[ESR_ELx_EC_PAC]		= kvm_hyp_handle_ptrauth,
 };
 
@@ -196,6 +197,7 @@ static const exit_handler_fn pvm_exit_handlers[] = {
 	[ESR_ELx_EC_FP_ASIMD]		= kvm_hyp_handle_fpsimd,
 	[ESR_ELx_EC_IABT_LOW]		= kvm_hyp_handle_iabt_low,
 	[ESR_ELx_EC_DABT_LOW]		= kvm_hyp_handle_dabt_low,
+	[ESR_ELx_EC_WATCHPT_LOW]	= kvm_hyp_handle_watchpt_low,
 	[ESR_ELx_EC_PAC]		= kvm_hyp_handle_ptrauth,
 };
 
diff --git a/arch/arm64/kvm/hyp/vhe/switch.c b/arch/arm64/kvm/hyp/vhe/switch.c
index e03c3413cf49..46f0fce9b01f 100644
--- a/arch/arm64/kvm/hyp/vhe/switch.c
+++ b/arch/arm64/kvm/hyp/vhe/switch.c
@@ -311,6 +311,7 @@ static const exit_handler_fn hyp_exit_handlers[] = {
 	[ESR_ELx_EC_FP_ASIMD]		= kvm_hyp_handle_fpsimd,
 	[ESR_ELx_EC_IABT_LOW]		= kvm_hyp_handle_iabt_low,
 	[ESR_ELx_EC_DABT_LOW]		= kvm_hyp_handle_dabt_low,
+	[ESR_ELx_EC_WATCHPT_LOW]	= kvm_hyp_handle_watchpt_low,
 	[ESR_ELx_EC_PAC]		= kvm_hyp_handle_ptrauth,
 	[ESR_ELx_EC_ERET]		= kvm_hyp_handle_eret,
 };

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