Re: [PATCH 1/7] powerpc: Add interface to get msi region information
On Thu, Oct 3, 2013 at 11:19 PM, Bhushan Bharat-R65777 r65...@freescale.com wrote: I don't know enough about VFIO to understand why these new interfaces are needed. Is this the first VFIO IOMMU driver? I see vfio_iommu_spapr_tce.c and vfio_iommu_type1.c but I don't know if they're comparable to the Freescale PAMU. Do other VFIO IOMMU implementations support MSI? If so, do they handle the problem of mapping the MSI regions in a different way? PAMU is an aperture type of IOMMU while other are paging type, So they are completely different from what PAMU is and handle that differently. This is not an explanation or a justification for adding new interfaces. I still have no idea what an aperture type IOMMU is, other than that it is different. But I see that Alex is working on this issue with you in a different thread, so I'm sure you guys will sort it out. Bjorn ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/7] powerpc: Add interface to get msi region information
On Tue, 2013-10-08 at 10:47 -0600, Bjorn Helgaas wrote: On Thu, Oct 3, 2013 at 11:19 PM, Bhushan Bharat-R65777 r65...@freescale.com wrote: I don't know enough about VFIO to understand why these new interfaces are needed. Is this the first VFIO IOMMU driver? I see vfio_iommu_spapr_tce.c and vfio_iommu_type1.c but I don't know if they're comparable to the Freescale PAMU. Do other VFIO IOMMU implementations support MSI? If so, do they handle the problem of mapping the MSI regions in a different way? PAMU is an aperture type of IOMMU while other are paging type, So they are completely different from what PAMU is and handle that differently. This is not an explanation or a justification for adding new interfaces. I still have no idea what an aperture type IOMMU is, other than that it is different. But I see that Alex is working on this issue with you in a different thread, so I'm sure you guys will sort it out. PAMU is a very constrained IOMMU that cannot do arbitrary page mappings. Due to these constraints, we cannot map the MSI I/O page at its normal address while also mapping RAM at the address we want. The address we can map it at depends on the addresses of other mappings, so it can't be hidden in the IOMMU driver -- the user needs to be in control. Another difference is that (if I understand correctly) PCs handle MSIs specially, via interrupt remapping, rather than being translated as a normal memory access through the IOMMU. -Scott ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/7] powerpc: Add interface to get msi region information
On Thu, 2013-09-19 at 12:59 +0530, Bharat Bhushan wrote:
@@ -376,6 +405,7 @@ static int fsl_of_msi_probe(struct platform_device *dev)
int len;
u32 offset;
static const u32 all_avail[] = { 0, NR_MSI_IRQS };
+ static int bank_index;
match = of_match_device(fsl_of_msi_ids, dev-dev);
if (!match)
@@ -419,8 +449,8 @@ static int fsl_of_msi_probe(struct platform_device *dev)
dev-dev.of_node-full_name);
goto error_out;
}
- msi-msiir_offset =
- features-msiir_offset + (res.start 0xf);
+ msi-msiir = res.start + features-msiir_offset;
+ printk(msi-msiir = %llx\n, msi-msiir);
dev_dbg or remove
}
msi-feature = features-fsl_pic_ip;
@@ -470,6 +500,7 @@ static int fsl_of_msi_probe(struct platform_device *dev)
}
}
+ msi-bank_index = bank_index++;
What if multiple MSIs are boing probed in parallel? bank_index is not
atomic.
diff --git a/arch/powerpc/sysdev/fsl_msi.h b/arch/powerpc/sysdev/fsl_msi.h
index 8225f86..6bd5cfc 100644
--- a/arch/powerpc/sysdev/fsl_msi.h
+++ b/arch/powerpc/sysdev/fsl_msi.h
@@ -29,12 +29,19 @@ struct fsl_msi {
struct irq_domain *irqhost;
unsigned long cascade_irq;
-
- u32 msiir_offset; /* Offset of MSIIR, relative to start of CCSR */
+ dma_addr_t msiir; /* MSIIR Address in CCSR */
Are you sure dma_addr_t is right here, versus phys_addr_t? It implies
that it's the output of the DMA API, but I don't think the DMA API is
used in the MSI driver. Perhaps it should be, but we still want the raw
physical address to pass on to VFIO.
void __iomem *msi_regs;
u32 feature;
int msi_virqs[NR_MSI_REG];
+ /*
+ * During probe each bank is assigned a index number.
+ * index number ranges from 0 to 2^32.
+ * Example MSI bank 1 = 0
+ * MSI bank 2 = 1, and so on.
+ */
+ int bank_index;
2^32 doesn't fit in int (nor does 2^32 - 1).
Just say that indices start at 0.
-Scott
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/7] powerpc: Add interface to get msi region information
- u32 msiir_offset; /* Offset of MSIIR, relative to start of CCSR */ + dma_addr_t msiir; /* MSIIR Address in CCSR */ Are you sure dma_addr_t is right here, versus phys_addr_t? It implies that it's the output of the DMA API, but I don't think the DMA API is used in the MSI driver. Perhaps it should be, but we still want the raw physical address to pass on to VFIO. I don't know what msiir is used for, but if it's an address you program into a PCI device, then it's a dma_addr_t even if you didn't get it from the DMA API. Maybe bus_addr_t would have been a more suggestive name than dma_addr_t. That said, I have no idea how this relates to VFIO. Bjorn ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/7] powerpc: Add interface to get msi region information
On Tue, 2013-10-08 at 17:25 -0600, Bjorn Helgaas wrote: - u32 msiir_offset; /* Offset of MSIIR, relative to start of CCSR */ + dma_addr_t msiir; /* MSIIR Address in CCSR */ Are you sure dma_addr_t is right here, versus phys_addr_t? It implies that it's the output of the DMA API, but I don't think the DMA API is used in the MSI driver. Perhaps it should be, but we still want the raw physical address to pass on to VFIO. I don't know what msiir is used for, but if it's an address you program into a PCI device, then it's a dma_addr_t even if you didn't get it from the DMA API. Maybe bus_addr_t would have been a more suggestive name than dma_addr_t. That said, I have no idea how this relates to VFIO. It's a bit awkward because it gets used both as something to program into a PCI device (and it's probably a bug that the DMA API doesn't get used), and also (if I understand the current plans correctly) as a physical address to give to VFIO to be a destination address in an IOMMU mapping. So I think the value we keep here should be a phys_addr_t (it comes straight from the MMIO address in the device tree), which gets trivially turned into a dma_addr_t by the non-VFIO code path because there's currently no translation there. -Scott ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH 1/7] powerpc: Add interface to get msi region information
-Original Message-
From: linux-pci-ow...@vger.kernel.org [mailto:linux-pci-ow...@vger.kernel.org]
On Behalf Of Bjorn Helgaas
Sent: Wednesday, September 25, 2013 5:28 AM
To: Bhushan Bharat-R65777
Cc: alex.william...@redhat.com; j...@8bytes.org; b...@kernel.crashing.org;
ga...@kernel.crashing.org; linux-ker...@vger.kernel.org; linuxppc-
d...@lists.ozlabs.org; linux-...@vger.kernel.org; ag...@suse.de; Wood Scott-
B07421; iommu@lists.linux-foundation.org; Bhushan Bharat-R65777
Subject: Re: [PATCH 1/7] powerpc: Add interface to get msi region information
On Thu, Sep 19, 2013 at 12:59:17PM +0530, Bharat Bhushan wrote:
This patch adds interface to get following information
- Number of MSI regions (which is number of MSI banks for powerpc).
- Get the region address range: Physical page which have the
address/addresses used for generating MSI interrupt
and size of the page.
These are required to create IOMMU (Freescale PAMU) mapping for
devices which are directly assigned using VFIO.
Signed-off-by: Bharat Bhushan bharat.bhus...@freescale.com
---
arch/powerpc/include/asm/machdep.h |8 +++
arch/powerpc/include/asm/pci.h |2 +
arch/powerpc/kernel/msi.c | 18
arch/powerpc/sysdev/fsl_msi.c | 39
+--
arch/powerpc/sysdev/fsl_msi.h | 11 -
drivers/pci/msi.c | 26
include/linux/msi.h|8 +++
include/linux/pci.h| 13
8 files changed, 120 insertions(+), 5 deletions(-)
...
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c index
aca7578..6d85c15 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -30,6 +30,20 @@ static int pci_msi_enable = 1;
/* Arch hooks */
+#ifndef arch_msi_get_region_count
+int arch_msi_get_region_count(void)
+{
+ return 0;
+}
+#endif
+
+#ifndef arch_msi_get_region
+int arch_msi_get_region(int region_num, struct msi_region *region) {
+ return 0;
+}
+#endif
This #define strategy is gone; see 4287d824 (PCI: use weak functions for MSI
arch-specific functions). Please use the weak function strategy for your new
MSI region functions.
ok
+
#ifndef arch_msi_check_device
int arch_msi_check_device(struct pci_dev *dev, int nvec, int type) {
@@ -903,6 +917,18 @@ void pci_disable_msi(struct pci_dev *dev) }
EXPORT_SYMBOL(pci_disable_msi);
+int msi_get_region_count(void)
+{
+ return arch_msi_get_region_count();
+}
+EXPORT_SYMBOL(msi_get_region_count);
+
+int msi_get_region(int region_num, struct msi_region *region) {
+ return arch_msi_get_region(region_num, region); }
+EXPORT_SYMBOL(msi_get_region);
Please split these interface additions, i.e., the drivers/pci/msi.c,
include/linux/msi.h, and include/linux/pci.h changes, into a separate patch.
ok
I don't know enough about VFIO to understand why these new interfaces are
needed. Is this the first VFIO IOMMU driver? I see vfio_iommu_spapr_tce.c
and
vfio_iommu_type1.c but I don't know if they're comparable to the Freescale
PAMU.
Do other VFIO IOMMU implementations support MSI? If so, do they handle the
problem of mapping the MSI regions in a different way?
PAMU is an aperture type of IOMMU while other are paging type, So they are
completely different from what PAMU is and handle that differently.
/**
* pci_msix_table_size - return the number of device's MSI-X table entries
* @dev: pointer to the pci_dev data structure of MSI-X device
function diff --git a/include/linux/msi.h b/include/linux/msi.h index
ee66f3a..ae32601 100644
--- a/include/linux/msi.h
+++ b/include/linux/msi.h
@@ -50,6 +50,12 @@ struct msi_desc {
struct kobject kobj;
};
+struct msi_region {
+ int region_num;
+ dma_addr_t addr;
+ size_t size;
+};
This needs some sort of explanatory comment.
Ok
-Bharat
/*
* The arch hook for setup up msi irqs
*/
@@ -58,5 +64,7 @@ void arch_teardown_msi_irq(unsigned int irq); int
arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type); void
arch_teardown_msi_irqs(struct pci_dev *dev); int
arch_msi_check_device(struct pci_dev* dev, int nvec, int type);
+int arch_msi_get_region_count(void);
+int arch_msi_get_region(int region_num, struct msi_region *region);
#endif /* LINUX_MSI_H */
diff --git a/include/linux/pci.h b/include/linux/pci.h index
186540d..2b26a59 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1126,6 +1126,7 @@ struct msix_entry {
u16 entry; /* driver uses to specify entry, OS writes */
};
+struct msi_region;
#ifndef CONFIG_PCI_MSI
static inline int pci_enable_msi_block(struct pci_dev *dev, unsigned
int nvec) @@ -1168,6 +1169,16 @@ static inline int
pci_msi_enabled(void) {
return 0;
}
+
+static inline int
Re: [PATCH 1/7] powerpc: Add interface to get msi region information
On Thu, Sep 19, 2013 at 12:59:17PM +0530, Bharat Bhushan wrote:
This patch adds interface to get following information
- Number of MSI regions (which is number of MSI banks for powerpc).
- Get the region address range: Physical page which have the
address/addresses used for generating MSI interrupt
and size of the page.
These are required to create IOMMU (Freescale PAMU) mapping for
devices which are directly assigned using VFIO.
Signed-off-by: Bharat Bhushan bharat.bhus...@freescale.com
---
arch/powerpc/include/asm/machdep.h |8 +++
arch/powerpc/include/asm/pci.h |2 +
arch/powerpc/kernel/msi.c | 18
arch/powerpc/sysdev/fsl_msi.c | 39 +--
arch/powerpc/sysdev/fsl_msi.h | 11 -
drivers/pci/msi.c | 26
include/linux/msi.h|8 +++
include/linux/pci.h| 13
8 files changed, 120 insertions(+), 5 deletions(-)
...
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index aca7578..6d85c15 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -30,6 +30,20 @@ static int pci_msi_enable = 1;
/* Arch hooks */
+#ifndef arch_msi_get_region_count
+int arch_msi_get_region_count(void)
+{
+ return 0;
+}
+#endif
+
+#ifndef arch_msi_get_region
+int arch_msi_get_region(int region_num, struct msi_region *region)
+{
+ return 0;
+}
+#endif
This #define strategy is gone; see 4287d824 (PCI: use weak functions for
MSI arch-specific functions). Please use the weak function strategy
for your new MSI region functions.
+
#ifndef arch_msi_check_device
int arch_msi_check_device(struct pci_dev *dev, int nvec, int type)
{
@@ -903,6 +917,18 @@ void pci_disable_msi(struct pci_dev *dev)
}
EXPORT_SYMBOL(pci_disable_msi);
+int msi_get_region_count(void)
+{
+ return arch_msi_get_region_count();
+}
+EXPORT_SYMBOL(msi_get_region_count);
+
+int msi_get_region(int region_num, struct msi_region *region)
+{
+ return arch_msi_get_region(region_num, region);
+}
+EXPORT_SYMBOL(msi_get_region);
Please split these interface additions, i.e., the drivers/pci/msi.c,
include/linux/msi.h, and include/linux/pci.h changes, into a separate
patch.
I don't know enough about VFIO to understand why these new interfaces
are needed. Is this the first VFIO IOMMU driver? I see
vfio_iommu_spapr_tce.c and vfio_iommu_type1.c but I don't know if
they're comparable to the Freescale PAMU. Do other VFIO IOMMU
implementations support MSI? If so, do they handle the problem of
mapping the MSI regions in a different way?
/**
* pci_msix_table_size - return the number of device's MSI-X table entries
* @dev: pointer to the pci_dev data structure of MSI-X device function
diff --git a/include/linux/msi.h b/include/linux/msi.h
index ee66f3a..ae32601 100644
--- a/include/linux/msi.h
+++ b/include/linux/msi.h
@@ -50,6 +50,12 @@ struct msi_desc {
struct kobject kobj;
};
+struct msi_region {
+ int region_num;
+ dma_addr_t addr;
+ size_t size;
+};
This needs some sort of explanatory comment.
/*
* The arch hook for setup up msi irqs
*/
@@ -58,5 +64,7 @@ void arch_teardown_msi_irq(unsigned int irq);
int arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type);
void arch_teardown_msi_irqs(struct pci_dev *dev);
int arch_msi_check_device(struct pci_dev* dev, int nvec, int type);
+int arch_msi_get_region_count(void);
+int arch_msi_get_region(int region_num, struct msi_region *region);
#endif /* LINUX_MSI_H */
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 186540d..2b26a59 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1126,6 +1126,7 @@ struct msix_entry {
u16 entry; /* driver uses to specify entry, OS writes */
};
+struct msi_region;
#ifndef CONFIG_PCI_MSI
static inline int pci_enable_msi_block(struct pci_dev *dev, unsigned int
nvec)
@@ -1168,6 +1169,16 @@ static inline int pci_msi_enabled(void)
{
return 0;
}
+
+static inline int msi_get_region_count(void)
+{
+ return 0;
+}
+
+static inline int msi_get_region(int region_num, struct msi_region *region)
+{
+ return 0;
+}
#else
int pci_enable_msi_block(struct pci_dev *dev, unsigned int nvec);
int pci_enable_msi_block_auto(struct pci_dev *dev, unsigned int *maxvec);
@@ -1180,6 +1191,8 @@ void pci_disable_msix(struct pci_dev *dev);
void msi_remove_pci_irq_vectors(struct pci_dev *dev);
void pci_restore_msi_state(struct pci_dev *dev);
int pci_msi_enabled(void);
+int msi_get_region_count(void);
+int msi_get_region(int region_num, struct msi_region *region);
#endif
#ifdef CONFIG_PCIEPORTBUS
--
1.7.0.4
--
To unsubscribe from this list: send the line unsubscribe linux-pci in
the body of a message to majord...@vger.kernel.org
More majordomo info at
