Re: [PATCH 01/18] dbtr: Add consistent range checks to trigger ecalls

[Nicholas Piggin <npiggin@gmail.com>]

On Fri, Mar 20, 2026 at 03:46:46PM +0300, Ilya Mamay wrote:
> Hi, Nicholas
> This is a significant improvement over the previous version!
> 
> I have a few comments below, though they may be off the mark since they're
> based on my own reading of the SBI and Sdtrig specifications.

Hey Ilya, thanks for the review, sorry to take a while to get back to it
I was on vacation for a few weeks.

> On Fri, Mar 13, 2026 at 03:19:30PM +1000, Nicholas Piggin wrote:
> > @@ -594,6 +597,9 @@ int sbi_dbtr_install_trig(unsigned long smode,
> >  	if (!hs)
> >  		return SBI_ERR_FAILED;
> >  
> > +	if (trig_count >= hs->total_trigs)
> > +		return SBI_ERR_BAD_RANGE;
> 
> According to the SBI specification (section 19.4):
> - "The sbiret.value is set to zero upon success or if shared memory is
>   disabled". I interpret this to mean that sbi_dbtr_install_trig must return a
>   trigger index along with an SBI_ERR_BAD_RANGE error.
> 
> - "sbiret.value is set to the array index i of the failing
>   trigger configuration". In this case, sbiret.value should be hs->total_trigs,
>   because that is the index of the first failing trigger configuration.
> 
> Same comment applies to sbi_dbtr_update_trig.

Shapr observation. I think it may be technically ambiguous, but I felt
the intention was the other way. That is, bad range is not any
particular trigger failing, but an overall error before we get to
individual trigger processing.

sbi_debug_read_triggers() can return SBI_ERR_BAD_RANGE too for example,
and that could not point to a bad trigger.

I would prefer to leave as is unless there is strong opinion or
requirement for now, since that is leaving existing behaviour unchanged.

Thanks,
Nick

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