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

Hi Nick,

Thanks for the review and for the QEMU context, I missed that work from your patch series.
If QEMU behaves as stated (tying trap directly to specific triggers), then yes my checks won't suffice.
I'll update my patch and submit a V2, lmk if we should maybe merge it into your patch series.

> 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?

Agreed. v2 will drop the boot-time probe and instead wrap every
tdata2/tdata3 read and write with csr_read_allowed/csr_write_allowed,
checking trap.cause locally, this should cover all cases.

For read path, I used your exact same code.

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

I'd like to keep the SBI_ERR_NOT_SUPPORTED checks in v2 to satisfy
the SBI spec wording in section 19.4 / 19.5:

  "One of the trigger configuration can't be programmed due to
   unimplemented optional bits in tdata1, tdata2, or tdata3 CSRs."

I'll implement them via csr_read_allowed so there's a single
consistent code path, and I'll add an explicit comment noting that
this only catches the "whole CSR unimplemented" case -- WARL
tied-off bits within a CSR are not caught and would require the
program-then-read-back approach you described. We can address that
as a separate follow-up patch.

v2 incoming.

Thanks,
David

-- 
opensbi mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/opensbi