| Message ID | 1538924364-18781-11-git-send-email-sunil.kovvuri@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | octeontx2-af: Add RVU Admin Function driver | expand |
On Sun, Oct 7, 2018 at 5:01 PM <sunil.kovvuri@gmail.com> wrote: > > + /* HW interprets RVU_AF_MSIXTR_BASE address as an IOVA, hence > + * create a IOMMU mapping for the physcial address configured by > + * firmware and reconfig RVU_AF_MSIXTR_BASE with IOVA. > + */ > + cfg = rvu_read64(rvu, BLKADDR_RVUM, RVU_PRIV_CONST); > + max_msix = cfg & 0xFFFFF; > + phy_addr = rvu_read64(rvu, BLKADDR_RVUM, RVU_AF_MSIXTR_BASE); > + iova = dma_map_single(rvu->dev, (void *)phy_addr, > + max_msix * PCI_MSIX_ENTRY_SIZE, > + DMA_BIDIRECTIONAL); > + if (dma_mapping_error(rvu->dev, iova)) > + return -ENOMEM; > + > + rvu_write64(rvu, BLKADDR_RVUM, RVU_AF_MSIXTR_BASE, (u64)iova); > + rvu->msix_base_iova = iova; > + I'm a bit puzzled by how this works. Does this rely on a specific iommu driver implementation? Normally a physical address makes no sense to the implementation backing dma_map_single() that tries to convert a linear kernel virtual address into a physical address. Arnd
On Mon, Oct 8, 2018 at 5:38 PM Arnd Bergmann <arnd@arndb.de> wrote: > > On Sun, Oct 7, 2018 at 5:01 PM <sunil.kovvuri@gmail.com> wrote: > > > > > + /* HW interprets RVU_AF_MSIXTR_BASE address as an IOVA, hence > > + * create a IOMMU mapping for the physcial address configured by > > + * firmware and reconfig RVU_AF_MSIXTR_BASE with IOVA. > > + */ > > + cfg = rvu_read64(rvu, BLKADDR_RVUM, RVU_PRIV_CONST); > > + max_msix = cfg & 0xFFFFF; > > + phy_addr = rvu_read64(rvu, BLKADDR_RVUM, RVU_AF_MSIXTR_BASE); > > + iova = dma_map_single(rvu->dev, (void *)phy_addr, > > + max_msix * PCI_MSIX_ENTRY_SIZE, > > + DMA_BIDIRECTIONAL); > > + if (dma_mapping_error(rvu->dev, iova)) > > + return -ENOMEM; > > + > > + rvu_write64(rvu, BLKADDR_RVUM, RVU_AF_MSIXTR_BASE, (u64)iova); > > + rvu->msix_base_iova = iova; > > + > > I'm a bit puzzled by how this works. Does this rely on a specific iommu > driver implementation? Normally a physical address makes no sense to the > implementation backing dma_map_single() that tries to convert a > linear kernel virtual address into a physical address. > > Arnd I understand what you are pointing at, but we did test this. IOMMU on this silicon is standard ARM64 SMMUv3. All dma_map_single does is virt_to_page and iommu_dma_map_page converts it back i.e page_to_phys. So the IOMMU driver gets the same physical address passed above and creates a iova translation mapping. For reference below is the captured debug info for the same ===== [ 19.435968] rvu_setup_msix_resources: phy_addr 0x3200000 iova 0xfff80000 [ 19.436967] rvu_setup_msix_resources: virt_to_page(phy_addr) 0xffff7fe00000c800 page_to_phys(page) 0x3200000 offset_in_page(phy_addr) 0x00 ===== Thanks, Sunil.
On Tue, Oct 9, 2018 at 9:03 AM Sunil Kovvuri <sunil.kovvuri@gmail.com> wrote: > On Mon, Oct 8, 2018 at 5:38 PM Arnd Bergmann <arnd@arndb.de> wrote: > > On Sun, Oct 7, 2018 at 5:01 PM <sunil.kovvuri@gmail.com> wrote: > > > > > > > > + /* HW interprets RVU_AF_MSIXTR_BASE address as an IOVA, hence > > > + * create a IOMMU mapping for the physcial address configured by > > > + * firmware and reconfig RVU_AF_MSIXTR_BASE with IOVA. > > > + */ > > > + cfg = rvu_read64(rvu, BLKADDR_RVUM, RVU_PRIV_CONST); > > > + max_msix = cfg & 0xFFFFF; > > > + phy_addr = rvu_read64(rvu, BLKADDR_RVUM, RVU_AF_MSIXTR_BASE); > > > + iova = dma_map_single(rvu->dev, (void *)phy_addr, > > > + max_msix * PCI_MSIX_ENTRY_SIZE, > > > + DMA_BIDIRECTIONAL); > > > + if (dma_mapping_error(rvu->dev, iova)) > > > + return -ENOMEM; > > > + > > > + rvu_write64(rvu, BLKADDR_RVUM, RVU_AF_MSIXTR_BASE, (u64)iova); > > > + rvu->msix_base_iova = iova; > > > + > > > > I'm a bit puzzled by how this works. Does this rely on a specific iommu > > driver implementation? Normally a physical address makes no sense to the > > implementation backing dma_map_single() that tries to convert a > > linear kernel virtual address into a physical address. > > > > Arnd > > I understand what you are pointing at, but we did test this. > IOMMU on this silicon is standard ARM64 SMMUv3. I didn't doubt that you get the right results, I just couldn't see how ;-) > All dma_map_single does is virt_to_page and iommu_dma_map_page > converts it back i.e page_to_phys. > So the IOMMU driver gets the same physical address passed above and > creates a iova translation mapping. > > For reference below is the captured debug info for the same > ===== > [ 19.435968] rvu_setup_msix_resources: phy_addr 0x3200000 iova 0xfff80000 > [ 19.436967] rvu_setup_msix_resources: virt_to_page(phy_addr) > 0xffff7fe00000c800 page_to_phys(page) 0x3200000 > offset_in_page(phy_addr) 0x00 > ===== I think if you enable CONFIG_DEBUG_VIRTUAL, the virt_to_page() above should trigger a warning in phys_addr_t __virt_to_phys(unsigned long x) { WARN(!__is_lm_address(x), "virt_to_phys used for non-linear address: %pK (%pS)\n", (void *)x, (void *)x); return __virt_to_phys_nodebug(x); } Can you verify that? Arnd
On Tue, Oct 9, 2018 at 1:27 PM Arnd Bergmann <arnd@arndb.de> wrote: > > On Tue, Oct 9, 2018 at 9:03 AM Sunil Kovvuri <sunil.kovvuri@gmail.com> wrote: > > On Mon, Oct 8, 2018 at 5:38 PM Arnd Bergmann <arnd@arndb.de> wrote: > > > On Sun, Oct 7, 2018 at 5:01 PM <sunil.kovvuri@gmail.com> wrote: > > > > > > > > > > > + /* HW interprets RVU_AF_MSIXTR_BASE address as an IOVA, hence > > > > + * create a IOMMU mapping for the physcial address configured by > > > > + * firmware and reconfig RVU_AF_MSIXTR_BASE with IOVA. > > > > + */ > > > > + cfg = rvu_read64(rvu, BLKADDR_RVUM, RVU_PRIV_CONST); > > > > + max_msix = cfg & 0xFFFFF; > > > > + phy_addr = rvu_read64(rvu, BLKADDR_RVUM, RVU_AF_MSIXTR_BASE); > > > > + iova = dma_map_single(rvu->dev, (void *)phy_addr, > > > > + max_msix * PCI_MSIX_ENTRY_SIZE, > > > > + DMA_BIDIRECTIONAL); > > > > + if (dma_mapping_error(rvu->dev, iova)) > > > > + return -ENOMEM; > > > > + > > > > + rvu_write64(rvu, BLKADDR_RVUM, RVU_AF_MSIXTR_BASE, (u64)iova); > > > > + rvu->msix_base_iova = iova; > > > > + > > > > > > I'm a bit puzzled by how this works. Does this rely on a specific iommu > > > driver implementation? Normally a physical address makes no sense to the > > > implementation backing dma_map_single() that tries to convert a > > > linear kernel virtual address into a physical address. > > > > > > Arnd > > > > I understand what you are pointing at, but we did test this. > > IOMMU on this silicon is standard ARM64 SMMUv3. > > I didn't doubt that you get the right results, I just couldn't see how ;-) > > > All dma_map_single does is virt_to_page and iommu_dma_map_page > > converts it back i.e page_to_phys. > > So the IOMMU driver gets the same physical address passed above and > > creates a iova translation mapping. > > > > For reference below is the captured debug info for the same > > ===== > > [ 19.435968] rvu_setup_msix_resources: phy_addr 0x3200000 iova 0xfff80000 > > [ 19.436967] rvu_setup_msix_resources: virt_to_page(phy_addr) > > 0xffff7fe00000c800 page_to_phys(page) 0x3200000 > > offset_in_page(phy_addr) 0x00 > > ===== > > I think if you enable CONFIG_DEBUG_VIRTUAL, the virt_to_page() > above should trigger a warning in > > phys_addr_t __virt_to_phys(unsigned long x) > { > WARN(!__is_lm_address(x), > "virt_to_phys used for non-linear address: %pK (%pS)\n", > (void *)x, > (void *)x); > > return __virt_to_phys_nodebug(x); > } > > Can you verify that? > > Arnd No, it isn't, as for 64bit systems CONFIG_SPARSEMEM_VMEMMAP is enabled. But is there any alternative to what is being done ? Thanks, Sunil.
On Tue, Oct 9, 2018 at 11:20 AM Sunil Kovvuri <sunil.kovvuri@gmail.com> wrote: > > On Tue, Oct 9, 2018 at 1:27 PM Arnd Bergmann <arnd@arndb.de> wrote: > > > > On Tue, Oct 9, 2018 at 9:03 AM Sunil Kovvuri <sunil.kovvuri@gmail.com> wrote: > > > On Mon, Oct 8, 2018 at 5:38 PM Arnd Bergmann <arnd@arndb.de> wrote: > > > > On Sun, Oct 7, 2018 at 5:01 PM <sunil.kovvuri@gmail.com> wrote: > > I think if you enable CONFIG_DEBUG_VIRTUAL, the virt_to_page() > > above should trigger a warning in > > > > phys_addr_t __virt_to_phys(unsigned long x) > > { > > WARN(!__is_lm_address(x), > > "virt_to_phys used for non-linear address: %pK (%pS)\n", > > (void *)x, > > (void *)x); > > > > return __virt_to_phys_nodebug(x); > > } > > > > Can you verify that? > > > > Arnd > > No, it isn't, as for 64bit systems CONFIG_SPARSEMEM_VMEMMAP is enabled. But that is also user-selectable, right? It still seems to be really fragile to rely on non-documented behavior of virt_to_phys() here, if that only works for some configurations. > But is there any alternative to what is being done ? I'm not completely sure, but there may be a way to do this correctly using the iommu API instead of the dma-mapping API. What you do here is fairly rare, but not unprecedented. Arnd
On Tue, Oct 9, 2018 at 5:30 PM Arnd Bergmann <arnd@arndb.de> wrote: > > On Tue, Oct 9, 2018 at 11:20 AM Sunil Kovvuri <sunil.kovvuri@gmail.com> wrote: > > > > On Tue, Oct 9, 2018 at 1:27 PM Arnd Bergmann <arnd@arndb.de> wrote: > > > > > > On Tue, Oct 9, 2018 at 9:03 AM Sunil Kovvuri <sunil.kovvuri@gmail.com> wrote: > > > > On Mon, Oct 8, 2018 at 5:38 PM Arnd Bergmann <arnd@arndb.de> wrote: > > > > > On Sun, Oct 7, 2018 at 5:01 PM <sunil.kovvuri@gmail.com> wrote: > > > I think if you enable CONFIG_DEBUG_VIRTUAL, the virt_to_page() > > > above should trigger a warning in > > > > > > phys_addr_t __virt_to_phys(unsigned long x) > > > { > > > WARN(!__is_lm_address(x), > > > "virt_to_phys used for non-linear address: %pK (%pS)\n", > > > (void *)x, > > > (void *)x); > > > > > > return __virt_to_phys_nodebug(x); > > > } > > > > > > Can you verify that? > > > > > > Arnd > > > > No, it isn't, as for 64bit systems CONFIG_SPARSEMEM_VMEMMAP is enabled. > > But that is also user-selectable, right? It still seems to be really > fragile to rely on non-documented behavior of virt_to_phys() > here, if that only works for some configurations. > > > But is there any alternative to what is being done ? > > I'm not completely sure, but there may be a way to do this correctly > using the iommu API instead of the dma-mapping API. What you do > here is fairly rare, but not unprecedented. > > Arnd After further checking the only way i found is using dma_map_resource() which accepts physical address and creates a mapping. Will test and submit next series with the changes. Thanks, Sunil.
On Wed, Oct 10, 2018 at 9:36 AM Sunil Kovvuri <sunil.kovvuri@gmail.com> wrote: > On Tue, Oct 9, 2018 at 5:30 PM Arnd Bergmann <arnd@arndb.de> wrote: > > On Tue, Oct 9, 2018 at 11:20 AM Sunil Kovvuri <sunil.kovvuri@gmail.com> wrote: > > > On Tue, Oct 9, 2018 at 1:27 PM Arnd Bergmann <arnd@arndb.de> wrote: > > > > On Tue, Oct 9, 2018 at 9:03 AM Sunil Kovvuri <sunil.kovvuri@gmail.com> wrote: > > > > > On Mon, Oct 8, 2018 at 5:38 PM Arnd Bergmann <arnd@arndb.de> wrote: > > > > > > On Sun, Oct 7, 2018 at 5:01 PM <sunil.kovvuri@gmail.com> wrote: > > > > I think if you enable CONFIG_DEBUG_VIRTUAL, the virt_to_page() > > > > above should trigger a warning in > > > > > > > > phys_addr_t __virt_to_phys(unsigned long x) > > > > { > > > > WARN(!__is_lm_address(x), > > > > "virt_to_phys used for non-linear address: %pK (%pS)\n", > > > > (void *)x, > > > > (void *)x); > > > > > > > > return __virt_to_phys_nodebug(x); > > > > } > > > > > > > > Can you verify that? > > > > > > No, it isn't, as for 64bit systems CONFIG_SPARSEMEM_VMEMMAP is enabled. > > > > But that is also user-selectable, right? It still seems to be really > > fragile to rely on non-documented behavior of virt_to_phys() > > here, if that only works for some configurations. > > > > > But is there any alternative to what is being done ? > > > > I'm not completely sure, but there may be a way to do this correctly > > using the iommu API instead of the dma-mapping API. What you do > > here is fairly rare, but not unprecedented. > > After further checking the only way i found is using dma_map_resource() which > accepts physical address and creates a mapping. Will test and submit next series > with the changes. Ah, perfect. I remember having seen this in the past, but forgot about it when commenting here. This should be the correct way to do it. If it doesn't work, we need to fix the implementation. Arnd
diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu.c index e4b8ed2..e0c3c18 100644 --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu.c +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu.c @@ -442,9 +442,10 @@ static int rvu_setup_msix_resources(struct rvu *rvu) { struct rvu_hwinfo *hw = rvu->hw; int pf, vf, numvfs, hwvf, err; + int nvecs, offset, max_msix; struct rvu_pfvf *pfvf; - int nvecs, offset; - u64 cfg; + u64 cfg, phy_addr; + dma_addr_t iova; for (pf = 0; pf < hw->total_pfs; pf++) { cfg = rvu_read64(rvu, BLKADDR_RVUM, RVU_PRIV_PFX_CFG(pf)); @@ -523,6 +524,22 @@ static int rvu_setup_msix_resources(struct rvu *rvu) } } + /* HW interprets RVU_AF_MSIXTR_BASE address as an IOVA, hence + * create a IOMMU mapping for the physcial address configured by + * firmware and reconfig RVU_AF_MSIXTR_BASE with IOVA. + */ + cfg = rvu_read64(rvu, BLKADDR_RVUM, RVU_PRIV_CONST); + max_msix = cfg & 0xFFFFF; + phy_addr = rvu_read64(rvu, BLKADDR_RVUM, RVU_AF_MSIXTR_BASE); + iova = dma_map_single(rvu->dev, (void *)phy_addr, + max_msix * PCI_MSIX_ENTRY_SIZE, + DMA_BIDIRECTIONAL); + if (dma_mapping_error(rvu->dev, iova)) + return -ENOMEM; + + rvu_write64(rvu, BLKADDR_RVUM, RVU_AF_MSIXTR_BASE, (u64)iova); + rvu->msix_base_iova = iova; + return 0; } @@ -531,7 +548,8 @@ static void rvu_free_hw_resources(struct rvu *rvu) struct rvu_hwinfo *hw = rvu->hw; struct rvu_block *block; struct rvu_pfvf *pfvf; - int id; + int id, max_msix; + u64 cfg; /* Free block LF bitmaps */ for (id = 0; id < BLK_COUNT; id++) { @@ -549,6 +567,15 @@ static void rvu_free_hw_resources(struct rvu *rvu) pfvf = &rvu->hwvf[id]; kfree(pfvf->msix.bmap); } + + /* Unmap MSIX vector base IOVA mapping */ + if (!rvu->msix_base_iova) + return; + cfg = rvu_read64(rvu, BLKADDR_RVUM, RVU_PRIV_CONST); + max_msix = cfg & 0xFFFFF; + dma_unmap_single(rvu->dev, rvu->msix_base_iova, + max_msix * PCI_MSIX_ENTRY_SIZE, + DMA_BIDIRECTIONAL); } static int rvu_setup_hw_resources(struct rvu *rvu) diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu.h b/drivers/net/ethernet/marvell/octeontx2/af/rvu.h index 7435e83..92c2022 100644 --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu.h +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu.h @@ -99,6 +99,7 @@ struct rvu { u16 num_vec; char *irq_name; bool *irq_allocated; + dma_addr_t msix_base_iova; }; static inline void rvu_write64(struct rvu *rvu, u64 block, u64 offset, u64 val)