[kvm-unit-tests PATCH 11/10] riscv: sbi: Add fwft pte_hw_ad_updating test
Clément Léger
cleger at rivosinc.com
Thu Feb 27 01:09:42 PST 2025
On 26/02/2025 18:59, Andrew Jones wrote:
> Add SBI FWFT tests for the PTE_HW_AD_UPDATING feature.
>
> Signed-off-by: Andrew Jones <andrew.jones at linux.dev>
> ---
> lib/riscv/asm/mmu.h | 3 +
> lib/riscv/mmu.c | 7 ++
> riscv/sbi-fwft.c | 162 ++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 172 insertions(+)
>
> diff --git a/lib/riscv/asm/mmu.h b/lib/riscv/asm/mmu.h
> index 28c332f11496..4cdc1f77d884 100644
> --- a/lib/riscv/asm/mmu.h
> +++ b/lib/riscv/asm/mmu.h
> @@ -32,4 +32,7 @@ static inline void local_flush_tlb_page(unsigned long addr)
> */
> pte_t *get_pte(pgd_t *pgtable, uintptr_t vaddr);
>
> +/* Update the pgprot to @prot for the pte of the corresponding @addr */
> +void pte_update_prot(unsigned long addr, pteval_t prot);
> +
> #endif /* _ASMRISCV_MMU_H_ */
> diff --git a/lib/riscv/mmu.c b/lib/riscv/mmu.c
> index 577c66aa77ba..4438327f6ffe 100644
> --- a/lib/riscv/mmu.c
> +++ b/lib/riscv/mmu.c
> @@ -54,6 +54,13 @@ pte_t *get_pte(pgd_t *pgtable, uintptr_t vaddr)
> return ptep;
> }
>
> +void pte_update_prot(unsigned long addr, pteval_t prot)
> +{
> + pte_t *ptep = get_pte(current_pgtable(), addr);
> + *ptep = __pte(pte_val(*ptep) | prot);
> + local_flush_tlb_page(addr);
> +}
> +
> static pteval_t *__install_page(pgd_t *pgtable, phys_addr_t paddr,
> uintptr_t vaddr, pgprot_t prot, bool flush)
> {
> diff --git a/riscv/sbi-fwft.c b/riscv/sbi-fwft.c
> index 0a70192b1da5..2b65285c82fc 100644
> --- a/riscv/sbi-fwft.c
> +++ b/riscv/sbi-fwft.c
> @@ -5,9 +5,12 @@
> * Copyright (C) 2024, Rivos Inc., Clément Léger <cleger at rivosinc.com>
> */
> #include <libcflat.h>
> +#include <alloc.h>
> #include <stdlib.h>
>
> #include <asm/csr.h>
> +#include <asm/io.h>
> +#include <asm/mmu.h>
> #include <asm/processor.h>
> #include <asm/ptrace.h>
> #include <asm/sbi.h>
> @@ -160,6 +163,164 @@ static void fwft_check_misaligned_exc_deleg(void)
> report_prefix_pop();
> }
>
> +static bool adue_triggered_read, adue_triggered_write;
> +
> +static void adue_read_handler(struct pt_regs *regs)
> +{
> + adue_triggered_read = true;
> + pte_update_prot(regs->badaddr, _PAGE_ACCESSED);
> +}
> +
> +static void adue_write_handler(struct pt_regs *regs)
> +{
> + adue_triggered_write = true;
> + pte_update_prot(regs->badaddr, _PAGE_ACCESSED | _PAGE_DIRTY);
> +}
> +
> +static bool adue_check_pte(pteval_t pte, bool write)
> +{
> + return (pte & (_PAGE_ACCESSED | _PAGE_DIRTY)) == (_PAGE_ACCESSED | (write ? _PAGE_DIRTY : 0));
> +}
> +
> +static void __adue_check(bool hw_updating_enabled, bool write)
> +{
> + unsigned long *ptr = malloc(sizeof(unsigned long));
> + pte_t *ptep = get_pte(current_pgtable(), (uintptr_t)ptr);
> + bool *triggered;
> + const char *op;
> +
> + WRITE_ONCE(adue_triggered_read, false);
> + WRITE_ONCE(adue_triggered_write, false);
> +
> + *ptep = __pte(pte_val(*ptep) & ~(_PAGE_ACCESSED | _PAGE_DIRTY));
> + local_flush_tlb_page((uintptr_t)ptr);
> +
> + if (write) {
> + op = "write";
> + triggered = &adue_triggered_write;
> + writel(0xdeadbeef, ptr);
> + } else {
> + op = "read";
> + triggered = &adue_triggered_read;
> + readl(ptr);
> + }
> +
> + ptep = get_pte(current_pgtable(), (uintptr_t)ptr);
> +
> + report(hw_updating_enabled != *triggered &&
> + adue_check_pte(pte_val(*ptep), write), "%s %s",
> + hw_updating_enabled ? "hw updating enabled" : "hw updating disabled", op);
Hi Andrew,
adue_check_pte(pte_val(*ptep), write), "hw updating %s %s",
hw_updating_enabled ? "enabled" : "disabled", op);
> +
> + free(ptr);
> +}
> +
> +static void adue_check(bool hw_updating_enabled)
> +{
> + __adue_check(hw_updating_enabled, false);
> + __adue_check(hw_updating_enabled, true);
> +}
> +
> +static void fwft_check_pte_ad_hw_updating(void)
> +{
> + struct sbiret ret;
> + bool enabled;
> +
> + report_prefix_push("pte_ad_hw_updating");
> +
> + ret = fwft_get(SBI_FWFT_PTE_AD_HW_UPDATING);
> + if (ret.error == SBI_ERR_NOT_SUPPORTED) {
> + report_skip("not supported by platform");
> + return;
> + } else if (!sbiret_report_error(&ret, SBI_SUCCESS, "get")) {
> + /* Not much we can do without a working get... */
> + return;
> + }
> +
> + enabled = !!ret.value;
Is there a reason to normalize value to a boolean ? The spec states that
the values for ADUE is either exactly 0 or 1. So i'd expect value to
contain 0 or 1 and thus no need to normalize it. That would even hide
some invalid returned value from the SBI.
> + report(!enabled, "resets to 0");
> +
> + install_exception_handler(EXC_LOAD_PAGE_FAULT, adue_read_handler);
> + install_exception_handler(EXC_STORE_PAGE_FAULT, adue_write_handler);
> +
> + adue_check(enabled);
> +
> + ret = fwft_set(SBI_FWFT_PTE_AD_HW_UPDATING, !enabled, 0);
> + if (!sbiret_report_error(&ret, SBI_SUCCESS, "set to %d", !enabled))
> + goto inval_tests;
> + else
> + enabled = !enabled;
> +
> + ret = fwft_get(SBI_FWFT_PTE_AD_HW_UPDATING);
> + sbiret_report(&ret, SBI_SUCCESS, enabled, "get %d", enabled);
> +
> + adue_check(enabled);
> +
> + ret = fwft_set(SBI_FWFT_PTE_AD_HW_UPDATING, !enabled, 0);
> + if (!sbiret_report_error(&ret, SBI_SUCCESS, "set to %d again", !enabled))
> + goto inval_tests;
> + else
> + enabled = !enabled;
> +
> + ret = fwft_get(SBI_FWFT_PTE_AD_HW_UPDATING);
> + sbiret_report(&ret, SBI_SUCCESS, enabled, "get %d again", enabled);
> +
> + adue_check(enabled);
That seems like this whole block was copy/pasted, it might be factorized.
> +
> +#if __riscv_xlen > 32
> + ret = fwft_set_raw(BIT(32) | SBI_FWFT_PTE_AD_HW_UPDATING, !enabled, 0);
> + if (!sbiret_report_error(&ret, SBI_SUCCESS, "set to %d with high feature bits set", !enabled))
> + goto inval_tests;
> + else
> + enabled = !enabled;
> +
> + ret = fwft_get(SBI_FWFT_PTE_AD_HW_UPDATING);
> + sbiret_report(&ret, SBI_SUCCESS, enabled, "get %d after set with high feature bits set", enabled);
> +
> + adue_check(enabled);
> +#endif
> +
> + ret = fwft_set(SBI_FWFT_PTE_AD_HW_UPDATING, !enabled, 1);
> + if (!sbiret_report_error(&ret, SBI_SUCCESS, "set to %d with lock", !enabled))
> + goto inval_tests;
> + else
> + enabled = !enabled;
> +
> + ret = fwft_get(SBI_FWFT_PTE_AD_HW_UPDATING);
> + sbiret_report(&ret, SBI_SUCCESS, enabled, "get %d after set with lock", enabled);
> +
> + adue_check(enabled);
> +
> + ret = fwft_set(SBI_FWFT_PTE_AD_HW_UPDATING, !enabled, 0);
> + sbiret_report_error(&ret, SBI_ERR_DENIED_LOCKED, "set locked to %d without lock", !enabled);
> +
> + ret = fwft_set(SBI_FWFT_PTE_AD_HW_UPDATING, !enabled, 1);
> + sbiret_report_error(&ret, SBI_ERR_DENIED_LOCKED, "set locked to %d with lock", !enabled);
> +
> + ret = fwft_set(SBI_FWFT_PTE_AD_HW_UPDATING, enabled, 0);
> + sbiret_report_error(&ret, SBI_ERR_DENIED_LOCKED, "set locked to %d without lock", enabled);
> +
> + ret = fwft_get(SBI_FWFT_PTE_AD_HW_UPDATING);
> + sbiret_report(&ret, SBI_SUCCESS, enabled, "get locked %d after same set without lock", enabled);
> +
> + ret = fwft_set(SBI_FWFT_PTE_AD_HW_UPDATING, enabled, 1);
> + sbiret_report_error(&ret, SBI_ERR_DENIED_LOCKED, "set locked to %d with lock", enabled);
> +
> + ret = fwft_get(SBI_FWFT_PTE_AD_HW_UPDATING);
> + sbiret_report(&ret, SBI_SUCCESS, enabled, "get locked %d after same set with lock", enabled);
Should this be factorized with the misaligned LOCKED testing as well ?
> +
> +inval_tests:
> + ret = fwft_set(SBI_FWFT_PTE_AD_HW_UPDATING, 2, 0);
> + sbiret_report_error(&ret, SBI_ERR_INVALID_PARAM, "set to 2");
> +
> + ret = fwft_set(SBI_FWFT_PTE_AD_HW_UPDATING, !enabled, 2);
> + sbiret_report_error(&ret, SBI_ERR_INVALID_PARAM, "set to %d with flags=2", !enabled);
Ditto.
Otherwise, looks good to me.
Thanks,
Clément
> +
> + install_exception_handler(EXC_LOAD_PAGE_FAULT, NULL);
> + install_exception_handler(EXC_STORE_PAGE_FAULT, NULL);
> +
> + report_prefix_pop();
> +}
> +
> void check_fwft(void)
> {
> report_prefix_push("fwft");
> @@ -174,6 +335,7 @@ void check_fwft(void)
>
> fwft_check_base();
> fwft_check_misaligned_exc_deleg();
> + fwft_check_pte_ad_hw_updating();
>
> report_prefix_pop();
> }
More information about the kvm-riscv
mailing list