Re: [PATCH 06/18] dbtr: Improve trigger update error checking

[Nicholas Piggin <npiggin@gmail.com>]

On Mon, May 04, 2026 at 08:53:18PM +0530, Himanshu Chauhan wrote:
> On Fri, Mar 13, 2026 at 03:19:35PM +1000, Nicholas Piggin wrote:
> > Trigger updates should ensure all triggers can be upated without failure
> > before making any changes. Updates that change the trigger type must
> > also be disallowed according to SBI specification.
> > 
> > Change the style of shmem access and checking to match the trigger
> > install code and perform all checks first. Add the missing check to
> > prevent type change.
> > 
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> >  lib/sbi/sbi_dbtr.c | 33 ++++++++++++++++++++++++++-------
> >  1 file changed, 26 insertions(+), 7 deletions(-)
> > 
> > diff --git a/lib/sbi/sbi_dbtr.c b/lib/sbi/sbi_dbtr.c
> > index 224f2350..d2845fec 100644
> > --- a/lib/sbi/sbi_dbtr.c
> > +++ b/lib/sbi/sbi_dbtr.c
> > @@ -727,9 +727,9 @@ int sbi_dbtr_enable_trig(unsigned long trig_idx_base,
> >  int sbi_dbtr_update_trig(unsigned long smode,
> >  			 unsigned long trig_count, unsigned long *out)
> >  {
> > -	unsigned long trig_idx;
> >  	struct sbi_dbtr_trigger *trig;
> >  	union sbi_dbtr_shmem_entry *entry;
> > +	struct sbi_dbtr_data_msg *recv;
> >  	void *shmem_base = NULL;
> >  	struct sbi_dbtr_hart_triggers_state *hs = NULL;
> >  
> > @@ -744,30 +744,49 @@ int sbi_dbtr_update_trig(unsigned long smode,
> >  		return SBI_ERR_NO_SHMEM;
> >  
> >  	shmem_base = hart_shmem_base(hs);
> > +	sbi_hart_protection_map_range((unsigned long)shmem_base,
> > +				      trig_count * sizeof(*entry));
> Hi Nicholas,
> 
> I would suggest to verify the value of trig_count before making a large map entry.
> It would also help in the two loops below.
> 
> >  
> > +	/* Check requested triggers configuration */
> >  	for_each_trig_entry(shmem_base, trig_count, typeof(*entry), entry) {
> > -		sbi_hart_protection_map_range((unsigned long)entry, sizeof(*entry));
> > -		trig_idx = entry->id.idx;
> > +		unsigned long trig_idx, tdata1;
> >  
> > +		trig_idx = entry->id.idx;
> >  		if (trig_idx >= hs->total_trigs) {
> > -			sbi_hart_protection_unmap_range((unsigned long)entry, sizeof(*entry));
> >  			*out = _idx;
> > +			sbi_hart_protection_unmap_range((unsigned long)shmem_base,
> > +							trig_count * sizeof(*entry));
> 
> A forward goto to bailout in success/error condition would be better.

Hey Himanshu,

Thanks for the reviews, I'll take a look at these suggestions and see
how they look.

I should repost this series soon, I will do so after hearing from David
about the tdata2/tdata3 issue.

Thanks,
Nick

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