Re: [PATCH v5 09/15] KVM: arm64: Handle HDBSS faults

[Leonardo Bras <leo.bras@arm.com>]

On Tue, Sep 29, 2026 at 06:36:49PM +0800, Tian Zheng wrote:
> From: Eillon <yezhenyu2@huawei.com>
> 
> Once the buffer is full, hardware stops promoting writable-clean
> descriptors and raises a stage-2 Permission fault with
> ESR_EL2.ISS2.HDBSSF instead. Failed HDBSS accesses, such as external
> aborts and granule protection faults, are reported the same way.
> 
> Dispatch from kvm_handle_guest_abort() via the new esr_iss2_is_hdbssf()
> helper. FSC == OK means the exit path already flushed, so resume the
> guest; any other FSC is an error - clear it and report -EFAULT.
> 
> Signed-off-by: Eillon <yezhenyu2@huawei.com>
> Signed-off-by: Tian Zheng <zhengtian10@huawei.com>
> ---
>  arch/arm64/include/asm/esr.h           |  5 +++++
>  arch/arm64/include/asm/kvm_dirty_bit.h |  1 +
>  arch/arm64/kvm/dirty_bit.c             | 29 ++++++++++++++++++++++++++
>  arch/arm64/kvm/mmu.c                   |  4 ++++
>  4 files changed, 39 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
> index f816f5d77f1a..4b3ccd407faa 100644
> --- a/arch/arm64/include/asm/esr.h
> +++ b/arch/arm64/include/asm/esr.h
> @@ -437,6 +437,11 @@
>  #ifndef __ASSEMBLER__
>  #include <asm/types.h>
> 
> +static inline bool esr_iss2_is_hdbssf(unsigned long esr)
> +{
> +	return !!(ESR_ELx_ISS2(esr) & ESR_ELx_HDBSSF);
> +}
> +
>  static inline unsigned long esr_brk_comment(unsigned long esr)
>  {
>  	return esr & ESR_ELx_BRK64_ISS_COMMENT_MASK;
> diff --git a/arch/arm64/include/asm/kvm_dirty_bit.h b/arch/arm64/include/asm/kvm_dirty_bit.h
> index d828e6b43fe9..eb2039820777 100644
> --- a/arch/arm64/include/asm/kvm_dirty_bit.h
> +++ b/arch/arm64/include/asm/kvm_dirty_bit.h
> @@ -39,5 +39,6 @@ static inline bool vcpu_hdbss_enabled(struct kvm_vcpu *vcpu)
>  int kvm_arm_vcpu_alloc_hdbss(struct kvm_vcpu *vcpu);
>  void kvm_arm_vcpu_free_hdbss(struct kvm_vcpu *vcpu);
>  void kvm_flush_hdbss_buffer(struct kvm_vcpu *vcpu);
> +int kvm_handle_hdbss_fault(struct kvm_vcpu *vcpu);
> 
>  #endif /* __ARM64_KVM_DIRTY_BIT_H__ */
> diff --git a/arch/arm64/kvm/dirty_bit.c b/arch/arm64/kvm/dirty_bit.c
> index be0d12555c84..893a8c4248bc 100644
> --- a/arch/arm64/kvm/dirty_bit.c
> +++ b/arch/arm64/kvm/dirty_bit.c
> @@ -98,3 +98,32 @@ void kvm_flush_hdbss_buffer(struct kvm_vcpu *vcpu)
>  	vcpu->arch.hdbss.hdbssprod_el2 = prod;
>  	isb();
>  }
> +
> +int kvm_handle_hdbss_fault(struct kvm_vcpu *vcpu)
> +{
> +	u64 prod;
> +	u64 fsc;
> +
> +	if (WARN_ON_ONCE(!system_supports_hdbss()))
> +		return -EFAULT;
> +
> +	if (WARN_ON_ONCE(!vcpu_hdbss_enabled(vcpu)))
> +		return -EFAULT;
> +
> +	prod = read_sysreg_s(SYS_HDBSSPROD_EL2);
> +	fsc = FIELD_GET(HDBSSPROD_EL2_FSC_MASK, prod);
> +
> +	if (fsc == HDBSSPROD_EL2_FSC_OK)
> +		/* Buffer full: the exit path drained it before handle_exit. */
> +		return 1;
> +
> +	if (fsc != HDBSSPROD_EL2_FSC_ExternalAbort &&
> +	    fsc != HDBSSPROD_EL2_FSC_GPF)
> +		WARN_ONCE(1,
> +			  "Unexpected HDBSS fault type, FSC: 0x%llx (prod=0x%llx, vcpu=%d)\n",
> +			  fsc, prod, vcpu->vcpu_id);

Any reason for using if() before warn_once, given the first parameter is a 
condition? I mean, why not use, instead:

+	WARN_ONCE(fsc != HDBSSPROD_EL2_FSC_ExternalAbort &&
+		  fsc != HDBSSPROD_EL2_FSC_GPF,
+		  "Unexpected HDBSS fault type, FSC: 0x%llx (prod=0x%llx, vcpu=%d)\n",
+		  fsc, prod, vcpu->vcpu_id);

> +
> +	/* Clear FSC so hardware dirty state updates can resume. */
> +	write_sysreg_s(prod & ~HDBSSPROD_EL2_FSC_MASK, SYS_HDBSSPROD_EL2);
> +	return -EFAULT;
> +}
> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> index 85a98d2c23a9..7bf82d65041c 100644
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c
> @@ -15,6 +15,7 @@
>  #include <asm/pgalloc.h>
>  #include <asm/cacheflush.h>
>  #include <asm/kvm_arm.h>
> +#include <asm/kvm_dirty_bit.h>
>  #include <asm/kvm_mmu.h>
>  #include <asm/kvm_pgtable.h>
>  #include <asm/kvm_pkvm.h>
> @@ -2315,6 +2316,9 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu)
> 
>  	is_iabt = kvm_vcpu_trap_is_iabt(vcpu);
> 
> +	if (esr_iss2_is_hdbssf(esr))
> +		return kvm_handle_hdbss_fault(vcpu);
> +
>  	if (esr_fsc_is_translation_fault(esr)) {
>  		/* Beyond sanitised PARange (which is the IPA limit) */
>  		if (fault_ipa >= BIT_ULL(get_kvm_ipa_limit())) {

I did not get the criteria used to positioning it here:
Why placed before is_abt, if not using the result before returning?
What is the rationale to putting it here?

Thanks!
Leo