Re: [PATCH v8 06/08] iommu/ipmmu-vmsa: Use fwspec iommu_priv on ARM64
Hi Robin, On Wed, May 17, 2017 at 11:29 PM, Robin Murphy wrote: > Hi Magnus, > > On 17/05/17 11:07, Magnus Damm wrote: >> From: Magnus Damm >> >> Convert from archdata to iommu_priv via iommu_fwspec on ARM64 but >> let 32-bit ARM keep on using archdata for now. >> >> Once the 32-bit ARM code and the IPMMU driver is able to move over >> to CONFIG_IOMMU_DMA=y then coverting to fwspec via ->of_xlate() will >> be easy. >> >> For now fwspec ids and num_ids are not used to allow code sharing between >> 32-bit and 64-bit ARM code inside the driver. > > That's not what I meant at all - this just looks like a crazy > unmaintainable mess that's making things that much harder to unpick in > future. > > Leaving the existing code fossilised and building up half of a second > separate driver around it wrapped in ifdefs is not only hideous, it's > more work in the end than simply pulling things into the right shape to > begin with. What I meant was to start with the below (compile-tested > only), and add the of_xlate support on top. AFAICS the arm/arm64 > differences overall should only come down to a bit of special-casing in > add_device(), and (optionally) whether you outright reject > IOMMU_DOMAIN_DMA or not. > > Sorry, but I just can't agree with the two-drivers-in-one approach. Thanks for checking the code and sorry to disappoint you by not using ->ids[] and ->num_ids right away. The two-drivers-on-one approach comes from wanting to use the same IOMMU driver on both 32-bit and 64-bit ARM architectures but the former is lacking IOMMU_DMA support upstream. Obviously using ->ids[] and ->num_ids is the right forward, and in my mind it is only a question of time and merge order. I'm more than happy to make use of your patch but I'm wondering if it will work on 32-bit ARM and R-Car Gen2 that currently does not use ->of_xlate(). I can see that you're using iommu_fwspec_init() so it might work right out of the box. Thanks for your help cooking up the patch by the way. >From my side I was hoping on doing one larger feature jump for 32-bit ARM by moving over to IOMMU_DMA=y together with ->of_xlate()and fwspec at the same time. Doing minor increments of the legacy code that is still using special mapping code instead of OMMU_DMA=y in case of 32-bit ARM just feels like a lot of potential breakage and little gain to me. What's the plan for supporting IOMMU_DMA=y on 32-bit ARM? Anything I can do to help? Or do you prefer minor increments on 32-bit ARM over larger feature jumps? Let me know what you think. My plan is to revisit this topic early next week. Cheers, / magnus
Re: [PATCH v8 06/08] iommu/ipmmu-vmsa: Use fwspec iommu_priv on ARM64
Hi Magnus,
On 17/05/17 11:07, Magnus Damm wrote:
> From: Magnus Damm
>
> Convert from archdata to iommu_priv via iommu_fwspec on ARM64 but
> let 32-bit ARM keep on using archdata for now.
>
> Once the 32-bit ARM code and the IPMMU driver is able to move over
> to CONFIG_IOMMU_DMA=y then coverting to fwspec via ->of_xlate() will
> be easy.
>
> For now fwspec ids and num_ids are not used to allow code sharing between
> 32-bit and 64-bit ARM code inside the driver.
That's not what I meant at all - this just looks like a crazy
unmaintainable mess that's making things that much harder to unpick in
future.
Leaving the existing code fossilised and building up half of a second
separate driver around it wrapped in ifdefs is not only hideous, it's
more work in the end than simply pulling things into the right shape to
begin with. What I meant was to start with the below (compile-tested
only), and add the of_xlate support on top. AFAICS the arm/arm64
differences overall should only come down to a bit of special-casing in
add_device(), and (optionally) whether you outright reject
IOMMU_DOMAIN_DMA or not.
Sorry, but I just can't agree with the two-drivers-in-one approach.
Robin.
->8-
From: Robin Murphy
Subject: [PATCH] iommu/ipmmu-vmsa: Convert to iommu_fwspec
The parent IPMMU pointer and set of uTLB IDs are, as intended, a perfect
fit for the generic iommu_fwspec. Get rid of the architecture-specific
archdata handling in favour of the common solution.
Signed-off-by: Robin Murphy
---
drivers/iommu/ipmmu-vmsa.c | 68
--
1 file changed, 29 insertions(+), 39 deletions(-)
diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index b7e14ee863f9..96479690269f 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -47,12 +47,6 @@ struct ipmmu_vmsa_domain {
spinlock_t lock;/* Protects mappings */
};
-struct ipmmu_vmsa_archdata {
- struct ipmmu_vmsa_device *mmu;
- unsigned int *utlbs;
- unsigned int num_utlbs;
-};
-
static DEFINE_SPINLOCK(ipmmu_devices_lock);
static LIST_HEAD(ipmmu_devices);
@@ -455,6 +449,8 @@ static irqreturn_t ipmmu_irq(int irq, void *dev)
* IOMMU Operations
*/
+static const struct iommu_ops ipmmu_ops;
+
static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
{
struct ipmmu_vmsa_domain *domain;
@@ -487,18 +483,20 @@ static void ipmmu_domain_free(struct iommu_domain
*io_domain)
static int ipmmu_attach_device(struct iommu_domain *io_domain,
struct device *dev)
{
- struct ipmmu_vmsa_archdata *archdata = dev->archdata.iommu;
- struct ipmmu_vmsa_device *mmu = archdata->mmu;
+ struct iommu_fwspec *fwspec = dev->iommu_fwspec;
+ struct ipmmu_vmsa_device *mmu;
struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);
unsigned long flags;
unsigned int i;
int ret = 0;
- if (!mmu) {
+ if (!fwspec || fwspec->ops != &ipmmu_ops) {
dev_err(dev, "Cannot attach to IPMMU\n");
return -ENXIO;
}
+ mmu = fwspec->iommu_priv;
+
spin_lock_irqsave(&domain->lock, flags);
if (!domain->mmu) {
@@ -520,8 +518,8 @@ static int ipmmu_attach_device(struct iommu_domain
*io_domain,
if (ret < 0)
return ret;
- for (i = 0; i < archdata->num_utlbs; ++i)
- ipmmu_utlb_enable(domain, archdata->utlbs[i]);
+ for (i = 0; i < fwspec->num_ids; ++i)
+ ipmmu_utlb_enable(domain, fwspec->ids[i]);
return 0;
}
@@ -529,12 +527,15 @@ static int ipmmu_attach_device(struct iommu_domain
*io_domain,
static void ipmmu_detach_device(struct iommu_domain *io_domain,
struct device *dev)
{
- struct ipmmu_vmsa_archdata *archdata = dev->archdata.iommu;
+ struct iommu_fwspec *fwspec = dev->iommu_fwspec;
struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);
unsigned int i;
- for (i = 0; i < archdata->num_utlbs; ++i)
- ipmmu_utlb_disable(domain, archdata->utlbs[i]);
+ if (!fwspec || fwspec->ops != &ipmmu_ops)
+ return;
+
+ for (i = 0; i < fwspec->num_ids; ++i)
+ ipmmu_utlb_disable(domain, fwspec->ids[i]);
/*
* TODO: Optimize by disabling the context when no device is attached.
@@ -597,7 +598,6 @@ static int ipmmu_find_utlbs(struct ipmmu_vmsa_device
*mmu, struct device *dev,
static int ipmmu_add_device(struct device *dev)
{
- struct ipmmu_vmsa_archdata *archdata;
struct ipmmu_vmsa_device *mmu;
struct iommu_group *group = NULL;
unsigned int *utlbs;
@@ -605,7 +605,7 @@ static int ipmmu_add_device(struct device *dev)
int num_utlbs;
int ret = -ENODEV;
- if (dev->archdata.iommu) {
+ if (dev->iommu_fwspec) {
dev_warn(dev, "IOMMU driver already ass
[PATCH v8 06/08] iommu/ipmmu-vmsa: Use fwspec iommu_priv on ARM64
From: Magnus Damm
Convert from archdata to iommu_priv via iommu_fwspec on ARM64 but
let 32-bit ARM keep on using archdata for now.
Once the 32-bit ARM code and the IPMMU driver is able to move over
to CONFIG_IOMMU_DMA=y then coverting to fwspec via ->of_xlate() will
be easy.
For now fwspec ids and num_ids are not used to allow code sharing between
32-bit and 64-bit ARM code inside the driver.
Signed-off-by: Magnus Damm
---
Changes since V7:
- Rewrote code to use iommu_fwspec, thanks Robin!
- Dropped reviewed-by from Joerg, also skipped tag from Geert
drivers/iommu/ipmmu-vmsa.c | 97 ++--
1 file changed, 58 insertions(+), 39 deletions(-)
--- 0011/drivers/iommu/ipmmu-vmsa.c
+++ work/drivers/iommu/ipmmu-vmsa.c 2017-05-17 16:45:07.510607110 +0900
@@ -56,7 +56,7 @@ struct ipmmu_vmsa_domain {
spinlock_t lock;/* Protects mappings */
};
-struct ipmmu_vmsa_archdata {
+struct ipmmu_vmsa_iommu_priv {
struct ipmmu_vmsa_device *mmu;
unsigned int *utlbs;
unsigned int num_utlbs;
@@ -72,6 +72,24 @@ static struct ipmmu_vmsa_domain *to_vmsa
return container_of(dom, struct ipmmu_vmsa_domain, io_domain);
}
+
+static struct ipmmu_vmsa_iommu_priv *to_priv(struct device *dev)
+{
+#if defined(CONFIG_ARM)
+ return dev->archdata.iommu;
+#else
+ return dev->iommu_fwspec->iommu_priv;
+#endif
+}
+static void set_priv(struct device *dev, struct ipmmu_vmsa_iommu_priv *p)
+{
+#if defined(CONFIG_ARM)
+ dev->archdata.iommu = p;
+#else
+ dev->iommu_fwspec->iommu_priv = p;
+#endif
+}
+
#define TLB_LOOP_TIMEOUT 100 /* 100us */
/*
-
@@ -543,8 +561,8 @@ static void ipmmu_domain_free(struct iom
static int ipmmu_attach_device(struct iommu_domain *io_domain,
struct device *dev)
{
- struct ipmmu_vmsa_archdata *archdata = dev->archdata.iommu;
- struct ipmmu_vmsa_device *mmu = archdata->mmu;
+ struct ipmmu_vmsa_iommu_priv *priv = to_priv(dev);
+ struct ipmmu_vmsa_device *mmu = priv->mmu;
struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);
unsigned long flags;
unsigned int i;
@@ -577,8 +595,8 @@ static int ipmmu_attach_device(struct io
if (ret < 0)
return ret;
- for (i = 0; i < archdata->num_utlbs; ++i)
- ipmmu_utlb_enable(domain, archdata->utlbs[i]);
+ for (i = 0; i < priv->num_utlbs; ++i)
+ ipmmu_utlb_enable(domain, priv->utlbs[i]);
return 0;
}
@@ -586,12 +604,12 @@ static int ipmmu_attach_device(struct io
static void ipmmu_detach_device(struct iommu_domain *io_domain,
struct device *dev)
{
- struct ipmmu_vmsa_archdata *archdata = dev->archdata.iommu;
+ struct ipmmu_vmsa_iommu_priv *priv = to_priv(dev);
struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);
unsigned int i;
- for (i = 0; i < archdata->num_utlbs; ++i)
- ipmmu_utlb_disable(domain, archdata->utlbs[i]);
+ for (i = 0; i < priv->num_utlbs; ++i)
+ ipmmu_utlb_disable(domain, priv->utlbs[i]);
/*
* TODO: Optimize by disabling the context when no device is attached.
@@ -654,7 +672,7 @@ static int ipmmu_find_utlbs(struct ipmmu
static int ipmmu_init_platform_device(struct device *dev)
{
- struct ipmmu_vmsa_archdata *archdata;
+ struct ipmmu_vmsa_iommu_priv *priv;
struct ipmmu_vmsa_device *mmu;
unsigned int *utlbs;
unsigned int i;
@@ -697,17 +715,17 @@ static int ipmmu_init_platform_device(st
}
}
- archdata = kzalloc(sizeof(*archdata), GFP_KERNEL);
- if (!archdata) {
+ priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+ if (!priv) {
ret = -ENOMEM;
goto error;
}
- archdata->mmu = mmu;
- archdata->utlbs = utlbs;
- archdata->num_utlbs = num_utlbs;
- archdata->dev = dev;
- dev->archdata.iommu = archdata;
+ priv->mmu = mmu;
+ priv->utlbs = utlbs;
+ priv->num_utlbs = num_utlbs;
+ priv->dev = dev;
+ set_priv(dev, priv);
return 0;
error:
@@ -727,12 +745,11 @@ static struct iommu_domain *ipmmu_domain
static int ipmmu_add_device(struct device *dev)
{
- struct ipmmu_vmsa_archdata *archdata;
struct ipmmu_vmsa_device *mmu = NULL;
struct iommu_group *group;
int ret;
- if (dev->archdata.iommu) {
+ if (to_priv(dev)) {
dev_warn(dev, "IOMMU driver already assigned to device %s\n",
dev_name(dev));
return -EINVAL;
@@ -768,8 +785,7 @@ static int ipmmu_add_device(struct devic
* - Make the mapping size configurable ? We currently use a 2GB mapping
* at a 1GB offset
