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

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 <[email protected]>
> ---
>  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.

Thanks
Regards
Himanshu

>  			return SBI_ERR_INVALID_PARAM;
>  		}
>  
>  		trig = INDEX_TO_TRIGGER(trig_idx);
> -
>  		if (!(trig->state & RV_DBTR_BIT_MASK(TS, MAPPED))) {
> -			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));
>  			return SBI_ERR_FAILED;
>  		}
>  
> +		recv = (struct sbi_dbtr_data_msg *)(&entry->data);
> +		tdata1 = lle_to_cpu(recv->tdata1);
> +		if (TDATA1_GET_TYPE(tdata1) != TDATA1_GET_TYPE(trig->tdata1)) {
> +			*out = _idx;
> +			sbi_hart_protection_unmap_range((unsigned long)shmem_base,
> +							trig_count * sizeof(*entry));
> +			return SBI_ERR_INVALID_PARAM;
> +		}
> +	}
> +
> +	/* Update triggers */
> +	for_each_trig_entry(shmem_base, trig_count, typeof(*entry), entry) {
> +		trig = INDEX_TO_TRIGGER(entry->id.idx);
>  		dbtr_trigger_setup(trig, &entry->data);
> -		sbi_hart_protection_unmap_range((unsigned long)entry, sizeof(*entry));
>  		dbtr_trigger_enable(trig);
>  	}
>  
> +	sbi_hart_protection_unmap_range((unsigned long)shmem_base,
> +					trig_count * sizeof(*entry));
> +
>  	return SBI_SUCCESS;
>  }
>  
> -- 
> 2.51.0
> 
> 
> -- 
> opensbi mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/opensbi

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