Re: [PATCH] lib: sbi: dbtr: do not unconditionally access tdata2/tdata3 CSRs

[Nicholas Piggin <npiggin@gmail.com>]

On Mon, May 25, 2026 at 12:50:33PM -0600, David E. Garcia Porras wrote:
> The current SBI DBTR extension implementation accesses tdata2 and tdata3
> without first checking whether either register is implemented on the underlying hart.
> This produces an illegal instruction exception on otherwise
> spec-compliant cores that legitimately omit one or both registers.

Hey,

I attempted to fix this to work around a crash I saw with QEMU.

https://lists.infradead.org/pipermail/opensbi/2026-March/009600.html

I didn't actually dig far enough into the spec or see the CSR write
side of the issue because it was following the recipe from the spec, and
QEMU doesn't crash in that case.

AFAIKS you're right though, tdata2/3 must always be readable if they
are implemented for any trigger. so QEMU's implementation seems contrary
to the Section 5 introduction (I have a QEMU Sdtrig fix series, I'll add
a fix to it).

Unfortunately your fix here I think won't help the existing QEMU bug
because trap depends on the selected trigger type (see
tdata_available()). Arguably we could just ignore QEMU... but using
csr_read_allowed/csr_write_allowed would solve both cases. What do you
think?

[snip]

> @@ -619,6 +647,20 @@ int sbi_dbtr_install_trig(unsigned long smode,
>  							trig_count * sizeof(*entry));
>  			return SBI_ERR_FAILED;
>  		}
> +
> +		if (recv->tdata2 && !hs->tdata2_supported) {
> +			*out = _idx;
> +			sbi_hart_protection_unmap_range((unsigned long)shmem_base,
> +							trig_count * sizeof(*entry));
> +			return SBI_ERR_NOT_SUPPORTED;
> +		}
> +
> +		if (recv->tdata3 && !hs->tdata3_supported) {
> +			*out = _idx;
> +			sbi_hart_protection_unmap_range((unsigned long)shmem_base,
> +							trig_count * sizeof(*entry));
> +			return SBI_ERR_NOT_SUPPORTED;
> +		}
>  	}

These checks seem consistent with the SBI spec, but even with them we
miss classes of these errors AFAIKS. To be comprehensive we might need
to program the trigger then read it back and compare. Which might
require some complicated unwinding.

In any case I don't dislike adding the checks, but perhaps they can be
addressed separately and we could decide whether to cover other cases
too.

Thanks,
Nick

> 
>  	if (hs->available_trigs < trig_count) {
> @@ -734,6 +776,16 @@ int sbi_dbtr_update_trig(unsigned long smode,
>  			return SBI_ERR_FAILED;
>  		}
> 
> +		if (entry->data.tdata2 && !hs->tdata2_supported) {
> +			sbi_hart_protection_unmap_range((unsigned long)entry, sizeof(*entry));
> +			return SBI_ERR_NOT_SUPPORTED;
> +		}
> +
> +		if (entry->data.tdata3 && !hs->tdata3_supported) {
> +			sbi_hart_protection_unmap_range((unsigned long)entry, sizeof(*entry));
> +			return SBI_ERR_NOT_SUPPORTED;
> +		}
> +
>  		dbtr_trigger_setup(trig, &entry->data);
>  		sbi_hart_protection_unmap_range((unsigned long)entry, sizeof(*entry));
>  		dbtr_trigger_enable(trig);
> --

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