On Tue, Mar 18, 2025 at 11:13:49PM +0530, Akshay Behl wrote:
> This patch adds a generic function for lock tests for all
> the sbi fwft features. It expects the feature is already
> locked before being called and tests the locked feature.
>
> Signed-off-by: Akshay Behl <akshaybehl231@xxxxxxxxx>
> ---
> v2:
> - Made changes to handel non boolean feature tests.
> riscv/sbi-fwft.c | 49 ++++++++++++++++++++++++++++++++----------------
> 1 file changed, 33 insertions(+), 16 deletions(-)
>
> diff --git a/riscv/sbi-fwft.c b/riscv/sbi-fwft.c
> index 581cbf6b..7d9735d7 100644
> --- a/riscv/sbi-fwft.c
> +++ b/riscv/sbi-fwft.c
> @@ -74,6 +74,34 @@ static void fwft_check_reset(uint32_t feature, unsigned long reset)
> sbiret_report(&ret, SBI_SUCCESS, reset, "resets to %lu", reset);
> }
>
> +/* Must be called after locking the feature using SBI_FWFT_SET_FLAG_LOCK */
> +static void fwft_feature_lock_test_values(uint32_t feature, size_t nr_values,
> + unsigned long test_values[], unsigned long locked_value)
What happened to the indentation here? Please proofread your patches.
> +{
> + struct sbiret ret;
We could push a "locked" prefix here and then...
> +
> + for (int i = 0; i < nr_values; ++i) {
> + ret = fwft_set(feature, test_values[i], 0);
> + sbiret_report_error(&ret, SBI_ERR_DENIED_LOCKED,
> + "Set locked feature to %lu without lock", test_values[i]);
^ flag
> +
> + ret = fwft_set(feature, test_values[i], SBI_FWFT_SET_FLAG_LOCK);
> + sbiret_report_error(&ret, SBI_ERR_DENIED_LOCKED,
> + "Set locked feature to %lu with lock", test_values[i]);
^ flag
> + }
> +
> + ret = fwft_get(feature);
> + sbiret_report(&ret, SBI_SUCCESS, locked_value,
> + "Get locked feature value %lu", locked_value);
...drop 'locked feature' from all the reports above.
Then pop the prefix here.
> +}
> +
> +static void fwft_feature_lock_test(uint32_t feature, unsigned long locked_value)
> +{
> + unsigned long values[] = {0, 1};
> +
> + fwft_feature_lock_test_values(feature, 2 , values, locked_value);
^ extra space
> +}
> +
> static void fwft_check_base(void)
> {
> report_prefix_push("base");
> @@ -181,11 +209,9 @@ static void fwft_check_misaligned_exc_deleg(void)
> /* Lock the feature */
> ret = fwft_misaligned_exc_set(0, SBI_FWFT_SET_FLAG_LOCK);
> sbiret_report_error(&ret, SBI_SUCCESS, "Set misaligned deleg feature value 0 and lock");
> - ret = fwft_misaligned_exc_set(1, 0);
> - sbiret_report_error(&ret, SBI_ERR_DENIED_LOCKED,
> - "Set locked misaligned deleg feature to new value");
> - ret = fwft_misaligned_exc_get();
> - sbiret_report(&ret, SBI_SUCCESS, 0, "Get misaligned deleg locked value 0");
> +
> + /* Test feature lock */
Can drop this comment since the function name says the same thing.
> + fwft_feature_lock_test(SBI_FWFT_MISALIGNED_EXC_DELEG, 0);
>
> report_prefix_pop();
> }
> @@ -326,17 +352,8 @@ adue_inval_tests:
> else
> enabled = !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_set(SBI_FWFT_PTE_AD_HW_UPDATING, enabled, 1);
> - sbiret_report_error(&ret, SBI_ERR_DENIED_LOCKED, "set locked to %d with lock", enabled);
> + /* Test the feature lock */
same comment
> + fwft_feature_lock_test(SBI_FWFT_PTE_AD_HW_UPDATING, enabled);
>
> adue_done:
> install_exception_handler(EXC_LOAD_PAGE_FAULT, NULL);
> --
> 2.34.1
>
Thanks,
drew
