Re: [PATCH] powerpc/iommu: limit number of TCEs to 512 for H_STUFF_TCE hcall

[Gaurav Batra <gbatra@linux.vnet.ibm.com>]

Hello Michael,

System test hit the crash. I believe, it was PHYP that resulted in it 
due to number of TCEs passed in to be >512.

I was wondering about the Fixes tag as well. But, this interface, in 
it's current form, is there from the day the file was created. So, in 
this case, should I mention the first commit which created this source file?

Thanks a lot for looking into it.

Gaurav

On 5/11/23 9:35 PM, Michael Ellerman wrote:
> Gaurav Batra <gbatra@linux.vnet.ibm.com> writes:
>> As of now, in tce_freemulti_pSeriesLP(), there is no limit on how many TCEs
>> are passed to H_STUFF_TCE hcall. PAPR is enforcing this to be limited to
>> 512 TCEs.
> Did you actually hit a bug here, or just noticed via code inspection?
>
> Can you provide a Fixes: tag ?
>
> cheers
>
>> Signed-off-by: Gaurav Batra <gbatra@linux.vnet.ibm.com>
>> Reviewed-by: Brian King <brking@linux.vnet.ibm.com>
>> ---
>>   arch/powerpc/platforms/pseries/iommu.c | 12 ++++++++++--
>>   1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
>> index c74b71d4733d..1b134b1b795a 100644
>> --- a/arch/powerpc/platforms/pseries/iommu.c
>> +++ b/arch/powerpc/platforms/pseries/iommu.c
>> @@ -306,13 +306,21 @@ static void tce_free_pSeriesLP(unsigned long liobn, long tcenum, long tceshift,
>>   static void tce_freemulti_pSeriesLP(struct iommu_table *tbl, long tcenum, long npages)
>>   {
>>   	u64 rc;
>> +	long limit, rpages = npages;
>    
> I don't know why npages is signed, but we don't ever want limit to be
> negative, so it'd be better of as unsigned long wouldn't it?
>
>>   	if (!firmware_has_feature(FW_FEATURE_STUFF_TCE))
>>   		return tce_free_pSeriesLP(tbl->it_index, tcenum,
>>   					  tbl->it_page_shift, npages);
>>   
>> -	rc = plpar_tce_stuff((u64)tbl->it_index,
>> -			     (u64)tcenum << tbl->it_page_shift, 0, npages);
>> +	do {
>> +		limit = min_t(long, rpages, 512);
> And here'd we'd use unsigned long.
>
>> +		rc = plpar_tce_stuff((u64)tbl->it_index,
>> +			     	(u64)tcenum << tbl->it_page_shift, 0, limit);
>> +
>> +		rpages -= limit;
>> +		tcenum += limit;
>> +	} while (rpages > 0 && !rc);
>>   
>>   	if (rc && printk_ratelimit()) {
>>   		printk("tce_freemulti_pSeriesLP: plpar_tce_stuff failed\n");
>> -- 
> cheers