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 <gba...@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 <gba...@linux.vnet.ibm.com>
Reviewed-by: Brian King <brk...@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