Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <97626d3883ed207b818760a8239babb08a6b5c59.camel@gmail.com>
Subject: Re: [PATCH v4 07/11] powerpc/pseries/iommu: Reorganize
 iommu_table_setparms*() with new helper
From:   Leonardo =?ISO-8859-1?Q?Br=E1s?= <leobras.c@gmail.com>
To:     Alexey Kardashevskiy <aik@ozlabs.ru>,
        Michael Ellerman <mpe@ellerman.id.au>,
        Benjamin Herrenschmidt <benh@kernel.crashing.org>,
        Paul Mackerras <paulus@samba.org>,
        Joel Stanley <joel@jms.id.au>,
        Christophe Leroy <christophe.leroy@c-s.fr>,
        Nicolin Chen <nicoleotsuka@gmail.com>,
        Niklas Schnelle <schnelle@linux.ibm.com>
Cc:     linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org
Date:   Fri, 18 Jun 2021 19:26:07 -0300
In-Reply-To: <e4984fa0-2afe-a987-4fb8-61b878431b1f@ozlabs.ru>
References: <20210430163145.146984-1-leobras.c@gmail.com>
         <20210430163145.146984-8-leobras.c@gmail.com>
         <e4984fa0-2afe-a987-4fb8-61b878431b1f@ozlabs.ru>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.40.2 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

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