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: <3951d2c425daa7b05e209a6fc8a550c2fca1645f.camel@gmail.com>
Subject: Re: [PATCH v4 10/11] powerpc/pseries/iommu: Make use of DDW for
 indirect mapping
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:   Wed, 14 Jul 2021 16:25:08 -0300
In-Reply-To: <2e88a275-3379-6f14-3c93-d2919bee2142@ozlabs.ru>
References: <20210430163145.146984-1-leobras.c@gmail.com>
         <20210430163145.146984-11-leobras.c@gmail.com>
         <95ac11e9-a709-e0a2-9690-ef13c4a2cd43@ozlabs.ru>
         <5b8676140f495dbbe3e28ce261e449b885dbae66.camel@gmail.com>
         <2e88a275-3379-6f14-3c93-d2919bee2142@ozlabs.ru>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.40.3 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

On Wed, 2021-07-14 at 18:38 +1000, Alexey Kardashevskiy wrote:
> >           for (i = 0; i <
> > > > ARRAY_SIZE(pci->phb->mem_resources);
> > > > i++) {
> > > > +                       const unsigned long mask =
> > > > IORESOURCE_MEM_64
> > > > > IORESOURCE_MEM;
> > > > +
> > > > +                       /* Look for MMIO32 */
> > > > +                       if ((pci->phb->mem_resources[i].flags &
> > > > mask)
> > > > == IORESOURCE_MEM)
> > > > +                               break;
> > > 
> > > What if there is no IORESOURCE_MEM? pci->phb-
> > > >mem_resources[i].start
> > > below will have garbage.
> > 
> > 
> > 
> > Yeah, that makes sense. I will add this lines after 'for':
> > 
> > if (i == ARRAY_SIZE(pci->phb->mem_resources)) {
> >   iommu_tce_table_put(newtbl);
> >   goto out_del_list;
> > }
> > 
> > What do you think?
> 
> 
> Move this and that "for" before iommu_pseries_alloc_table() so you
> won't 
> need to free if there is no IORESOURCE_MEM.

Done!

> 
> 
> > 
> > 
> > > 
> > > 
> > > > +               }
> > > > +
> > > > +               _iommu_table_setparms(newtbl, pci->phb->bus-
> > > > >number,
> > > > create.liobn, win_addr,
> > > > +                                     1UL << len, page_shift,
> > > > 0,
> > > > &iommu_table_lpar_multi_ops);
> > > > +               iommu_init_table(newtbl, pci->phb->node, pci-
> > > > >phb-
> > > > > mem_resources[i].start,
> > > > +                                pci->phb-
> > > > >mem_resources[i].end);
> > > > +
> > > > +               if (default_win_removed)
> > > > +                       iommu_tce_table_put(tbl);
> > > 
> > > 
> > > iommu_tce_table_put() should have been called when the window was
> > > removed.
> > > 
> > > Also after some thinking - what happens if there were 2 devices
> > > in the
> > > PE and one requested 64bit DMA? This will only update
> > > set_iommu_table_base() for the 64bit one but not for the other.
> > > 
> > > I think the right thing to do is:
> > > 
> > > 1. check if table[0] is in use, if yes => fail (which this does
> > > already)
> > > 
> > > 2. remove default dma window but keep the iommu_table struct with
> > > one
> > > change - set it_size to 0 (and free it_map) so the 32bit device
> > > won't
> > > look at a stale structure and think there is some window
> > > (imaginery
> > > situation for phyp but easy to recreate in qemu).
> > > 
> > > 3. use table[1] for newly created indirect DDW window.
> > > 
> > > 4. change get_iommu_table_base() to return a usable table (or may
> > > be
> > > not
> > > needed?).
> > > 
> > > If this sounds reasonable (does it?),
> > 
> > Looks ok, I will try your suggestion.
> > I was not aware of how pci->table_group->tables[] worked, so I
> > replaced
> > pci->table_group->tables[0] with the new tbl, while moving the
> > older in
> > pci->table_group->tables[1].
> 
> 
> pci->table_group->tables[0] is window#0 at @0.
> pci->table_group->tables[1] is window#1 at 0x0800.0000.0000.0000.
> That 
> is all :)
> 
> pseries does not use tables[1] but powernv does (by VFIO only
> though).

Thanks! This helped a lot!

> 
> 
> > (4) get_iommu_table_base() does not seem to need update, as it
> > returns
> > the tlb set by set_iommu_table_base() which is already called in
> > the
> > !direct_mapping path in current patch.
> 
> Sounds right.
> 
> > 
> > >   the question is now if you have
> > > time to do that and the hardware to test that, or I'll have to
> > > finish
> > > the work :)
> > 
> > Sorry, for some reason part of this got lost in Evolution mail
> > client.
> > 
> > If possible, I do want to finish this work, and I am talking to IBM
> > Virt people in order to get testing HW.
> 
> 
> Even I struggle to find a powervm machine :)

> 
> > > 
> > > 
> > > > +               else
> > > > +                       pci->table_group->tables[1] = tbl;
> > > 
> > > 
> > > What is this for?
> > 
> > I was thinking of adding the older table to pci->table_group-
> > >tables[1]
> > while keeping the newer table on pci->table_group->tables[0].
> > This did work, but I think your suggestion may work better.
> > 
> > Best regards,
> > Leonardo Bras
> > 
> > 
> 


Thanks a lot for reviewing Alexey!
I will send a v5 soon.
Best regards,

Leonardo Bras