Re: [PATCH v2 10/14] powerpc/pseries/iommu: Reorganize iommu_table_setparms*() with new helper

[Leonardo Bras <leobras.c@gmail.com>]

On Tue, 2020-09-29 at 13:56 +1000, Alexey Kardashevskiy wrote:
> 
> On 12/09/2020 03:07, Leonardo Bras wrote:
> > Cc: linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org,
> > 
> > Add a new helper _iommu_table_setparms(), and use it in
> > iommu_table_setparms() and iommu_table_setparms_lpar() to avoid duplicated
> > code.
> > 
> > Also, setting tbl->it_ops was happening outsite iommu_table_setparms*(),
> > so move it to the new helper. Since we need the iommu_table_ops to be
> > declared before used, move iommu_table_lpar_multi_ops and
> > iommu_table_pseries_ops to before their respective iommu_table_setparms*().
> > 
> > The tce_exchange_pseries() also had to be moved up, since it's used in
> > iommu_table_lpar_multi_ops.xchg_no_kill.
> 
> 
> Use forward declarations (preferred) or make a separate patch for moving 
> chunks (I do not see much point).

Fixed :)

> > @@ -509,8 +559,13 @@ static void iommu_table_setparms(struct pci_controller *phb,
> >   	const unsigned long *basep;
> >   	const u32 *sizep;

> > -	node = phb->dn;
> > +	/* Test if we are going over 2GB of DMA space */
> > 
> > 
> > 
> > +	if (phb->dma_window_base_cur + phb->dma_window_size > 0x80000000ul) {
> > +		udbg_printf("PCI_DMA: Unexpected number of IOAs under this PHB.\n");
> > +		panic("PCI_DMA: Unexpected number of IOAs under this PHB.\n");
> > +	}
> 
> 
> s/0x80000000ul/2*SZ_1G/

Done!

> 
> but more to the point - why this check? QEMU can create windows at 0 and 
> as big as the VM requested. And I am pretty sure I can construct QEMU 
> command line such as it won't have MMIO32 at all and a 4GB default DMA 
> window.
> 

Oh, the diff was a little strange here. I did not add this snippet, it
was already in that function, but since I created the helper, the diff
made it look like I introduced this piece of code.
Please take a look in the diff snippet bellow. (This same lines were
there.)

> > @@ -519,33 +574,25 @@ static void iommu_table_setparms(struct pci_controller *phb,
> >   		return;
> >   	}
> >   
> > -	tbl->it_base = (unsigned long)__va(*basep);
> > 
> > 
> > 
> > +	_iommu_table_setparms(tbl, phb->bus->number, 0, phb->dma_window_base_cur,
> > +			      phb->dma_window_size, IOMMU_PAGE_SHIFT_4K,
> > +			      (unsigned long)__va(*basep), &iommu_table_pseries_ops);
> >     	if (!is_kdump_kernel())
> > 
> > 
> > 
> >   		memset((void *)tbl->it_base, 0, *sizep);
> > 
> > -	tbl->it_busno = phb->bus->number;
> > -	tbl->it_page_shift = IOMMU_PAGE_SHIFT_4K;
> > -
> > -	/* Units of tce entries */
> > -	tbl->it_offset = phb->dma_window_base_cur >> tbl->it_page_shift;
> > -
> > -	/* Test if we are going over 2GB of DMA space */
> > -	if (phb->dma_window_base_cur + phb->dma_window_size > 0x80000000ul) {
> > -		udbg_printf("PCI_DMA: Unexpected number of IOAs under this PHB.\n");
> > -		panic("PCI_DMA: Unexpected number of IOAs under this PHB.\n");
> > -	}
> > -
> >   	phb->dma_window_base_cur += phb->dma_window_size;
> > -
> > -	/* Set the tce table size - measured in entries */
> > -	tbl->it_size = phb->dma_window_size >> tbl->it_page_shift;
> > -
> > -	tbl->it_index = 0;
> > -	tbl->it_blocksize = 16;
> > -	tbl->it_type = TCE_PCI;
> >   }
> >   

Thanks for reviewing, Alexey!