Re: [kvm-unit-tests PATCH 11/10] riscv: sbi: Add fwft pte_hw_ad_updating test

From: Andrew Jones <andrew.jones@linux.dev>
To: "Clément Léger" <cleger@rivosinc.com>
Cc: kvm-riscv@lists.infradead.org, atishp@rivosinc.com,
	 jamestiotio@gmail.com
Subject: Re: [kvm-unit-tests PATCH 11/10] riscv: sbi: Add fwft pte_hw_ad_updating test
Date: Thu, 27 Feb 2025 13:07:18 +0100	[thread overview]
Message-ID: <20250227-aa83c6934b574c860989b31b@orel> (raw)
In-Reply-To: <8f02937b-4a01-4c69-91a2-59623ef81246@rivosinc.com>

On Thu, Feb 27, 2025 at 10:09:42AM +0100, Clément Léger wrote:
...
> > +	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);

Will do

> 
> > +
> > +	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.

Good thought. We should test the exact 0/1.

> 
> > +	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 for the review. I'll refactor as suggested. I've also got a couple
other changes to make, such as marking the reset value test as kfail,
since opensbi currently enables svadu when it's present.

I'll send a v2 of this series with patch 1/10 dropped and this patch
added after fixing it up.

Thanks,
drew

-- 
kvm-riscv mailing list
kvm-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kvm-riscv