Re: [PATCH v4 07/11] powerpc/pseries/iommu: Reorganize iommu_table_setparms*() with new helper
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
Re: [PATCH v4 07/11] powerpc/pseries/iommu: Reorganize iommu_table_setparms*() with new helper
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, -- Alexey
Re: [PATCH v4 07/11] powerpc/pseries/iommu: Reorganize iommu_table_setparms*() with new helper
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 :)
Re: [PATCH v4 07/11] powerpc/pseries/iommu: Reorganize iommu_table_setparms*() with new helper
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,
>
Re: [PATCH v4 07/11] powerpc/pseries/iommu: Reorganize iommu_table_setparms*() with new helper
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
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 > 0x8000ul) {
- udbg_printf("PCI_DMA: Unexpected number of IOAs under this
PHB.\n");
- panic(
[PATCH v4 07/11] powerpc/pseries/iommu: Reorganize iommu_table_setparms*() with new helper
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
---
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
+
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 > 0x8000ul) {
- 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(d
