Hi Felix,
On 2026-10-30T14:38:40+11:00, Felix Moessbauer
<[email protected]> wrote:
> efi_selftest: add test for EFI_TIMESTAMP_PROTOCOL
>
> Make sure the protocol is usable: the counter must advance at roughly
> the reported frequency, otherwise applications get wrong time
> measurements.
I don't think the test can actually check this. See below.
> diff --git a/lib/efi_selftest/efi_selftest_timestamp.c
> b/lib/efi_selftest/efi_selftest_timestamp.c
> @@ -0,0 +1,99 @@
> + t0 = ts->get_timestamp();
> + ret = boottime->stall(10000);
> + if (ret != EFI_SUCCESS) {
> + efi_st_error("Stall failed\n");
> + return EFI_ST_FAILURE;
> + }
> + t1 = ts->get_timestamp();
> +
> + if (t1 <= t0) {
> + efi_st_error("Timestamp did not advance\n");
> + return EFI_ST_FAILURE;
> + }
> + /* 10 ms correspond to frequency / 100 ticks, allow 50 % slack */
> + expected = props.frequency / 100;
> + if (t1 - t0 < expected / 2) {
> + efi_st_error("Timestamp advanced too slowly\n");
> + return EFI_ST_FAILURE;
> + }
This check is circular. efi_stall() sets 'end_tick = get_ticks() +
usec_to_tick(microseconds)' and spins until get_ticks() reaches it,
and usec_to_tick() uses get_tbclk(). So when stall() returns, the
counter has already advanced by at least frequency / 100 ticks,
whatever the real frequency is. The check can only fail if
get_timestamp() stops using get_ticks(); a wrong get_tbclk() value
(the case the commit message describes) still passes. The 50% slack
suggests tolerance for a real-time measurement, but there isn't one
here.
Checking the frequency needs an independent time source, e.g. the
runtime get_time() service when CONFIG_EFI_GET_TIME is enabled,
measuring over a second or two, though that makes the test slower.
Otherwise, please can you reword the commit message and the comment to
claim only that the counter is monotonic and consistent with Stall(),
and drop the slack, since stall() already guarantees 't1 - t0 >=
expected'. What do you think?
There is also no upper bound, so a reported frequency that is far too
low would not be caught either way.
>
> Signed-off-by: Felix Moessbauer <[email protected]>
>
> lib/efi_selftest/Makefile | 1 +
> lib/efi_selftest/efi_selftest_timestamp.c | 99
> +++++++++++++++++++++++++++++++
> 2 files changed, 100 insertions(+)
Regards,
Simon
