Re: [PATCH kernel v9 21/32] powerpc/powernv/ioda2: Introduce pnv_pci_ioda2_set_window

From: Alexey Kardashevskiy
Date: Wed Apr 29 2015 - 05:26:41 EST


On 04/29/2015 02:45 PM, David Gibson wrote:
On Sat, Apr 25, 2015 at 10:14:45PM +1000, Alexey Kardashevskiy wrote:
This is a part of moving DMA window programming to an iommu_ops
callback. pnv_pci_ioda2_set_window() takes an iommu_table_group as
a first parameter (not pnv_ioda_pe) as it is going to be used as
a callback for VFIO DDW code.

This adds pnv_pci_ioda2_tvt_invalidate() to invalidate TVT as it is
a good thing to do.

What's the TVT and why is invalidating it a good thing?


"TCE Validation Table". Yeah, I need to rephrase it. Will do.


Also, it looks like it didn't add it, just move it.

Agrh. Lost it in rebases. Will fix.


It does not have immediate effect now as the table
is never recreated after reboot but it will in the following patches.

This should cause no behavioural change.

Signed-off-by: Alexey Kardashevskiy <aik@xxxxxxxxx>
Reviewed-by: David Gibson <david@xxxxxxxxxxxxxxxxxxxxx>

Really? I don't remember this one.


Message-ID: <20150416064351.GK3632@xxxxxxxxxxxxxxx>
:)

But I believe it did not have TVT stuff then so I should have removed your RB from here.


---
Changes:
v9:
* initialize pe->table_group.tables[0] at the very end when
tbl is fully initialized
* moved pnv_pci_ioda2_tvt_invalidate() from earlier patch
---
arch/powerpc/platforms/powernv/pci-ioda.c | 67 +++++++++++++++++++++++--------
1 file changed, 51 insertions(+), 16 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
index b9b3773..59baa15 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
@@ -1960,6 +1960,52 @@ static void pnv_pci_ioda_setup_dma_pe(struct pnv_phb *phb,
__free_pages(tce_mem, get_order(TCE32_TABLE_SIZE * segs));
}

+static long pnv_pci_ioda2_set_window(struct iommu_table_group *table_group,
+ struct iommu_table *tbl)
+{
+ struct pnv_ioda_pe *pe = container_of(table_group, struct pnv_ioda_pe,
+ table_group);
+ struct pnv_phb *phb = pe->phb;
+ int64_t rc;
+ const __u64 start_addr = tbl->it_offset << tbl->it_page_shift;
+ const __u64 win_size = tbl->it_size << tbl->it_page_shift;
+
+ pe_info(pe, "Setting up window at %llx..%llx "
+ "pgsize=0x%x tablesize=0x%lx\n",
+ start_addr, start_addr + win_size - 1,
+ 1UL << tbl->it_page_shift, tbl->it_size << 3);
+
+ tbl->it_table_group = &pe->table_group;
+
+ /*
+ * Map TCE table through TVT. The TVE index is the PE number
+ * shifted by 1 bit for 32-bits DMA space.
+ */
+ rc = opal_pci_map_pe_dma_window(phb->opal_id,
+ pe->pe_number,
+ pe->pe_number << 1,
+ 1,
+ __pa(tbl->it_base),
+ tbl->it_size << 3,
+ 1ULL << tbl->it_page_shift);
+ if (rc) {
+ pe_err(pe, "Failed to configure TCE table, err %ld\n", rc);
+ goto fail;
+ }
+
+ pnv_pci_ioda2_tvt_invalidate(pe);
+
+ /* Store fully initialized *tbl (may be external) in PE */
+ pe->table_group.tables[0] = *tbl;

Hrm, a non-atomic copy of a whole structure into the array. Is that
really what you want?


set_window is called from VFIO (protected by mutex there) and the platform code which I believe is not racy (or hotplug takes care of it anyway). Or I am missing something else?


+ return 0;
+fail:
+ if (pe->tce32_seg >= 0)
+ pe->tce32_seg = -1;
+
+ return rc;
+}
+
static void pnv_pci_ioda2_set_bypass(struct pnv_ioda_pe *pe, bool enable)
{
uint16_t window_id = (pe->pe_number << 1 ) + 1;
@@ -2068,21 +2114,16 @@ static void pnv_pci_ioda2_setup_dma_pe(struct pnv_phb *phb,
pe->table_group.ops = &pnv_pci_ioda2_ops;
#endif

- /*
- * Map TCE table through TVT. The TVE index is the PE number
- * shifted by 1 bit for 32-bits DMA space.
- */
- rc = opal_pci_map_pe_dma_window(phb->opal_id, pe->pe_number,
- pe->pe_number << 1, 1, __pa(tbl->it_base),
- tbl->it_size << 3, 1ULL << tbl->it_page_shift);
+ rc = pnv_pci_ioda2_set_window(&pe->table_group, tbl);
if (rc) {
pe_err(pe, "Failed to configure 32-bit TCE table,"
" err %ld\n", rc);
- goto fail;
+ pnv_pci_free_table(tbl);
+ if (pe->tce32_seg >= 0)
+ pe->tce32_seg = -1;
+ return;
}

- pnv_pci_ioda2_tvt_invalidate(pe);
-
/* OPAL variant of PHB3 invalidated TCEs */
if (pe->tce_inval_reg)
tbl->it_type |= (TCE_PCI_SWINV_CREATE | TCE_PCI_SWINV_FREE);
@@ -2103,12 +2144,6 @@ static void pnv_pci_ioda2_setup_dma_pe(struct pnv_phb *phb,
/* Also create a bypass window */
if (!pnv_iommu_bypass_disabled)
pnv_pci_ioda2_setup_bypass_pe(phb, pe);
-
- return;
-fail:
- if (pe->tce32_seg >= 0)
- pe->tce32_seg = -1;
- pnv_pci_free_table(tbl);
}

static void pnv_ioda_setup_dma(struct pnv_phb *phb)



--
Alexey
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/