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

Nicholas Piggin npiggin at gmail.com
Thu Apr 23 18:37:01 PDT 2026


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



More information about the opensbi mailing list