[PATCH] KVM: riscv: Fix NACL hfence entry update order

[PATCH] KVM: riscv: Fix NACL hfence entry update order

[Zongmin Zhou]

From: Zongmin Zhou <zhouzongmin@kylinos.cn>

The SBI v3.0 specification (section 15.1.2) requires a nested HFENCE
entry to be populated as follows:
1) find an unused entry with Config.Pending == 0
2) update the Page_Number and Page_Count words
3) update the Config word with Config.Pending set

__kvm_riscv_nacl_hfence() writes the Config word first, so the SBI
implementation (or NACL hardware) can observe a pending entry with
pnum/pcount values left over from the previous use of that entry,
resulting in incorrect TLB flush ranges.

Write pnum and pcount first and the Config word last. Since the
consumer is an external agent on coherent shared memory, use
WRITE_ONCE() to stop the compiler from reordering the stores and
smp_wmb() to make the parameter words globally visible before the
Pending bit is set.

Fixes: d466c19cead5 ("RISC-V: KVM: Add common nested acceleration support")
Signed-off-by: Zongmin Zhou <zhouzongmin@kylinos.cn>
---
 arch/riscv/kvm/nacl.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/arch/riscv/kvm/nacl.c b/arch/riscv/kvm/nacl.c
index 9aff03c4f667..a5cda9a65156 100644
--- a/arch/riscv/kvm/nacl.c
+++ b/arch/riscv/kvm/nacl.c
@@ -42,12 +42,24 @@ void __kvm_riscv_nacl_hfence(void *shmem,
 		}
 	}
 
-	entp = shmem + SBI_NACL_SHMEM_HFENCE_ENTRY_CONFIG(i);
-	*entp = cpu_to_lelong(control);
+	/*
+	 * Per SBI v3.0 section 15.1.2, the Page_Number and Page_Count
+	 * words must be updated before the Config word with its Pending
+	 * bit set. WRITE_ONCE() stops the compiler from reordering the
+	 * stores and smp_wmb() makes the parameter words globally
+	 * visible to the SBI implementation (or NACL hardware) before
+	 * the Pending bit is set.
+	 */
 	entp = shmem + SBI_NACL_SHMEM_HFENCE_ENTRY_PNUM(i);
-	*entp = cpu_to_lelong(page_num);
+	WRITE_ONCE(*entp, cpu_to_lelong(page_num));
 	entp = shmem + SBI_NACL_SHMEM_HFENCE_ENTRY_PCOUNT(i);
-	*entp = cpu_to_lelong(page_count);
+	WRITE_ONCE(*entp, cpu_to_lelong(page_count));
+
+	/* Ensure the parameter words are visible before the Pending bit */
+	smp_wmb();
+
+	entp = shmem + SBI_NACL_SHMEM_HFENCE_ENTRY_CONFIG(i);
+	WRITE_ONCE(*entp, cpu_to_lelong(control));
 }
 
 int kvm_riscv_nacl_enable(void)
-- 
2.34.1


No virus found
		Checked by Hillstone Network AntiVirus

Re: [PATCH] KVM: riscv: Fix NACL hfence entry update order

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Using `smp_wmb()` instead of `dma_wmb()` or `wmb()` to synchronize with a hardware device or external agent.

Pre-existing issues:
- [High] The retry loop in `__kvm_riscv_nacl_hfence` can spin indefinitely because `try_count` is never decremented.
--

commit 7e01a77077e49fef54c573574fad3624c0817345
Author: Zongmin Zhou <zhouzongmin@kylinos.cn>

KVM: riscv: Fix NACL hfence entry update order

The commit fixes the order in which nested HFENCE entries are populated
by writing the page number and count first, then the config word. It uses
WRITE_ONCE() and smp_wmb() to prevent hardware or firmware from observing
incomplete entries.

[Severity: High]
This isn't a bug introduced by this patch, but looking at the retry logic in
arch/riscv/kvm/nacl.c:__kvm_riscv_nacl_hfence():

    if (ent < 0) {
        if (try_count) {
            nacl_sync_hfence(-1UL);
            goto again;
        } else {
            ...

Does this loop run indefinitely? I noticed try_count is initialized to 5
but doesn't appear to be decremented anywhere in this path. If the NACL
hardware or SBI implementation stalls and fails to clear pending entries,
could this cause a host CPU soft lockup?

> diff --git a/arch/riscv/kvm/nacl.c b/arch/riscv/kvm/nacl.c
> index 08a95ad9ada2c..f922e9f62fee3 100644
> --- a/arch/riscv/kvm/nacl.c
> +++ b/arch/riscv/kvm/nacl.c
[ ... ]
> @@ -42,12 +42,24 @@ void __kvm_riscv_nacl_hfence(void *shmem,
>  		}
>  	}
>  
> -	entp = shmem + SBI_NACL_SHMEM_HFENCE_ENTRY_CONFIG(i);
> -	*entp = cpu_to_lelong(control);
> +	/*
> +	 * Per SBI v3.0 section 15.1.2, the Page_Number and Page_Count
> +	 * words must be updated before the Config word with its Pending
> +	 * bit set. WRITE_ONCE() stops the compiler from reordering the
> +	 * stores and smp_wmb() makes the parameter words globally
> +	 * visible to the SBI implementation (or NACL hardware) before
> +	 * the Pending bit is set.
> +	 */
>  	entp = shmem + SBI_NACL_SHMEM_HFENCE_ENTRY_PNUM(i);
> -	*entp = cpu_to_lelong(page_num);
> +	WRITE_ONCE(*entp, cpu_to_lelong(page_num));
>  	entp = shmem + SBI_NACL_SHMEM_HFENCE_ENTRY_PCOUNT(i);
> -	*entp = cpu_to_lelong(page_count);
> +	WRITE_ONCE(*entp, cpu_to_lelong(page_count));
> +
> +	/* Ensure the parameter words are visible before the Pending bit */
> +	smp_wmb();
> +

[Severity: High]
Does smp_wmb() provide the necessary hardware ordering guarantees on all
configurations?

Looking at arch/riscv/kvm/nacl.c:__kvm_riscv_nacl_hfence(), since the
memory is coherent and shared with an external agent or NACL hardware, I am
concerned about non-SMP kernel builds (!CONFIG_SMP) where smp_wmb()
degrades to a simple compiler barrier (barrier()) and emits no actual
hardware fence instructions.

Would it be safer to use dma_wmb() or wmb() here to ensure the memory
ordering is strictly enforced against external hardware, regardless of
the kernel's SMP configuration?

> +	entp = shmem + SBI_NACL_SHMEM_HFENCE_ENTRY_CONFIG(i);
> +	WRITE_ONCE(*entp, cpu_to_lelong(control));
>  }
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826075009.68952-1-min_halo@163.com?part=1