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

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

[Gaurav Batra]

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.

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;
 
 	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);
+
+		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");
-- 

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

[Michael Ellerman]

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

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

[Gaurav Batra]

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

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

[Michael Ellerman]

Gaurav Batra <gbatra@linux.vnet.ibm.com> writes:
> 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.

OK. It's always good to spell out in the change log whether it's a
theoretical/unlikely bug, or one that's actually been hit in testing or
the field.

> 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?

If it really goes back to the origin commit, then it's probably better
to just say so and tag it for stable, rather than pointing to 1da177e4.

I wonder though is there something else that changed that means this bug
is now being hit but wasn't before? Or maybe it's just that we are
testing on systems with large enough amounts of memory to hit this but
which aren't using a direct mapping?

cheers

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

[Gaurav Batra]

[-- Attachment #1: Type: text/plain, Size: 1946 bytes --]


On 5/17/23 7:19 AM, Michael Ellerman wrote:
> Gaurav Batra<gbatra@linux.vnet.ibm.com>  writes:
>> 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.
> OK. It's always good to spell out in the change log whether it's a
> theoretical/unlikely bug, or one that's actually been hit in testing or
> the field.
I will submit another version of the patch with some changes in the log 
once I figure out how to Tag it for stable (as mentioned below).
>> 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?
> If it really goes back to the origin commit, then it's probably better
> to just say so and tag it for stable, rather than pointing to 1da177e4.
How to do I tag it for stable? Will it be part of the "Fixes:" tag or 
some other tag?
>
> I wonder though is there something else that changed that means this bug
> is now being hit but wasn't before? Or maybe it's just that we are
> testing on systems with large enough amounts of memory to hit this but
> which aren't using a direct mapping?

 From the details in Bugzilla, it does seems like the HCALL was 
previously taking long as well but PHYP was more relaxed about it. Now, 
PHYP is limiting on how long can an HCALL take.

Below are some excerpts from the Bug: 202349

Linux is passing too many counts in H_STUFF_TCE. The higher the counts, 
the longer the HCALL takes. From a Hypervisor perspective, we cannot 
stop Linux from doing this or it will violate the rules in the PAPR 
(which then would cause Linux to crash). The dispatcher team has 
"tightened the screws" on long running HCALLs by causing this trap to 
fire. From our discussions, they will not put the limits back where they 
were before.


Thanks

Gaurav

>
> cheers

[-- Attachment #2: Type: text/html, Size: 3876 bytes --]

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

[Michael Ellerman]

Gaurav Batra <gbatra@linux.vnet.ibm.com> writes:
> On 5/17/23 7:19 AM, Michael Ellerman wrote:
>> Gaurav Batra<gbatra@linux.vnet.ibm.com>  writes:
>>> 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.
>> OK. It's always good to spell out in the change log whether it's a
>> theoretical/unlikely bug, or one that's actually been hit in testing or
>> the field.

> I will submit another version of the patch with some changes in the log 
> once I figure out how to Tag it for stable (as mentioned below).
> 
>>> 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?
>> If it really goes back to the origin commit, then it's probably better
>> to just say so and tag it for stable, rather than pointing to 1da177e4.

> How to do I tag it for stable? Will it be part of the "Fixes:" tag or 
> some other tag?

A stable tag is just adding in the change log:

Cc: stable@vger.kernel.org

Note *not* in the email headers, in the change log.

That asks the stable kernel folks to backport the commit to all
currently active stable trees. Whereas when you use a Fixes: tag it will
be only backported to stable trees that contain the offending commit.

>> I wonder though is there something else that changed that means this bug
>> is now being hit but wasn't before? Or maybe it's just that we are
>> testing on systems with large enough amounts of memory to hit this but
>> which aren't using a direct mapping?
>
>  From the details in Bugzilla, it does seems like the HCALL was 
> previously taking long as well but PHYP was more relaxed about it. Now, 
> PHYP is limiting on how long can an HCALL take.
>
> Below are some excerpts from the Bug: 202349
>
> Linux is passing too many counts in H_STUFF_TCE. The higher the counts, 
> the longer the HCALL takes. From a Hypervisor perspective, we cannot 
> stop Linux from doing this or it will violate the rules in the PAPR 
> (which then would cause Linux to crash). The dispatcher team has 
> "tightened the screws" on long running HCALLs by causing this trap to 
> fire. From our discussions, they will not put the limits back where they 
> were before.

OK, that explains why it's only been noticed recently, so that's good
info. In the change log you can say something like "newer firmware has
started enforcing the limits".

cheers