| Message ID | 20210430163145.146984-8-leobras.c@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | DDW + Indirect Mapping | expand |
| Related | show |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch powerpc/merge (e3a9b9d6a03f5fbf99b540e863b001d46ba1735c) |
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch powerpc/next (5256426247837feb8703625bda7fcfc824af04cf) |
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch linus/master (8ca5297e7e38f2dc8c753d33a5092e7be181fff0) |
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch powerpc/fixes (791f9e36599d94af5a76d3f74d04e16326761aae) |
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch linux-next (d72cd4ad4174cfd2257c426ad51e4f53bcfde9c9) |
| snowpatch_ozlabs/apply_patch | fail | Failed to apply to any branch |
On 5/1/21 02:31, Leonardo Bras wrote: > 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*(). > > Signed-off-by: Leonardo Bras <leobras.c@gmail.com> This does not apply anymore as it conflicts with my 4be518d838809e2135. > --- > arch/powerpc/platforms/pseries/iommu.c | 100 ++++++++++++------------- > 1 file changed, 50 insertions(+), 50 deletions(-) > > diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c > index 5a70ecd579b8..89cb6e9e9f31 100644 > --- a/arch/powerpc/platforms/pseries/iommu.c > +++ b/arch/powerpc/platforms/pseries/iommu.c > @@ -53,6 +53,11 @@ enum { > DDW_EXT_QUERY_OUT_SIZE = 2 > }; > > +#ifdef CONFIG_IOMMU_API > +static int tce_exchange_pseries(struct iommu_table *tbl, long index, unsigned long *tce, > + enum dma_data_direction *direction, bool realmode); > +#endif Instead of declaring this so far from the the code which needs it, may be add struct iommu_table_ops iommu_table_lpar_multi_ops; right before iommu_table_setparms() (as the sctruct is what you actually want there), and you won't need to move iommu_table_pseries_ops as well. > + > static struct iommu_table *iommu_pseries_alloc_table(int node) > { > struct iommu_table *tbl; > @@ -501,6 +506,28 @@ static int tce_setrange_multi_pSeriesLP_walk(unsigned long start_pfn, > return tce_setrange_multi_pSeriesLP(start_pfn, num_pfn, arg); > } > > +static inline void _iommu_table_setparms(struct iommu_table *tbl, unsigned long busno, The underscore is confusing, may be iommu_table_do_setparms()? iommu_table_setparms_common()? Not sure. I cannot recall a single function with just one leading underscore, I suspect I was pushed back when I tried adding one ages ago :) "inline" seems excessive, the compiler will probably figure it out anyway. > + unsigned long liobn, unsigned long win_addr, > + unsigned long window_size, unsigned long page_shift, > + unsigned long base, struct iommu_table_ops *table_ops) Make "base" a pointer. Or, better, just keep setting it directly in iommu_table_setparms() rather than passing 0 around. The same comment about "liobn" - set it in iommu_table_setparms_lpar(). The reviewer will see what field atters in what situation imho. > +{ > + tbl->it_busno = busno; > + tbl->it_index = liobn; > + tbl->it_offset = win_addr >> page_shift; > + tbl->it_size = window_size >> page_shift; > + tbl->it_page_shift = page_shift; > + tbl->it_base = base; > + tbl->it_blocksize = 16; > + tbl->it_type = TCE_PCI; > + tbl->it_ops = table_ops; > +} > + > +struct iommu_table_ops iommu_table_pseries_ops = { > + .set = tce_build_pSeries, > + .clear = tce_free_pSeries, > + .get = tce_get_pseries > +}; > + > static void iommu_table_setparms(struct pci_controller *phb, > struct device_node *dn, > struct iommu_table *tbl) > @@ -509,8 +536,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 > SZ_2G) { > + udbg_printf("PCI_DMA: Unexpected number of IOAs under this PHB.\n"); > + panic("PCI_DMA: Unexpected number of IOAs under this PHB.\n"); > + } > > + node = phb->dn; > basep = of_get_property(node, "linux,tce-base", NULL); > sizep = of_get_property(node, "linux,tce-size", NULL); > if (basep == NULL || sizep == NULL) { > @@ -519,33 +551,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; > } > > +struct iommu_table_ops iommu_table_lpar_multi_ops = { > + .set = tce_buildmulti_pSeriesLP, > +#ifdef CONFIG_IOMMU_API > + .xchg_no_kill = tce_exchange_pseries, > +#endif > + .clear = tce_freemulti_pSeriesLP, > + .get = tce_get_pSeriesLP > +}; > + > /* > * iommu_table_setparms_lpar > * > @@ -557,28 +581,17 @@ static void iommu_table_setparms_lpar(struct pci_controller *phb, > struct iommu_table_group *table_group, > const __be32 *dma_window) > { > - unsigned long offset, size; > + unsigned long offset, size, liobn; > > - of_parse_dma_window(dn, dma_window, &tbl->it_index, &offset, &size); > + of_parse_dma_window(dn, dma_window, &liobn, &offset, &size); > > - tbl->it_busno = phb->bus->number; > - tbl->it_page_shift = IOMMU_PAGE_SHIFT_4K; > - tbl->it_base = 0; > - tbl->it_blocksize = 16; > - tbl->it_type = TCE_PCI; > - tbl->it_offset = offset >> tbl->it_page_shift; > - tbl->it_size = size >> tbl->it_page_shift; > + _iommu_table_setparms(tbl, phb->bus->number, liobn, offset, size, IOMMU_PAGE_SHIFT_4K, 0, > + &iommu_table_lpar_multi_ops); > > table_group->tce32_start = offset; > table_group->tce32_size = size; > } > > -struct iommu_table_ops iommu_table_pseries_ops = { > - .set = tce_build_pSeries, > - .clear = tce_free_pSeries, > - .get = tce_get_pseries > -}; > - > static void pci_dma_bus_setup_pSeries(struct pci_bus *bus) > { > struct device_node *dn; > @@ -647,7 +660,6 @@ static void pci_dma_bus_setup_pSeries(struct pci_bus *bus) > tbl = pci->table_group->tables[0]; > > iommu_table_setparms(pci->phb, dn, tbl); > - tbl->it_ops = &iommu_table_pseries_ops; > iommu_init_table(tbl, pci->phb->node, 0, 0); > > /* Divide the rest (1.75GB) among the children */ > @@ -664,7 +676,7 @@ static int tce_exchange_pseries(struct iommu_table *tbl, long index, unsigned > bool realmode) > { > long rc; > - unsigned long ioba = (unsigned long) index << tbl->it_page_shift; > + unsigned long ioba = (unsigned long)index << tbl->it_page_shift; Unrelated change, why, did checkpatch.pl complain? > unsigned long flags, oldtce = 0; > u64 proto_tce = iommu_direction_to_tce_perm(*direction); > unsigned long newtce = *tce | proto_tce; > @@ -686,15 +698,6 @@ static int tce_exchange_pseries(struct iommu_table *tbl, long index, unsigned > } > #endif > > -struct iommu_table_ops iommu_table_lpar_multi_ops = { > - .set = tce_buildmulti_pSeriesLP, > -#ifdef CONFIG_IOMMU_API > - .xchg_no_kill = tce_exchange_pseries, > -#endif > - .clear = tce_freemulti_pSeriesLP, > - .get = tce_get_pSeriesLP > -}; > - > static void pci_dma_bus_setup_pSeriesLP(struct pci_bus *bus) > { > struct iommu_table *tbl; > @@ -729,7 +732,6 @@ static void pci_dma_bus_setup_pSeriesLP(struct pci_bus *bus) > tbl = ppci->table_group->tables[0]; > iommu_table_setparms_lpar(ppci->phb, pdn, tbl, > ppci->table_group, dma_window); > - tbl->it_ops = &iommu_table_lpar_multi_ops; > iommu_init_table(tbl, ppci->phb->node, 0, 0); > iommu_register_group(ppci->table_group, > pci_domain_nr(bus), 0); > @@ -758,7 +760,6 @@ static void pci_dma_dev_setup_pSeries(struct pci_dev *dev) > PCI_DN(dn)->table_group = iommu_pseries_alloc_group(phb->node); > tbl = PCI_DN(dn)->table_group->tables[0]; > iommu_table_setparms(phb, dn, tbl); > - tbl->it_ops = &iommu_table_pseries_ops; > iommu_init_table(tbl, phb->node, 0, 0); > set_iommu_table_base(&dev->dev, tbl); > return; > @@ -1436,7 +1437,6 @@ static void pci_dma_dev_setup_pSeriesLP(struct pci_dev *dev) > tbl = pci->table_group->tables[0]; > iommu_table_setparms_lpar(pci->phb, pdn, tbl, > pci->table_group, dma_window); > - tbl->it_ops = &iommu_table_lpar_multi_ops; > iommu_init_table(tbl, pci->phb->node, 0, 0); > iommu_register_group(pci->table_group, > pci_domain_nr(pci->phb->bus), 0); >
Hello Alexey, thanks for this feedback! On Mon, 2021-05-10 at 17:34 +1000, Alexey Kardashevskiy wrote: > > > This does not apply anymore as it conflicts with my 4be518d838809e2135. ok, rebasing on top of torvalds/master > > > > --- > > arch/powerpc/platforms/pseries/iommu.c | 100 ++++++++++++---------- > > --- > > 1 file changed, 50 insertions(+), 50 deletions(-) > > > > diff --git a/arch/powerpc/platforms/pseries/iommu.c > > b/arch/powerpc/platforms/pseries/iommu.c > > index 5a70ecd579b8..89cb6e9e9f31 100644 > > --- a/arch/powerpc/platforms/pseries/iommu.c > > +++ b/arch/powerpc/platforms/pseries/iommu.c > > @@ -53,6 +53,11 @@ enum { > > DDW_EXT_QUERY_OUT_SIZE = 2 > > }; > > > > +#ifdef CONFIG_IOMMU_API > > +static int tce_exchange_pseries(struct iommu_table *tbl, long index, > > unsigned long *tce, > > + enum dma_data_direction *direction, > > bool realmode); > > +#endif > > > Instead of declaring this so far from the the code which needs it, may > be add > > struct iommu_table_ops iommu_table_lpar_multi_ops; > > right before iommu_table_setparms() (as the sctruct is what you > actually > want there), > and you won't need to move iommu_table_pseries_ops as well. Oh, I was not aware I could declare a variable and then define it statically. I mean, it does make sense, but I never thought of that. I will change that :) > > > > + > > static struct iommu_table *iommu_pseries_alloc_table(int node) > > { > > struct iommu_table *tbl; > > @@ -501,6 +506,28 @@ static int > > tce_setrange_multi_pSeriesLP_walk(unsigned long start_pfn, > > return tce_setrange_multi_pSeriesLP(start_pfn, num_pfn, arg); > > } > > > > +static inline void _iommu_table_setparms(struct iommu_table *tbl, > > unsigned long busno, > > > The underscore is confusing, may be iommu_table_do_setparms()? > iommu_table_setparms_common()? Not sure. I cannot recall a single > function with just one leading underscore, I suspect I was pushed back > when I tried adding one ages ago :) "inline" seems excessive, the > compiler will probably figure it out anyway. > > sure, done. > > > + unsigned long liobn, > > unsigned long win_addr, > > + unsigned long window_size, > > unsigned long page_shift, > > + unsigned long base, struct > > iommu_table_ops *table_ops) > > > Make "base" a pointer. Or, better, just keep setting it directly in > iommu_table_setparms() rather than passing 0 around. > > The same comment about "liobn" - set it in iommu_table_setparms_lpar(). > The reviewer will see what field atters in what situation imho. > The idea here was to keep all tbl parameters setting in _iommu_table_setparms (or iommu_table_setparms_common). I understand the idea that each one of those is optional in the other case, but should we keep whatever value is present in the other variable (not zeroing the other variable), or do someting like: tbl->it_index = 0; tbl->it_base = basep; (in iommu_table_setparms) tbl->it_index = liobn; tbl->it_base = 0; (in iommu_table_setparms_lpar) > > > +{ > > + tbl->it_busno = busno; > > + tbl->it_index = liobn; > > + tbl->it_offset = win_addr >> page_shift; > > + tbl->it_size = window_size >> page_shift; > > + tbl->it_page_shift = page_shift; > > + tbl->it_base = base; > > + tbl->it_blocksize = 16; > > + tbl->it_type = TCE_PCI; > > + tbl->it_ops = table_ops; > > +} > > + > > +struct iommu_table_ops iommu_table_pseries_ops = { > > + .set = tce_build_pSeries, > > + .clear = tce_free_pSeries, > > + .get = tce_get_pseries > > +}; > > + > > static void iommu_table_setparms(struct pci_controller *phb, > > struct device_node *dn, > > struct iommu_table *tbl) > > @@ -509,8 +536,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 > SZ_2G) > > { > > + udbg_printf("PCI_DMA: Unexpected number of IOAs under > > this PHB.\n"); > > + panic("PCI_DMA: Unexpected number of IOAs under this > > PHB.\n"); > > + } > > > > + node = phb->dn; > > basep = of_get_property(node, "linux,tce-base", NULL); > > sizep = of_get_property(node, "linux,tce-size", NULL); > > if (basep == NULL || sizep == NULL) { > > @@ -519,33 +551,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; > > } > > > > +struct iommu_table_ops iommu_table_lpar_multi_ops = { > > + .set = tce_buildmulti_pSeriesLP, > > +#ifdef CONFIG_IOMMU_API > > + .xchg_no_kill = tce_exchange_pseries, > > +#endif > > + .clear = tce_freemulti_pSeriesLP, > > + .get = tce_get_pSeriesLP > > +}; > > + > > /* > > * iommu_table_setparms_lpar > > * > > @@ -557,28 +581,17 @@ static void iommu_table_setparms_lpar(struct > > pci_controller *phb, > > struct iommu_table_group > > *table_group, > > const __be32 *dma_window) > > { > > - unsigned long offset, size; > > + unsigned long offset, size, liobn; > > > > - of_parse_dma_window(dn, dma_window, &tbl->it_index, &offset, > > &size); > > + of_parse_dma_window(dn, dma_window, &liobn, &offset, &size); > > > > - tbl->it_busno = phb->bus->number; > > - tbl->it_page_shift = IOMMU_PAGE_SHIFT_4K; > > - tbl->it_base = 0; > > - tbl->it_blocksize = 16; > > - tbl->it_type = TCE_PCI; > > - tbl->it_offset = offset >> tbl->it_page_shift; > > - tbl->it_size = size >> tbl->it_page_shift; > > + _iommu_table_setparms(tbl, phb->bus->number, liobn, offset, > > size, IOMMU_PAGE_SHIFT_4K, 0, > > + &iommu_table_lpar_multi_ops); > > > > table_group->tce32_start = offset; > > table_group->tce32_size = size; > > } > > > > -struct iommu_table_ops iommu_table_pseries_ops = { > > - .set = tce_build_pSeries, > > - .clear = tce_free_pSeries, > > - .get = tce_get_pseries > > -}; > > - > > static void pci_dma_bus_setup_pSeries(struct pci_bus *bus) > > { > > struct device_node *dn; > > @@ -647,7 +660,6 @@ static void pci_dma_bus_setup_pSeries(struct > > pci_bus *bus) > > tbl = pci->table_group->tables[0]; > > > > iommu_table_setparms(pci->phb, dn, tbl); > > - tbl->it_ops = &iommu_table_pseries_ops; > > iommu_init_table(tbl, pci->phb->node, 0, 0); > > > > /* Divide the rest (1.75GB) among the children */ > > @@ -664,7 +676,7 @@ static int tce_exchange_pseries(struct > > iommu_table *tbl, long index, unsigned > > bool realmode) > > { > > long rc; > > - unsigned long ioba = (unsigned long) index << tbl- > > >it_page_shift; > > + unsigned long ioba = (unsigned long)index << tbl- > > >it_page_shift; > > > Unrelated change, why, did checkpatch.pl complain? My bad, this one could pass my git-add unnoticed. Reverting. > > > unsigned long flags, oldtce = 0; > > u64 proto_tce = iommu_direction_to_tce_perm(*direction); > > unsigned long newtce = *tce | proto_tce; > > @@ -686,15 +698,6 @@ static int tce_exchange_pseries(struct > > iommu_table *tbl, long index, unsigned > > } > > #endif > > > > -struct iommu_table_ops iommu_table_lpar_multi_ops = { > > - .set = tce_buildmulti_pSeriesLP, > > -#ifdef CONFIG_IOMMU_API > > - .xchg_no_kill = tce_exchange_pseries, > > -#endif > > - .clear = tce_freemulti_pSeriesLP, > > - .get = tce_get_pSeriesLP > > -}; > > - > > static void pci_dma_bus_setup_pSeriesLP(struct pci_bus *bus) > > { > > struct iommu_table *tbl; > > @@ -729,7 +732,6 @@ static void pci_dma_bus_setup_pSeriesLP(struct > > pci_bus *bus) > > tbl = ppci->table_group->tables[0]; > > iommu_table_setparms_lpar(ppci->phb, pdn, tbl, > > ppci->table_group, dma_window); > > - tbl->it_ops = &iommu_table_lpar_multi_ops; > > iommu_init_table(tbl, ppci->phb->node, 0, 0); > > iommu_register_group(ppci->table_group, > > pci_domain_nr(bus), 0); > > @@ -758,7 +760,6 @@ static void pci_dma_dev_setup_pSeries(struct > > pci_dev *dev) > > PCI_DN(dn)->table_group = > > iommu_pseries_alloc_group(phb->node); > > tbl = PCI_DN(dn)->table_group->tables[0]; > > iommu_table_setparms(phb, dn, tbl); > > - tbl->it_ops = &iommu_table_pseries_ops; > > iommu_init_table(tbl, phb->node, 0, 0); > > set_iommu_table_base(&dev->dev, tbl); > > return; > > @@ -1436,7 +1437,6 @@ static void > > pci_dma_dev_setup_pSeriesLP(struct pci_dev *dev) > > tbl = pci->table_group->tables[0]; > > iommu_table_setparms_lpar(pci->phb, pdn, tbl, > > pci->table_group, dma_window); > > - tbl->it_ops = &iommu_table_lpar_multi_ops; > > iommu_init_table(tbl, pci->phb->node, 0, 0); > > iommu_register_group(pci->table_group, > > pci_domain_nr(pci->phb->bus), 0); > > > Best regards, Leonardo Bras
Hello Alexey, On Fri, 2021-06-18 at 19:26 -0300, Leonardo Brás wrote: > > > > > + unsigned long liobn, > > > unsigned long win_addr, > > > + unsigned long > > > window_size, > > > unsigned long page_shift, > > > + unsigned long base, > > > struct > > > iommu_table_ops *table_ops) > > > > > > iommu_table_setparms() rather than passing 0 around. > > > > The same comment about "liobn" - set it in > > iommu_table_setparms_lpar(). > > The reviewer will see what field atters in what situation imho. > > > > The idea here was to keep all tbl parameters setting in > _iommu_table_setparms (or iommu_table_setparms_common). > > I understand the idea that each one of those is optional in the other > case, but should we keep whatever value is present in the other > variable (not zeroing the other variable), or do someting like: > > tbl->it_index = 0; > tbl->it_base = basep; > (in iommu_table_setparms) > > tbl->it_index = liobn; > tbl->it_base = 0; > (in iommu_table_setparms_lpar) > This one is supposed to be a question, but I missed the question mark. Sorry about that. I would like to get your opinion in this :)
On 13/07/2021 14:47, Leonardo Brás wrote: > Hello Alexey, > > On Fri, 2021-06-18 at 19:26 -0300, Leonardo Brás wrote: >>> >>>> + unsigned long liobn, >>>> unsigned long win_addr, >>>> + unsigned long >>>> window_size, >>>> unsigned long page_shift, >>>> + unsigned long base, >>>> struct >>>> iommu_table_ops *table_ops) >>> >>> >>> iommu_table_setparms() rather than passing 0 around. >>> >>> The same comment about "liobn" - set it in >>> iommu_table_setparms_lpar(). >>> The reviewer will see what field atters in what situation imho. >>> >> >> The idea here was to keep all tbl parameters setting in >> _iommu_table_setparms (or iommu_table_setparms_common). >> >> I understand the idea that each one of those is optional in the other >> case, but should we keep whatever value is present in the other >> variable (not zeroing the other variable), or do someting like: >> >> tbl->it_index = 0; >> tbl->it_base = basep; >> (in iommu_table_setparms) >> >> tbl->it_index = liobn; >> tbl->it_base = 0; >> (in iommu_table_setparms_lpar) >> > > This one is supposed to be a question, but I missed the question mark. > Sorry about that. Ah ok :) > I would like to get your opinion in this :) Besides making the "base" parameter a pointer, I really do not have strong preference, just make it not hurting eyes of a reader, that's all :) imho in general, rather than answering 5 weeks later, it is more productive to address whatever comments were made, add comments (in the code or commit logs) why you are sticking to your initial approach, rebase and repost the whole thing. Thanks,
On Wed, 2021-07-14 at 18:32 +1000, Alexey Kardashevskiy wrote: > > > On 13/07/2021 14:47, Leonardo Brás wrote: > > Hello Alexey, > > > > On Fri, 2021-06-18 at 19:26 -0300, Leonardo Brás wrote: > > > > > > > > > + unsigned long liobn, > > > > > unsigned long win_addr, > > > > > + unsigned long > > > > > window_size, > > > > > unsigned long page_shift, > > > > > + unsigned long base, > > > > > struct > > > > > iommu_table_ops *table_ops) > > > > > > > > > > > > iommu_table_setparms() rather than passing 0 around. > > > > > > > > The same comment about "liobn" - set it in > > > > iommu_table_setparms_lpar(). > > > > The reviewer will see what field atters in what situation imho. > > > > > > > > > > The idea here was to keep all tbl parameters setting in > > > _iommu_table_setparms (or iommu_table_setparms_common). > > > > > > I understand the idea that each one of those is optional in the > > > other > > > case, but should we keep whatever value is present in the other > > > variable (not zeroing the other variable), or do someting like: > > > > > > tbl->it_index = 0; > > > tbl->it_base = basep; > > > (in iommu_table_setparms) > > > > > > tbl->it_index = liobn; > > > tbl->it_base = 0; > > > (in iommu_table_setparms_lpar) > > > > > > > This one is supposed to be a question, but I missed the question > > mark. > > Sorry about that. > > Ah ok :) > > > I would like to get your opinion in this :) > > Besides making the "base" parameter a pointer, I really do not have > strong preference, just make it not hurting eyes of a reader, that's > all :) Ok, done :) > imho in general, rather than answering 5 weeks later, it is more > productive to address whatever comments were made, add comments (in > the > code or commit logs) why you are sticking to your initial approach, > rebase and repost the whole thing. Thanks, Thanks for the tip, and for the reviewing :) Best regards, Leonardo Bras
diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c index 5a70ecd579b8..89cb6e9e9f31 100644 --- a/arch/powerpc/platforms/pseries/iommu.c +++ b/arch/powerpc/platforms/pseries/iommu.c @@ -53,6 +53,11 @@ enum { DDW_EXT_QUERY_OUT_SIZE = 2 }; +#ifdef CONFIG_IOMMU_API +static int tce_exchange_pseries(struct iommu_table *tbl, long index, unsigned long *tce, + enum dma_data_direction *direction, bool realmode); +#endif + static struct iommu_table *iommu_pseries_alloc_table(int node) { struct iommu_table *tbl; @@ -501,6 +506,28 @@ static int tce_setrange_multi_pSeriesLP_walk(unsigned long start_pfn, return tce_setrange_multi_pSeriesLP(start_pfn, num_pfn, arg); } +static inline void _iommu_table_setparms(struct iommu_table *tbl, unsigned long busno, + unsigned long liobn, unsigned long win_addr, + unsigned long window_size, unsigned long page_shift, + unsigned long base, struct iommu_table_ops *table_ops) +{ + tbl->it_busno = busno; + tbl->it_index = liobn; + tbl->it_offset = win_addr >> page_shift; + tbl->it_size = window_size >> page_shift; + tbl->it_page_shift = page_shift; + tbl->it_base = base; + tbl->it_blocksize = 16; + tbl->it_type = TCE_PCI; + tbl->it_ops = table_ops; +} + +struct iommu_table_ops iommu_table_pseries_ops = { + .set = tce_build_pSeries, + .clear = tce_free_pSeries, + .get = tce_get_pseries +}; + static void iommu_table_setparms(struct pci_controller *phb, struct device_node *dn, struct iommu_table *tbl) @@ -509,8 +536,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 > SZ_2G) { + udbg_printf("PCI_DMA: Unexpected number of IOAs under this PHB.\n"); + panic("PCI_DMA: Unexpected number of IOAs under this PHB.\n"); + } + node = phb->dn; basep = of_get_property(node, "linux,tce-base", NULL); sizep = of_get_property(node, "linux,tce-size", NULL); if (basep == NULL || sizep == NULL) { @@ -519,33 +551,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; } +struct iommu_table_ops iommu_table_lpar_multi_ops = { + .set = tce_buildmulti_pSeriesLP, +#ifdef CONFIG_IOMMU_API + .xchg_no_kill = tce_exchange_pseries, +#endif + .clear = tce_freemulti_pSeriesLP, + .get = tce_get_pSeriesLP +}; + /* * iommu_table_setparms_lpar * @@ -557,28 +581,17 @@ static void iommu_table_setparms_lpar(struct pci_controller *phb, struct iommu_table_group *table_group, const __be32 *dma_window) { - unsigned long offset, size; + unsigned long offset, size, liobn; - of_parse_dma_window(dn, dma_window, &tbl->it_index, &offset, &size); + of_parse_dma_window(dn, dma_window, &liobn, &offset, &size); - tbl->it_busno = phb->bus->number; - tbl->it_page_shift = IOMMU_PAGE_SHIFT_4K; - tbl->it_base = 0; - tbl->it_blocksize = 16; - tbl->it_type = TCE_PCI; - tbl->it_offset = offset >> tbl->it_page_shift; - tbl->it_size = size >> tbl->it_page_shift; + _iommu_table_setparms(tbl, phb->bus->number, liobn, offset, size, IOMMU_PAGE_SHIFT_4K, 0, + &iommu_table_lpar_multi_ops); table_group->tce32_start = offset; table_group->tce32_size = size; } -struct iommu_table_ops iommu_table_pseries_ops = { - .set = tce_build_pSeries, - .clear = tce_free_pSeries, - .get = tce_get_pseries -}; - static void pci_dma_bus_setup_pSeries(struct pci_bus *bus) { struct device_node *dn; @@ -647,7 +660,6 @@ static void pci_dma_bus_setup_pSeries(struct pci_bus *bus) tbl = pci->table_group->tables[0]; iommu_table_setparms(pci->phb, dn, tbl); - tbl->it_ops = &iommu_table_pseries_ops; iommu_init_table(tbl, pci->phb->node, 0, 0); /* Divide the rest (1.75GB) among the children */ @@ -664,7 +676,7 @@ static int tce_exchange_pseries(struct iommu_table *tbl, long index, unsigned bool realmode) { long rc; - unsigned long ioba = (unsigned long) index << tbl->it_page_shift; + unsigned long ioba = (unsigned long)index << tbl->it_page_shift; unsigned long flags, oldtce = 0; u64 proto_tce = iommu_direction_to_tce_perm(*direction); unsigned long newtce = *tce | proto_tce; @@ -686,15 +698,6 @@ static int tce_exchange_pseries(struct iommu_table *tbl, long index, unsigned } #endif -struct iommu_table_ops iommu_table_lpar_multi_ops = { - .set = tce_buildmulti_pSeriesLP, -#ifdef CONFIG_IOMMU_API - .xchg_no_kill = tce_exchange_pseries, -#endif - .clear = tce_freemulti_pSeriesLP, - .get = tce_get_pSeriesLP -}; - static void pci_dma_bus_setup_pSeriesLP(struct pci_bus *bus) { struct iommu_table *tbl; @@ -729,7 +732,6 @@ static void pci_dma_bus_setup_pSeriesLP(struct pci_bus *bus) tbl = ppci->table_group->tables[0]; iommu_table_setparms_lpar(ppci->phb, pdn, tbl, ppci->table_group, dma_window); - tbl->it_ops = &iommu_table_lpar_multi_ops; iommu_init_table(tbl, ppci->phb->node, 0, 0); iommu_register_group(ppci->table_group, pci_domain_nr(bus), 0); @@ -758,7 +760,6 @@ static void pci_dma_dev_setup_pSeries(struct pci_dev *dev) PCI_DN(dn)->table_group = iommu_pseries_alloc_group(phb->node); tbl = PCI_DN(dn)->table_group->tables[0]; iommu_table_setparms(phb, dn, tbl); - tbl->it_ops = &iommu_table_pseries_ops; iommu_init_table(tbl, phb->node, 0, 0); set_iommu_table_base(&dev->dev, tbl); return; @@ -1436,7 +1437,6 @@ static void pci_dma_dev_setup_pSeriesLP(struct pci_dev *dev) tbl = pci->table_group->tables[0]; iommu_table_setparms_lpar(pci->phb, pdn, tbl, pci->table_group, dma_window); - tbl->it_ops = &iommu_table_lpar_multi_ops; iommu_init_table(tbl, pci->phb->node, 0, 0); iommu_register_group(pci->table_group, pci_domain_nr(pci->phb->bus), 0);
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*(). Signed-off-by: Leonardo Bras <leobras.c@gmail.com> --- arch/powerpc/platforms/pseries/iommu.c | 100 ++++++++++++------------- 1 file changed, 50 insertions(+), 50 deletions(-)