This patchset primarily adds Broadcom FlexRM reset module for
VFIO platform driver. We also have minor improvments in IOMMU
and VFIO driver to allow VFIO no-IOMMU mode access to FlexRM.
The patches are based on Linux-4.13-rc1 and can also be
found at flexrm-vfio-v1 branch of
https://github.com/Broadcom/arm64-linux.git
Anup Patel (5):
iommu: Add capability IOMMU_CAP_BYPASS
iommu/arm-smmu: add IOMMU_CAP_BYPASS to the ARM SMMU driver
iommu/arm-smmu-v3: add IOMMU_CAP_BYPASS to the ARM SMMUv3 driver
vfio: Allow No-IOMMU mode for IOMMUs with bypass capability
vfio: platform: reset: Add Broadcom FlexRM reset module
drivers/iommu/arm-smmu-v3.c | 2 +
drivers/iommu/arm-smmu.c | 2 +
drivers/vfio/platform/reset/Kconfig | 9 +++
drivers/vfio/platform/reset/Makefile | 1 +
.../vfio/platform/reset/vfio_platform_bcmflexrm.c | 91 ++++++++++++++++++++++
drivers/vfio/vfio.c | 13 +++-
include/linux/iommu.h | 4 +
7 files changed, 119 insertions(+), 3 deletions(-)
create mode 100644 drivers/vfio/platform/reset/vfio_platform_bcmflexrm.c
--
2.7.4
Some of the IOMMUs (such as ARM SMMU) are capable of bypassing
transactions for which no IOMMU domain is configured.
This patch adds IOMMU_CAP_BYPASS which can be used by IOMMU
drivers to advertise transation bypass capability of an IOMMU.
Signed-off-by: Anup Patel <[email protected]>
---
include/linux/iommu.h | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 2cb54ad..6bbb4cc 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -101,6 +101,10 @@ enum iommu_cap {
transactions */
IOMMU_CAP_INTR_REMAP, /* IOMMU supports interrupt isolation */
IOMMU_CAP_NOEXEC, /* IOMMU_NOEXEC flag */
+ IOMMU_CAP_BYPASS, /*
+ * IOMMU can bypass transactions for
+ * which domain is not configured
+ */
};
/*
--
2.7.4
The ARM SMMUv1 and SMMUv2 support bypassing transactions for
which domain is not configured. The patch adds corresponding
IOMMU capability to advertise this fact.
Signed-off-by: Anup Patel <[email protected]>
---
drivers/iommu/arm-smmu.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index bc89b4d..08a9020 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1483,6 +1483,8 @@ static bool arm_smmu_capable(enum iommu_cap cap)
return true;
case IOMMU_CAP_NOEXEC:
return true;
+ case IOMMU_CAP_BYPASS:
+ return true;
default:
return false;
}
--
2.7.4
The ARM SMMUv3 support bypassing transactions for which domain
is not configured. The patch adds corresponding IOMMU capability
to advertise this fact.
Signed-off-by: Anup Patel <[email protected]>
---
drivers/iommu/arm-smmu-v3.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 568c400..a6c7f66 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -1423,6 +1423,8 @@ static bool arm_smmu_capable(enum iommu_cap cap)
return true;
case IOMMU_CAP_NOEXEC:
return true;
+ case IOMMU_CAP_BYPASS:
+ return true;
default:
return false;
}
--
2.7.4
Not allowing No-IOMMU mode for devices already having
iommu_ops on their bus is little conservative.
We now have IOMMU (such as ARM SMMU) which can bypass
transcations for which IOMMU domain is not configured
hence No-IOMMU mode should not be allowed when iommu_ops
are available and IOMMU_CAP_BYPASS is not available.
Signed-off-by: Anup Patel <[email protected]>
---
drivers/vfio/vfio.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 330d505..61f3807 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -124,11 +124,18 @@ struct iommu_group *vfio_iommu_group_get(struct device *dev)
#ifdef CONFIG_VFIO_NOIOMMU
/*
* With noiommu enabled, an IOMMU group will be created for a device
- * that doesn't already have one and doesn't have an iommu_ops on their
- * bus. We set iommudata simply to be able to identify these groups
+ * that:
+ * 1. Doesn't already have IOMMU group
+ * 2. Doesn't have an iommu_ops on their bus
+ * 3. Doesn't have transaction bypass capability if iommu_ops
+ * is available on their bus
+ *
+ * We set iommudata simply to be able to identify these groups
* as special use and for reclamation later.
*/
- if (group || !noiommu || iommu_present(dev->bus))
+ if (group || !noiommu ||
+ (iommu_present(dev->bus) &&
+ !iommu_capable(dev->bus, IOMMU_CAP_BYPASS)))
return group;
group = iommu_group_alloc();
--
2.7.4
This patch adds low-level reset for Broadcom FlexRM to
VFIO platform.
It will do the following:
1. Disable/Deactivate each FlexRM ring
2. Flush each FlexRM ring
The cleanup sequence for FlexRM rings is adapted from
Broadcom FlexRM mailbox driver.
Signed-off-by: Anup Patel <[email protected]>
Reviewed-by: Oza Oza <[email protected]>
Reviewed-by: Scott Branden <[email protected]>
---
drivers/vfio/platform/reset/Kconfig | 9 +++
drivers/vfio/platform/reset/Makefile | 1 +
.../vfio/platform/reset/vfio_platform_bcmflexrm.c | 91 ++++++++++++++++++++++
3 files changed, 101 insertions(+)
create mode 100644 drivers/vfio/platform/reset/vfio_platform_bcmflexrm.c
diff --git a/drivers/vfio/platform/reset/Kconfig b/drivers/vfio/platform/reset/Kconfig
index 70cccc5..8d8d226 100644
--- a/drivers/vfio/platform/reset/Kconfig
+++ b/drivers/vfio/platform/reset/Kconfig
@@ -13,3 +13,12 @@ config VFIO_PLATFORM_AMDXGBE_RESET
Enables the VFIO platform driver to handle reset for AMD XGBE
If you don't know what to do here, say N.
+
+config VFIO_PLATFORM_BCMFLEXRM_RESET
+ tristate "VFIO support for Broadcom FlexRM reset"
+ depends on VFIO_PLATFORM
+ depends on ARCH_BCM_IPROC || COMPILE_TEST
+ help
+ Enables the VFIO platform driver to handle reset for Broadcom FlexRM
+
+ If you don't know what to do here, say N.
diff --git a/drivers/vfio/platform/reset/Makefile b/drivers/vfio/platform/reset/Makefile
index 93f4e23..8d9874b 100644
--- a/drivers/vfio/platform/reset/Makefile
+++ b/drivers/vfio/platform/reset/Makefile
@@ -5,3 +5,4 @@ ccflags-y += -Idrivers/vfio/platform
obj-$(CONFIG_VFIO_PLATFORM_CALXEDAXGMAC_RESET) += vfio-platform-calxedaxgmac.o
obj-$(CONFIG_VFIO_PLATFORM_AMDXGBE_RESET) += vfio-platform-amdxgbe.o
+obj-$(CONFIG_VFIO_PLATFORM_BCMFLEXRM_RESET) += vfio_platform_bcmflexrm.o
diff --git a/drivers/vfio/platform/reset/vfio_platform_bcmflexrm.c b/drivers/vfio/platform/reset/vfio_platform_bcmflexrm.c
new file mode 100644
index 0000000..a890472
--- /dev/null
+++ b/drivers/vfio/platform/reset/vfio_platform_bcmflexrm.c
@@ -0,0 +1,91 @@
+/*
+ * Copyright (C) 2017 Broadcom
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ *
+ * This driver provides reset support for Broadcom FlexRM ring manager
+ * to VFIO platform.
+ */
+
+#include <linux/delay.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+
+#include "vfio_platform_private.h"
+
+/* FlexRM configuration */
+#define RING_REGS_SIZE 0x10000
+#define RING_VER_MAGIC 0x76303031
+
+/* Per-Ring register offsets */
+#define RING_VER 0x000
+#define RING_CONTROL 0x034
+#define RING_FLUSH_DONE 0x038
+
+/* Register RING_CONTROL fields */
+#define CONTROL_FLUSH_SHIFT 5
+#define CONTROL_ACTIVE_SHIFT 4
+
+/* Register RING_FLUSH_DONE fields */
+#define FLUSH_DONE_MASK 0x1
+
+static void vfio_platform_bcmflexrm_shutdown(void __iomem *ring)
+{
+ unsigned int timeout;
+
+ /* Disable/inactivate ring */
+ writel_relaxed(0x0, ring + RING_CONTROL);
+
+ /* Flush ring with timeout of 1s */
+ timeout = 1000;
+ writel_relaxed(BIT(CONTROL_FLUSH_SHIFT), ring + RING_CONTROL);
+ do {
+ if (readl_relaxed(ring + RING_FLUSH_DONE) & FLUSH_DONE_MASK)
+ break;
+ mdelay(1);
+ } while (timeout--);
+
+ if (!timeout)
+ pr_warn("VFIO FlexRM shutdown timedout\n");
+}
+
+static int vfio_platform_bcmflexrm_reset(struct vfio_platform_device *vdev)
+{
+ void __iomem *ring;
+ struct vfio_platform_region *reg = &vdev->regions[0];
+
+ /* Map FlexRM ring registers if not mapped */
+ if (!reg->ioaddr) {
+ reg->ioaddr = ioremap_nocache(reg->addr, reg->size);
+ if (!reg->ioaddr)
+ return -ENOMEM;
+ }
+
+ /* Discover and shutdown each FlexRM ring */
+ for (ring = reg->ioaddr;
+ ring < (reg->ioaddr + reg->size); ring += RING_REGS_SIZE) {
+ if (readl_relaxed(ring + RING_VER) == RING_VER_MAGIC)
+ vfio_platform_bcmflexrm_shutdown(ring);
+ }
+
+ return 0;
+}
+
+module_vfio_reset_handler("brcm,iproc-flexrm-mbox",
+ vfio_platform_bcmflexrm_reset);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Anup Patel <[email protected]>");
+MODULE_DESCRIPTION("Reset support for Broadcom FlexRM VFIO platform device");
--
2.7.4
On 19/07/17 10:33, Anup Patel wrote:
> This patchset primarily adds Broadcom FlexRM reset module for
> VFIO platform driver. We also have minor improvments in IOMMU
> and VFIO driver to allow VFIO no-IOMMU mode access to FlexRM.
I'm struggling to understand the IOMMU changes here - what's the
FlexRM's hardware relationship with the IOMMU, and how is it different
from any other device? Furthermore, if there *is* a relevant IOMMU
present, why would no-IOMMU mode need to be involved at all?
Robin.
> The patches are based on Linux-4.13-rc1 and can also be
> found at flexrm-vfio-v1 branch of
> https://github.com/Broadcom/arm64-linux.git
>
> Anup Patel (5):
> iommu: Add capability IOMMU_CAP_BYPASS
> iommu/arm-smmu: add IOMMU_CAP_BYPASS to the ARM SMMU driver
> iommu/arm-smmu-v3: add IOMMU_CAP_BYPASS to the ARM SMMUv3 driver
> vfio: Allow No-IOMMU mode for IOMMUs with bypass capability
> vfio: platform: reset: Add Broadcom FlexRM reset module
>
> drivers/iommu/arm-smmu-v3.c | 2 +
> drivers/iommu/arm-smmu.c | 2 +
> drivers/vfio/platform/reset/Kconfig | 9 +++
> drivers/vfio/platform/reset/Makefile | 1 +
> .../vfio/platform/reset/vfio_platform_bcmflexrm.c | 91 ++++++++++++++++++++++
> drivers/vfio/vfio.c | 13 +++-
> include/linux/iommu.h | 4 +
> 7 files changed, 119 insertions(+), 3 deletions(-)
> create mode 100644 drivers/vfio/platform/reset/vfio_platform_bcmflexrm.c
>
On 19/07/17 10:33, Anup Patel wrote:
> Some of the IOMMUs (such as ARM SMMU) are capable of bypassing
> transactions for which no IOMMU domain is configured.
>
> This patch adds IOMMU_CAP_BYPASS which can be used by IOMMU
> drivers to advertise transation bypass capability of an IOMMU.
Whatever the intended semantics of this are, I can't help thinking it
would be better served by allowing callers to explicitly allocate their
own IOMMU_DOMAIN_IDENTITY domains. That would also be useful for the
problem we have with legacy virtio devices behind real IOMMUs.
Robin.
> Signed-off-by: Anup Patel <[email protected]>
> ---
> include/linux/iommu.h | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 2cb54ad..6bbb4cc 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -101,6 +101,10 @@ enum iommu_cap {
> transactions */
> IOMMU_CAP_INTR_REMAP, /* IOMMU supports interrupt isolation */
> IOMMU_CAP_NOEXEC, /* IOMMU_NOEXEC flag */
> + IOMMU_CAP_BYPASS, /*
> + * IOMMU can bypass transactions for
> + * which domain is not configured
> + */
> };
>
> /*
>
On 19/07/17 10:33, Anup Patel wrote:
> The ARM SMMUv1 and SMMUv2 support bypassing transactions for
> which domain is not configured. The patch adds corresponding
> IOMMU capability to advertise this fact.
>
> Signed-off-by: Anup Patel <[email protected]>
> ---
> drivers/iommu/arm-smmu.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index bc89b4d..08a9020 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -1483,6 +1483,8 @@ static bool arm_smmu_capable(enum iommu_cap cap)
> return true;
> case IOMMU_CAP_NOEXEC:
> return true;
> + case IOMMU_CAP_BYPASS:
> + return true;
Except when it isn't, of course.
Beware that what we'd like to do in the long term is flip the polarity
of disable_bypass, because there isn't generally a good reason for
Non-Secure DMA to be happening behind Linux's back.
Robin.
> default:
> return false;
> }
>
On 19/07/17 10:33, Anup Patel wrote:
> The ARM SMMUv3 support bypassing transactions for which domain
> is not configured. The patch adds corresponding IOMMU capability
> to advertise this fact.
>
> Signed-off-by: Anup Patel <[email protected]>
> ---
> drivers/iommu/arm-smmu-v3.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index 568c400..a6c7f66 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -1423,6 +1423,8 @@ static bool arm_smmu_capable(enum iommu_cap cap)
> return true;
> case IOMMU_CAP_NOEXEC:
> return true;
> + case IOMMU_CAP_BYPASS:
> + return true;
And this is never true. If Linux knows a device masters through the
SMMU, it will always have a default domain of some sort (either identity
or DMA ops). If Linux doesn't know, then it won't have been able to
initialise the stream table for the relevant stream IDs, thus any
'bypass' DMA is going to raise C_BAD_STE. SMMUv3 can effectively only
bypass unknown stream IDs if disabled entirely.
Robin.
> default:
> return false;
> }
>
On Wed, Jul 19, 2017 at 4:27 PM, Robin Murphy <[email protected]> wrote:
> On 19/07/17 10:33, Anup Patel wrote:
>> This patchset primarily adds Broadcom FlexRM reset module for
>> VFIO platform driver. We also have minor improvments in IOMMU
>> and VFIO driver to allow VFIO no-IOMMU mode access to FlexRM.
>
> I'm struggling to understand the IOMMU changes here - what's the
> FlexRM's hardware relationship with the IOMMU, and how is it different
> from any other device? Furthermore, if there *is* a relevant IOMMU
> present, why would no-IOMMU mode need to be involved at all?
We want to have FlexRM device accessible from user-space
using VFIO platform with and without IOMMU.
Currently, if IOMMU ops are available for platform bus then
I cannot access FlexRM device using VFIO no-IOMMU mode.
Since, SMMU can bypass transactions which do not match
any SMRs, we should allow no-IOMMU mode for devices
when IOMMU group is not available.
Regards,
Anup
On Wed, Jul 19, 2017 at 4:28 PM, Robin Murphy <[email protected]> wrote:
> On 19/07/17 10:33, Anup Patel wrote:
>> Some of the IOMMUs (such as ARM SMMU) are capable of bypassing
>> transactions for which no IOMMU domain is configured.
>>
>> This patch adds IOMMU_CAP_BYPASS which can be used by IOMMU
>> drivers to advertise transation bypass capability of an IOMMU.
>
> Whatever the intended semantics of this are, I can't help thinking it
> would be better served by allowing callers to explicitly allocate their
> own IOMMU_DOMAIN_IDENTITY domains. That would also be useful for the
> problem we have with legacy virtio devices behind real IOMMUs.
We want to use VFIO no-IOMMU mode for FlexRM device but
currently it does not allow on our SOC because IOMMU ops are
registered for platform bus.
Regards,
Anup
On Wed, Jul 19, 2017 at 4:30 PM, Robin Murphy <[email protected]> wrote:
> On 19/07/17 10:33, Anup Patel wrote:
>> The ARM SMMUv3 support bypassing transactions for which domain
>> is not configured. The patch adds corresponding IOMMU capability
>> to advertise this fact.
>>
>> Signed-off-by: Anup Patel <[email protected]>
>> ---
>> drivers/iommu/arm-smmu-v3.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>> index 568c400..a6c7f66 100644
>> --- a/drivers/iommu/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm-smmu-v3.c
>> @@ -1423,6 +1423,8 @@ static bool arm_smmu_capable(enum iommu_cap cap)
>> return true;
>> case IOMMU_CAP_NOEXEC:
>> return true;
>> + case IOMMU_CAP_BYPASS:
>> + return true;
>
> And this is never true. If Linux knows a device masters through the
> SMMU, it will always have a default domain of some sort (either identity
> or DMA ops). If Linux doesn't know, then it won't have been able to
> initialise the stream table for the relevant stream IDs, thus any
> 'bypass' DMA is going to raise C_BAD_STE. SMMUv3 can effectively only
> bypass unknown stream IDs if disabled entirely.
What if we don't want to use IOMMU for certain device and
due to this we never provide "iommus" DT attribute in the
device DT node. Further, we want to access device without
"iommus" DT attribute from user-space using VFIO no-IOMMU.
Regards,
Anup
On Wed, Jul 19, 2017 at 04:49:00PM +0530, Anup Patel wrote:
> On Wed, Jul 19, 2017 at 4:28 PM, Robin Murphy <[email protected]> wrote:
> > On 19/07/17 10:33, Anup Patel wrote:
> >> Some of the IOMMUs (such as ARM SMMU) are capable of bypassing
> >> transactions for which no IOMMU domain is configured.
> >>
> >> This patch adds IOMMU_CAP_BYPASS which can be used by IOMMU
> >> drivers to advertise transation bypass capability of an IOMMU.
> >
> > Whatever the intended semantics of this are, I can't help thinking it
> > would be better served by allowing callers to explicitly allocate their
> > own IOMMU_DOMAIN_IDENTITY domains. That would also be useful for the
> > problem we have with legacy virtio devices behind real IOMMUs.
>
> We want to use VFIO no-IOMMU mode for FlexRM device but
> currently it does not allow on our SOC because IOMMU ops are
> registered for platform bus.
Why do you want to use no-IOMMU mode if you have an IOMMU, and why you do
think the individual IOMMU drivers are the place to implement this?
NAK to the SMMU patches, for the reasons outlined by Robin.
Will
On Wed, Jul 19, 2017 at 04:53:04PM +0530, Anup Patel wrote:
> On Wed, Jul 19, 2017 at 4:30 PM, Robin Murphy <[email protected]> wrote:
> > On 19/07/17 10:33, Anup Patel wrote:
> >> The ARM SMMUv3 support bypassing transactions for which domain
> >> is not configured. The patch adds corresponding IOMMU capability
> >> to advertise this fact.
> >>
> >> Signed-off-by: Anup Patel <[email protected]>
> >> ---
> >> drivers/iommu/arm-smmu-v3.c | 2 ++
> >> 1 file changed, 2 insertions(+)
> >>
> >> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> >> index 568c400..a6c7f66 100644
> >> --- a/drivers/iommu/arm-smmu-v3.c
> >> +++ b/drivers/iommu/arm-smmu-v3.c
> >> @@ -1423,6 +1423,8 @@ static bool arm_smmu_capable(enum iommu_cap cap)
> >> return true;
> >> case IOMMU_CAP_NOEXEC:
> >> return true;
> >> + case IOMMU_CAP_BYPASS:
> >> + return true;
> >
> > And this is never true. If Linux knows a device masters through the
> > SMMU, it will always have a default domain of some sort (either identity
> > or DMA ops). If Linux doesn't know, then it won't have been able to
> > initialise the stream table for the relevant stream IDs, thus any
> > 'bypass' DMA is going to raise C_BAD_STE. SMMUv3 can effectively only
> > bypass unknown stream IDs if disabled entirely.
>
> What if we don't want to use IOMMU for certain device and
> due to this we never provide "iommus" DT attribute in the
> device DT node. Further, we want to access device without
> "iommus" DT attribute from user-space using VFIO no-IOMMU.
Wait, you want to pass a device through to userspace but you don't want to
use the IOMMU? Why not?
If you describe the SMMU in firmware with only a partial topology
description, then you will run into problems with unknown masters trying to
perform DMA. That's the IOMMU doing its job!
Will
On 19/07/17 12:17, Anup Patel wrote:
> On Wed, Jul 19, 2017 at 4:27 PM, Robin Murphy <[email protected]> wrote:
>> On 19/07/17 10:33, Anup Patel wrote:
>>> This patchset primarily adds Broadcom FlexRM reset module for
>>> VFIO platform driver. We also have minor improvments in IOMMU
>>> and VFIO driver to allow VFIO no-IOMMU mode access to FlexRM.
>>
>> I'm struggling to understand the IOMMU changes here - what's the
>> FlexRM's hardware relationship with the IOMMU, and how is it different
>> from any other device? Furthermore, if there *is* a relevant IOMMU
>> present, why would no-IOMMU mode need to be involved at all?
>
> We want to have FlexRM device accessible from user-space
> using VFIO platform with and without IOMMU.
>
> Currently, if IOMMU ops are available for platform bus then
> I cannot access FlexRM device using VFIO no-IOMMU mode.
So does the FlexRM hardware master through the SMMU or not? If it does,
why do you need no-IOMMU mode? If it doesn't, then that's yet another
reason to fix the real problem, which is the utterly broken notion of
there being 'an IOMMU' on the platform 'bus', rather than papering over
it in VFIO.
Robin.
> Since, SMMU can bypass transactions which do not match
> any SMRs, we should allow no-IOMMU mode for devices
> when IOMMU group is not available.
>
> Regards,
> Anup
>
On Wed, Jul 19, 2017 at 4:53 PM, Will Deacon <[email protected]> wrote:
> On Wed, Jul 19, 2017 at 04:49:00PM +0530, Anup Patel wrote:
>> On Wed, Jul 19, 2017 at 4:28 PM, Robin Murphy <[email protected]> wrote:
>> > On 19/07/17 10:33, Anup Patel wrote:
>> >> Some of the IOMMUs (such as ARM SMMU) are capable of bypassing
>> >> transactions for which no IOMMU domain is configured.
>> >>
>> >> This patch adds IOMMU_CAP_BYPASS which can be used by IOMMU
>> >> drivers to advertise transation bypass capability of an IOMMU.
>> >
>> > Whatever the intended semantics of this are, I can't help thinking it
>> > would be better served by allowing callers to explicitly allocate their
>> > own IOMMU_DOMAIN_IDENTITY domains. That would also be useful for the
>> > problem we have with legacy virtio devices behind real IOMMUs.
>>
>> We want to use VFIO no-IOMMU mode for FlexRM device but
>> currently it does not allow on our SOC because IOMMU ops are
>> registered for platform bus.
>
> Why do you want to use no-IOMMU mode if you have an IOMMU, and why you do
> think the individual IOMMU drivers are the place to implement this?
>
> NAK to the SMMU patches, for the reasons outlined by Robin.
We have limited number of SMRs on our SOC.
There are lot of devices for which we can potentially
configure SMMU but then due to limited number of
SMRs so we use SMMU only for certain devices.
For FlexRM device on our SOC, we don't intend to
use SMMU hence we need VFIO no-IOMMU mode
working for FlexRM device on our SOC.
Please re-consider your NAK.
Regards,
Anup
On Wed, Jul 19, 2017 at 4:55 PM, Robin Murphy <[email protected]> wrote:
> On 19/07/17 12:17, Anup Patel wrote:
>> On Wed, Jul 19, 2017 at 4:27 PM, Robin Murphy <[email protected]> wrote:
>>> On 19/07/17 10:33, Anup Patel wrote:
>>>> This patchset primarily adds Broadcom FlexRM reset module for
>>>> VFIO platform driver. We also have minor improvments in IOMMU
>>>> and VFIO driver to allow VFIO no-IOMMU mode access to FlexRM.
>>>
>>> I'm struggling to understand the IOMMU changes here - what's the
>>> FlexRM's hardware relationship with the IOMMU, and how is it different
>>> from any other device? Furthermore, if there *is* a relevant IOMMU
>>> present, why would no-IOMMU mode need to be involved at all?
>>
>> We want to have FlexRM device accessible from user-space
>> using VFIO platform with and without IOMMU.
>>
>> Currently, if IOMMU ops are available for platform bus then
>> I cannot access FlexRM device using VFIO no-IOMMU mode.
>
> So does the FlexRM hardware master through the SMMU or not? If it does,
> why do you need no-IOMMU mode? If it doesn't, then that's yet another
> reason to fix the real problem, which is the utterly broken notion of
> there being 'an IOMMU' on the platform 'bus', rather than papering over
> it in VFIO.
>
We are not trying to paper-over the issue. The ARM SMMU will
have limited number of SMRs so on a big SOC with large number
of DMA-capable devices we can run-out of SMRs if we try to
configure SMMU for all DMA-capable devices.
Regards,
Anup
On 19/07/17 12:26, Anup Patel wrote:
> On Wed, Jul 19, 2017 at 4:53 PM, Will Deacon <[email protected]> wrote:
>> On Wed, Jul 19, 2017 at 04:49:00PM +0530, Anup Patel wrote:
>>> On Wed, Jul 19, 2017 at 4:28 PM, Robin Murphy <[email protected]> wrote:
>>>> On 19/07/17 10:33, Anup Patel wrote:
>>>>> Some of the IOMMUs (such as ARM SMMU) are capable of bypassing
>>>>> transactions for which no IOMMU domain is configured.
>>>>>
>>>>> This patch adds IOMMU_CAP_BYPASS which can be used by IOMMU
>>>>> drivers to advertise transation bypass capability of an IOMMU.
>>>>
>>>> Whatever the intended semantics of this are, I can't help thinking it
>>>> would be better served by allowing callers to explicitly allocate their
>>>> own IOMMU_DOMAIN_IDENTITY domains. That would also be useful for the
>>>> problem we have with legacy virtio devices behind real IOMMUs.
>>>
>>> We want to use VFIO no-IOMMU mode for FlexRM device but
>>> currently it does not allow on our SOC because IOMMU ops are
>>> registered for platform bus.
>>
>> Why do you want to use no-IOMMU mode if you have an IOMMU, and why you do
>> think the individual IOMMU drivers are the place to implement this?
>>
>> NAK to the SMMU patches, for the reasons outlined by Robin.
>
> We have limited number of SMRs on our SOC.
>
> There are lot of devices for which we can potentially
> configure SMMU but then due to limited number of
> SMRs so we use SMMU only for certain devices.
Is the stream ID allocation so whacked out that you can't use masking?
Robin.
> For FlexRM device on our SOC, we don't intend to
> use SMMU hence we need VFIO no-IOMMU mode
> working for FlexRM device on our SOC.
>
> Please re-consider your NAK.
>
> Regards,
> Anup
>
On Wed, Jul 19, 2017 at 04:56:38PM +0530, Anup Patel wrote:
> On Wed, Jul 19, 2017 at 4:53 PM, Will Deacon <[email protected]> wrote:
> > On Wed, Jul 19, 2017 at 04:49:00PM +0530, Anup Patel wrote:
> >> On Wed, Jul 19, 2017 at 4:28 PM, Robin Murphy <[email protected]> wrote:
> >> > On 19/07/17 10:33, Anup Patel wrote:
> >> >> Some of the IOMMUs (such as ARM SMMU) are capable of bypassing
> >> >> transactions for which no IOMMU domain is configured.
> >> >>
> >> >> This patch adds IOMMU_CAP_BYPASS which can be used by IOMMU
> >> >> drivers to advertise transation bypass capability of an IOMMU.
> >> >
> >> > Whatever the intended semantics of this are, I can't help thinking it
> >> > would be better served by allowing callers to explicitly allocate their
> >> > own IOMMU_DOMAIN_IDENTITY domains. That would also be useful for the
> >> > problem we have with legacy virtio devices behind real IOMMUs.
> >>
> >> We want to use VFIO no-IOMMU mode for FlexRM device but
> >> currently it does not allow on our SOC because IOMMU ops are
> >> registered for platform bus.
> >
> > Why do you want to use no-IOMMU mode if you have an IOMMU, and why you do
> > think the individual IOMMU drivers are the place to implement this?
> >
> > NAK to the SMMU patches, for the reasons outlined by Robin.
>
> We have limited number of SMRs on our SOC.
>
> There are lot of devices for which we can potentially
> configure SMMU but then due to limited number of
> SMRs so we use SMMU only for certain devices.
>
> For FlexRM device on our SOC, we don't intend to
> use SMMU hence we need VFIO no-IOMMU mode
> working for FlexRM device on our SOC.
>
> Please re-consider your NAK.
I'm afraid it still stands for the current implementation. If you can't
solve the SMR restriction by grouping things appropriately (which would be
my strong preference), then I think you'll have to follow-up on Robin's
suggestion of implementing support for IDENTITY domains in VFIO for no-IOMMU
mode to be used even when an IOMMU is present.
Will
On Wed, Jul 19, 2017 at 4:55 PM, Will Deacon <[email protected]> wrote:
> On Wed, Jul 19, 2017 at 04:53:04PM +0530, Anup Patel wrote:
>> On Wed, Jul 19, 2017 at 4:30 PM, Robin Murphy <[email protected]> wrote:
>> > On 19/07/17 10:33, Anup Patel wrote:
>> >> The ARM SMMUv3 support bypassing transactions for which domain
>> >> is not configured. The patch adds corresponding IOMMU capability
>> >> to advertise this fact.
>> >>
>> >> Signed-off-by: Anup Patel <[email protected]>
>> >> ---
>> >> drivers/iommu/arm-smmu-v3.c | 2 ++
>> >> 1 file changed, 2 insertions(+)
>> >>
>> >> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>> >> index 568c400..a6c7f66 100644
>> >> --- a/drivers/iommu/arm-smmu-v3.c
>> >> +++ b/drivers/iommu/arm-smmu-v3.c
>> >> @@ -1423,6 +1423,8 @@ static bool arm_smmu_capable(enum iommu_cap cap)
>> >> return true;
>> >> case IOMMU_CAP_NOEXEC:
>> >> return true;
>> >> + case IOMMU_CAP_BYPASS:
>> >> + return true;
>> >
>> > And this is never true. If Linux knows a device masters through the
>> > SMMU, it will always have a default domain of some sort (either identity
>> > or DMA ops). If Linux doesn't know, then it won't have been able to
>> > initialise the stream table for the relevant stream IDs, thus any
>> > 'bypass' DMA is going to raise C_BAD_STE. SMMUv3 can effectively only
>> > bypass unknown stream IDs if disabled entirely.
>>
>> What if we don't want to use IOMMU for certain device and
>> due to this we never provide "iommus" DT attribute in the
>> device DT node. Further, we want to access device without
>> "iommus" DT attribute from user-space using VFIO no-IOMMU.
>
> Wait, you want to pass a device through to userspace but you don't want to
> use the IOMMU? Why not?
>
> If you describe the SMMU in firmware with only a partial topology
> description, then you will run into problems with unknown masters trying to
> perform DMA. That's the IOMMU doing its job!
We are keeping disable_bypass = false. In other words, we
are using bypass mode for unmatched streams. The real
reason is limited number of SMRs due to which we choose
not to provide "iommus" DT attribute for certain devices.
Regards,
Anup
On Wed, Jul 19, 2017 at 05:01:11PM +0530, Anup Patel wrote:
> On Wed, Jul 19, 2017 at 4:55 PM, Will Deacon <[email protected]> wrote:
> > On Wed, Jul 19, 2017 at 04:53:04PM +0530, Anup Patel wrote:
> >> On Wed, Jul 19, 2017 at 4:30 PM, Robin Murphy <[email protected]> wrote:
> >> > On 19/07/17 10:33, Anup Patel wrote:
> >> >> The ARM SMMUv3 support bypassing transactions for which domain
> >> >> is not configured. The patch adds corresponding IOMMU capability
> >> >> to advertise this fact.
> >> >>
> >> >> Signed-off-by: Anup Patel <[email protected]>
> >> >> ---
> >> >> drivers/iommu/arm-smmu-v3.c | 2 ++
> >> >> 1 file changed, 2 insertions(+)
> >> >>
> >> >> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> >> >> index 568c400..a6c7f66 100644
> >> >> --- a/drivers/iommu/arm-smmu-v3.c
> >> >> +++ b/drivers/iommu/arm-smmu-v3.c
> >> >> @@ -1423,6 +1423,8 @@ static bool arm_smmu_capable(enum iommu_cap cap)
> >> >> return true;
> >> >> case IOMMU_CAP_NOEXEC:
> >> >> return true;
> >> >> + case IOMMU_CAP_BYPASS:
> >> >> + return true;
> >> >
> >> > And this is never true. If Linux knows a device masters through the
> >> > SMMU, it will always have a default domain of some sort (either identity
> >> > or DMA ops). If Linux doesn't know, then it won't have been able to
> >> > initialise the stream table for the relevant stream IDs, thus any
> >> > 'bypass' DMA is going to raise C_BAD_STE. SMMUv3 can effectively only
> >> > bypass unknown stream IDs if disabled entirely.
> >>
> >> What if we don't want to use IOMMU for certain device and
> >> due to this we never provide "iommus" DT attribute in the
> >> device DT node. Further, we want to access device without
> >> "iommus" DT attribute from user-space using VFIO no-IOMMU.
> >
> > Wait, you want to pass a device through to userspace but you don't want to
> > use the IOMMU? Why not?
> >
> > If you describe the SMMU in firmware with only a partial topology
> > description, then you will run into problems with unknown masters trying to
> > perform DMA. That's the IOMMU doing its job!
>
> We are keeping disable_bypass = false. In other words, we
> are using bypass mode for unmatched streams. The real
> reason is limited number of SMRs due to which we choose
> not to provide "iommus" DT attribute for certain devices.
Understood, but that's not robust for SMMUv3 and we *really* shouldn't have
a user ABI that changes behaviour based on a cmdline option. VFIO should be
requesting its own identity mappings, if that's what you need.
Will
On Wed, Jul 19, 2017 at 5:00 PM, Will Deacon <[email protected]> wrote:
> On Wed, Jul 19, 2017 at 04:56:38PM +0530, Anup Patel wrote:
>> On Wed, Jul 19, 2017 at 4:53 PM, Will Deacon <[email protected]> wrote:
>> > On Wed, Jul 19, 2017 at 04:49:00PM +0530, Anup Patel wrote:
>> >> On Wed, Jul 19, 2017 at 4:28 PM, Robin Murphy <[email protected]> wrote:
>> >> > On 19/07/17 10:33, Anup Patel wrote:
>> >> >> Some of the IOMMUs (such as ARM SMMU) are capable of bypassing
>> >> >> transactions for which no IOMMU domain is configured.
>> >> >>
>> >> >> This patch adds IOMMU_CAP_BYPASS which can be used by IOMMU
>> >> >> drivers to advertise transation bypass capability of an IOMMU.
>> >> >
>> >> > Whatever the intended semantics of this are, I can't help thinking it
>> >> > would be better served by allowing callers to explicitly allocate their
>> >> > own IOMMU_DOMAIN_IDENTITY domains. That would also be useful for the
>> >> > problem we have with legacy virtio devices behind real IOMMUs.
>> >>
>> >> We want to use VFIO no-IOMMU mode for FlexRM device but
>> >> currently it does not allow on our SOC because IOMMU ops are
>> >> registered for platform bus.
>> >
>> > Why do you want to use no-IOMMU mode if you have an IOMMU, and why you do
>> > think the individual IOMMU drivers are the place to implement this?
>> >
>> > NAK to the SMMU patches, for the reasons outlined by Robin.
>>
>> We have limited number of SMRs on our SOC.
>>
>> There are lot of devices for which we can potentially
>> configure SMMU but then due to limited number of
>> SMRs so we use SMMU only for certain devices.
>>
>> For FlexRM device on our SOC, we don't intend to
>> use SMMU hence we need VFIO no-IOMMU mode
>> working for FlexRM device on our SOC.
>>
>> Please re-consider your NAK.
>
> I'm afraid it still stands for the current implementation. If you can't
> solve the SMR restriction by grouping things appropriately (which would be
> my strong preference), then I think you'll have to follow-up on Robin's
> suggestion of implementing support for IDENTITY domains in VFIO for no-IOMMU
> mode to be used even when an IOMMU is present.
>
Yes, we have considered making stream-id space for
devices to be continuguous but this cannot be fixed
in our current SOC. We will have to live with this
limitation for our current SOC and it will be only
fixed in future.
Regards,
Anup
On Wed, Jul 19, 2017 at 5:03 PM, Will Deacon <[email protected]> wrote:
> On Wed, Jul 19, 2017 at 05:01:11PM +0530, Anup Patel wrote:
>> On Wed, Jul 19, 2017 at 4:55 PM, Will Deacon <[email protected]> wrote:
>> > On Wed, Jul 19, 2017 at 04:53:04PM +0530, Anup Patel wrote:
>> >> On Wed, Jul 19, 2017 at 4:30 PM, Robin Murphy <[email protected]> wrote:
>> >> > On 19/07/17 10:33, Anup Patel wrote:
>> >> >> The ARM SMMUv3 support bypassing transactions for which domain
>> >> >> is not configured. The patch adds corresponding IOMMU capability
>> >> >> to advertise this fact.
>> >> >>
>> >> >> Signed-off-by: Anup Patel <[email protected]>
>> >> >> ---
>> >> >> drivers/iommu/arm-smmu-v3.c | 2 ++
>> >> >> 1 file changed, 2 insertions(+)
>> >> >>
>> >> >> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>> >> >> index 568c400..a6c7f66 100644
>> >> >> --- a/drivers/iommu/arm-smmu-v3.c
>> >> >> +++ b/drivers/iommu/arm-smmu-v3.c
>> >> >> @@ -1423,6 +1423,8 @@ static bool arm_smmu_capable(enum iommu_cap cap)
>> >> >> return true;
>> >> >> case IOMMU_CAP_NOEXEC:
>> >> >> return true;
>> >> >> + case IOMMU_CAP_BYPASS:
>> >> >> + return true;
>> >> >
>> >> > And this is never true. If Linux knows a device masters through the
>> >> > SMMU, it will always have a default domain of some sort (either identity
>> >> > or DMA ops). If Linux doesn't know, then it won't have been able to
>> >> > initialise the stream table for the relevant stream IDs, thus any
>> >> > 'bypass' DMA is going to raise C_BAD_STE. SMMUv3 can effectively only
>> >> > bypass unknown stream IDs if disabled entirely.
>> >>
>> >> What if we don't want to use IOMMU for certain device and
>> >> due to this we never provide "iommus" DT attribute in the
>> >> device DT node. Further, we want to access device without
>> >> "iommus" DT attribute from user-space using VFIO no-IOMMU.
>> >
>> > Wait, you want to pass a device through to userspace but you don't want to
>> > use the IOMMU? Why not?
>> >
>> > If you describe the SMMU in firmware with only a partial topology
>> > description, then you will run into problems with unknown masters trying to
>> > perform DMA. That's the IOMMU doing its job!
>>
>> We are keeping disable_bypass = false. In other words, we
>> are using bypass mode for unmatched streams. The real
>> reason is limited number of SMRs due to which we choose
>> not to provide "iommus" DT attribute for certain devices.
>
> Understood, but that's not robust for SMMUv3 and we *really* shouldn't have
> a user ABI that changes behaviour based on a cmdline option. VFIO should be
> requesting its own identity mappings, if that's what you need.
Currently, the iommu_present() check in vfio_iommu_group_get()
is preventing us allow no-IOMMU mode for certain devices.
Is there a better replacement of iommu_present() check in
vfio_iommu_group_get()?
Regards,
Anup
On Wed, Jul 19, 2017 at 05:09:05PM +0530, Anup Patel wrote:
> On Wed, Jul 19, 2017 at 5:03 PM, Will Deacon <[email protected]> wrote:
> > On Wed, Jul 19, 2017 at 05:01:11PM +0530, Anup Patel wrote:
> >> On Wed, Jul 19, 2017 at 4:55 PM, Will Deacon <[email protected]> wrote:
> >> > On Wed, Jul 19, 2017 at 04:53:04PM +0530, Anup Patel wrote:
> >> >> On Wed, Jul 19, 2017 at 4:30 PM, Robin Murphy <[email protected]> wrote:
> >> >> > On 19/07/17 10:33, Anup Patel wrote:
> >> >> >> The ARM SMMUv3 support bypassing transactions for which domain
> >> >> >> is not configured. The patch adds corresponding IOMMU capability
> >> >> >> to advertise this fact.
> >> >> >>
> >> >> >> Signed-off-by: Anup Patel <[email protected]>
> >> >> >> ---
> >> >> >> drivers/iommu/arm-smmu-v3.c | 2 ++
> >> >> >> 1 file changed, 2 insertions(+)
> >> >> >>
> >> >> >> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> >> >> >> index 568c400..a6c7f66 100644
> >> >> >> --- a/drivers/iommu/arm-smmu-v3.c
> >> >> >> +++ b/drivers/iommu/arm-smmu-v3.c
> >> >> >> @@ -1423,6 +1423,8 @@ static bool arm_smmu_capable(enum iommu_cap cap)
> >> >> >> return true;
> >> >> >> case IOMMU_CAP_NOEXEC:
> >> >> >> return true;
> >> >> >> + case IOMMU_CAP_BYPASS:
> >> >> >> + return true;
> >> >> >
> >> >> > And this is never true. If Linux knows a device masters through the
> >> >> > SMMU, it will always have a default domain of some sort (either identity
> >> >> > or DMA ops). If Linux doesn't know, then it won't have been able to
> >> >> > initialise the stream table for the relevant stream IDs, thus any
> >> >> > 'bypass' DMA is going to raise C_BAD_STE. SMMUv3 can effectively only
> >> >> > bypass unknown stream IDs if disabled entirely.
> >> >>
> >> >> What if we don't want to use IOMMU for certain device and
> >> >> due to this we never provide "iommus" DT attribute in the
> >> >> device DT node. Further, we want to access device without
> >> >> "iommus" DT attribute from user-space using VFIO no-IOMMU.
> >> >
> >> > Wait, you want to pass a device through to userspace but you don't want to
> >> > use the IOMMU? Why not?
> >> >
> >> > If you describe the SMMU in firmware with only a partial topology
> >> > description, then you will run into problems with unknown masters trying to
> >> > perform DMA. That's the IOMMU doing its job!
> >>
> >> We are keeping disable_bypass = false. In other words, we
> >> are using bypass mode for unmatched streams. The real
> >> reason is limited number of SMRs due to which we choose
> >> not to provide "iommus" DT attribute for certain devices.
> >
> > Understood, but that's not robust for SMMUv3 and we *really* shouldn't have
> > a user ABI that changes behaviour based on a cmdline option. VFIO should be
> > requesting its own identity mappings, if that's what you need.
>
> Currently, the iommu_present() check in vfio_iommu_group_get()
> is preventing us allow no-IOMMU mode for certain devices.
>
> Is there a better replacement of iommu_present() check in
> vfio_iommu_group_get()?
There are two things here:
1. iommu_present() is pretty useless, because it applies to a "bus" which
doesn't actually tell you what you need to know for things like the
platform_bus, where some masters might be upstream of an SMMU and
others might not be.
2. If a master *is* upstream of an IOMMU and you want to use no-IOMMU,
then the VFIO no-IOMMU code needs to be extended so that it creates
an IDENTITY domain on that IOMMU.
I think you need to solve both of those to make this robust. That way,
VFIO can accurately tell whether a given master has an IOMMU or not and
then figure out whether or not it needs to create an IDENTITY domain,
or whether it can do like it does at the moment and ignore the IOMMU
entirely (for the no-IOMMU case).
Will
Hi Anup,
NAK - as indicated in internal review please use unmodified Broadcom
legal header in its own comment block.
On 17-07-19 02:33 AM, Anup Patel wrote:
> This patch adds low-level reset for Broadcom FlexRM to
> VFIO platform.
>
> It will do the following:
> 1. Disable/Deactivate each FlexRM ring
> 2. Flush each FlexRM ring
>
> The cleanup sequence for FlexRM rings is adapted from
> Broadcom FlexRM mailbox driver.
>
> Signed-off-by: Anup Patel <[email protected]>
> Reviewed-by: Oza Oza <[email protected]>
> Reviewed-by: Scott Branden <[email protected]>
> ---
> drivers/vfio/platform/reset/Kconfig | 9 +++
> drivers/vfio/platform/reset/Makefile | 1 +
> .../vfio/platform/reset/vfio_platform_bcmflexrm.c | 91 ++++++++++++++++++++++
> 3 files changed, 101 insertions(+)
> create mode 100644 drivers/vfio/platform/reset/vfio_platform_bcmflexrm.c
>
> diff --git a/drivers/vfio/platform/reset/Kconfig b/drivers/vfio/platform/reset/Kconfig
> index 70cccc5..8d8d226 100644
> --- a/drivers/vfio/platform/reset/Kconfig
> +++ b/drivers/vfio/platform/reset/Kconfig
> @@ -13,3 +13,12 @@ config VFIO_PLATFORM_AMDXGBE_RESET
> Enables the VFIO platform driver to handle reset for AMD XGBE
>
> If you don't know what to do here, say N.
> +
> +config VFIO_PLATFORM_BCMFLEXRM_RESET
> + tristate "VFIO support for Broadcom FlexRM reset"
> + depends on VFIO_PLATFORM
> + depends on ARCH_BCM_IPROC || COMPILE_TEST
> + help
> + Enables the VFIO platform driver to handle reset for Broadcom FlexRM
> +
> + If you don't know what to do here, say N.
> diff --git a/drivers/vfio/platform/reset/Makefile b/drivers/vfio/platform/reset/Makefile
> index 93f4e23..8d9874b 100644
> --- a/drivers/vfio/platform/reset/Makefile
> +++ b/drivers/vfio/platform/reset/Makefile
> @@ -5,3 +5,4 @@ ccflags-y += -Idrivers/vfio/platform
>
> obj-$(CONFIG_VFIO_PLATFORM_CALXEDAXGMAC_RESET) += vfio-platform-calxedaxgmac.o
> obj-$(CONFIG_VFIO_PLATFORM_AMDXGBE_RESET) += vfio-platform-amdxgbe.o
> +obj-$(CONFIG_VFIO_PLATFORM_BCMFLEXRM_RESET) += vfio_platform_bcmflexrm.o
> diff --git a/drivers/vfio/platform/reset/vfio_platform_bcmflexrm.c b/drivers/vfio/platform/reset/vfio_platform_bcmflexrm.c
> new file mode 100644
> index 0000000..a890472
> --- /dev/null
> +++ b/drivers/vfio/platform/reset/vfio_platform_bcmflexrm.c
> @@ -0,0 +1,91 @@
> +/*
> + * Copyright (C) 2017 Broadcom
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + *
*/
Place additional comments in different comment block.
/*
> + * This driver provides reset support for Broadcom FlexRM ring manager
> + * to VFIO platform.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +
> +#include "vfio_platform_private.h"
> +
> +/* FlexRM configuration */
> +#define RING_REGS_SIZE 0x10000
> +#define RING_VER_MAGIC 0x76303031
> +
> +/* Per-Ring register offsets */
> +#define RING_VER 0x000
> +#define RING_CONTROL 0x034
> +#define RING_FLUSH_DONE 0x038
> +
> +/* Register RING_CONTROL fields */
> +#define CONTROL_FLUSH_SHIFT 5
> +#define CONTROL_ACTIVE_SHIFT 4
> +
> +/* Register RING_FLUSH_DONE fields */
> +#define FLUSH_DONE_MASK 0x1
> +
> +static void vfio_platform_bcmflexrm_shutdown(void __iomem *ring)
> +{
> + unsigned int timeout;
> +
> + /* Disable/inactivate ring */
> + writel_relaxed(0x0, ring + RING_CONTROL);
> +
> + /* Flush ring with timeout of 1s */
> + timeout = 1000;
> + writel_relaxed(BIT(CONTROL_FLUSH_SHIFT), ring + RING_CONTROL);
> + do {
> + if (readl_relaxed(ring + RING_FLUSH_DONE) & FLUSH_DONE_MASK)
> + break;
> + mdelay(1);
> + } while (timeout--);
> +
> + if (!timeout)
> + pr_warn("VFIO FlexRM shutdown timedout\n");
> +}
> +
> +static int vfio_platform_bcmflexrm_reset(struct vfio_platform_device *vdev)
> +{
> + void __iomem *ring;
> + struct vfio_platform_region *reg = &vdev->regions[0];
> +
> + /* Map FlexRM ring registers if not mapped */
> + if (!reg->ioaddr) {
> + reg->ioaddr = ioremap_nocache(reg->addr, reg->size);
> + if (!reg->ioaddr)
> + return -ENOMEM;
> + }
> +
> + /* Discover and shutdown each FlexRM ring */
> + for (ring = reg->ioaddr;
> + ring < (reg->ioaddr + reg->size); ring += RING_REGS_SIZE) {
> + if (readl_relaxed(ring + RING_VER) == RING_VER_MAGIC)
> + vfio_platform_bcmflexrm_shutdown(ring);
> + }
> +
> + return 0;
> +}
> +
> +module_vfio_reset_handler("brcm,iproc-flexrm-mbox",
> + vfio_platform_bcmflexrm_reset);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Anup Patel <[email protected]>");
> +MODULE_DESCRIPTION("Reset support for Broadcom FlexRM VFIO platform device");
Thanks,
Scott
On Wed, Jul 19, 2017 at 5:23 PM, Will Deacon <[email protected]> wrote:
> On Wed, Jul 19, 2017 at 05:09:05PM +0530, Anup Patel wrote:
>> On Wed, Jul 19, 2017 at 5:03 PM, Will Deacon <[email protected]> wrote:
>> > On Wed, Jul 19, 2017 at 05:01:11PM +0530, Anup Patel wrote:
>> >> On Wed, Jul 19, 2017 at 4:55 PM, Will Deacon <[email protected]> wrote:
>> >> > On Wed, Jul 19, 2017 at 04:53:04PM +0530, Anup Patel wrote:
>> >> >> On Wed, Jul 19, 2017 at 4:30 PM, Robin Murphy <[email protected]> wrote:
>> >> >> > On 19/07/17 10:33, Anup Patel wrote:
>> >> >> >> The ARM SMMUv3 support bypassing transactions for which domain
>> >> >> >> is not configured. The patch adds corresponding IOMMU capability
>> >> >> >> to advertise this fact.
>> >> >> >>
>> >> >> >> Signed-off-by: Anup Patel <[email protected]>
>> >> >> >> ---
>> >> >> >> drivers/iommu/arm-smmu-v3.c | 2 ++
>> >> >> >> 1 file changed, 2 insertions(+)
>> >> >> >>
>> >> >> >> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>> >> >> >> index 568c400..a6c7f66 100644
>> >> >> >> --- a/drivers/iommu/arm-smmu-v3.c
>> >> >> >> +++ b/drivers/iommu/arm-smmu-v3.c
>> >> >> >> @@ -1423,6 +1423,8 @@ static bool arm_smmu_capable(enum iommu_cap cap)
>> >> >> >> return true;
>> >> >> >> case IOMMU_CAP_NOEXEC:
>> >> >> >> return true;
>> >> >> >> + case IOMMU_CAP_BYPASS:
>> >> >> >> + return true;
>> >> >> >
>> >> >> > And this is never true. If Linux knows a device masters through the
>> >> >> > SMMU, it will always have a default domain of some sort (either identity
>> >> >> > or DMA ops). If Linux doesn't know, then it won't have been able to
>> >> >> > initialise the stream table for the relevant stream IDs, thus any
>> >> >> > 'bypass' DMA is going to raise C_BAD_STE. SMMUv3 can effectively only
>> >> >> > bypass unknown stream IDs if disabled entirely.
>> >> >>
>> >> >> What if we don't want to use IOMMU for certain device and
>> >> >> due to this we never provide "iommus" DT attribute in the
>> >> >> device DT node. Further, we want to access device without
>> >> >> "iommus" DT attribute from user-space using VFIO no-IOMMU.
>> >> >
>> >> > Wait, you want to pass a device through to userspace but you don't want to
>> >> > use the IOMMU? Why not?
>> >> >
>> >> > If you describe the SMMU in firmware with only a partial topology
>> >> > description, then you will run into problems with unknown masters trying to
>> >> > perform DMA. That's the IOMMU doing its job!
>> >>
>> >> We are keeping disable_bypass = false. In other words, we
>> >> are using bypass mode for unmatched streams. The real
>> >> reason is limited number of SMRs due to which we choose
>> >> not to provide "iommus" DT attribute for certain devices.
>> >
>> > Understood, but that's not robust for SMMUv3 and we *really* shouldn't have
>> > a user ABI that changes behaviour based on a cmdline option. VFIO should be
>> > requesting its own identity mappings, if that's what you need.
>>
>> Currently, the iommu_present() check in vfio_iommu_group_get()
>> is preventing us allow no-IOMMU mode for certain devices.
>>
>> Is there a better replacement of iommu_present() check in
>> vfio_iommu_group_get()?
>
> There are two things here:
>
> 1. iommu_present() is pretty useless, because it applies to a "bus" which
> doesn't actually tell you what you need to know for things like the
> platform_bus, where some masters might be upstream of an SMMU and
> others might not be.
I agree with you. The iommu_present() check in vfio_iommu_group_get()
is not much useful. We only reach line which checks iommu_present()
when iommu_group_get() returns NULL for given "struct device *". If there
is no IOMMU group for a "struct device *" then it means there is no IOMMU
HW doing translations for such device.
If we drop the iommu_present() check (due to above reasons) in
vfio_iommu_group_get() then we don't require the IOMMU_CAP_BYPASS
and we can happily drop PATCH1, PATCH2, and PATCH3.
I will remove the iommu_present() check in vfio_iommu_group_get()
because it is only comes into actions when VFIO_NOIOMMU is
enabled. This will also help us drop PATCH1-to-PATCH3.
>
> 2. If a master *is* upstream of an IOMMU and you want to use no-IOMMU,
> then the VFIO no-IOMMU code needs to be extended so that it creates
> an IDENTITY domain on that IOMMU.
The VFIO no-IOMMU mode is equivalent to Linux UIO hence having
IDENTITY domain for VFIO no-IOMMU is not appropriate here.
In fact, going forward it has been suggested to use VFIO no-IOMMU
instead of Linux UIO for user-space poll-mode drivers so that we have
one Linux interface for user-space poll-mode drivers.
Regards,
Anup
On Wed, Jul 19, 2017 at 10:20 PM, Scott Branden
<[email protected]> wrote:
> Hi Anup,
>
> NAK - as indicated in internal review please use unmodified Broadcom legal
> header in its own comment block.
I had addressed your internal review comments and used
standard GLPv2 header (also present in other drivers). The
only part I missed was to keep additional comments as
separate comment block (which you had not mentioned
in internal review).
Anyways, I will update the comment header but I think
using "NAK" in such situation is a bit over-kill
Regards,
Anup
On Thu, Jul 20, 2017 at 09:32:00AM +0530, Anup Patel wrote:
> On Wed, Jul 19, 2017 at 5:23 PM, Will Deacon <[email protected]> wrote:
> > There are two things here:
> >
> > 1. iommu_present() is pretty useless, because it applies to a "bus" which
> > doesn't actually tell you what you need to know for things like the
> > platform_bus, where some masters might be upstream of an SMMU and
> > others might not be.
>
> I agree with you. The iommu_present() check in vfio_iommu_group_get()
> is not much useful. We only reach line which checks iommu_present()
> when iommu_group_get() returns NULL for given "struct device *". If there
> is no IOMMU group for a "struct device *" then it means there is no IOMMU
> HW doing translations for such device.
>
> If we drop the iommu_present() check (due to above reasons) in
> vfio_iommu_group_get() then we don't require the IOMMU_CAP_BYPASS
> and we can happily drop PATCH1, PATCH2, and PATCH3.
>
> I will remove the iommu_present() check in vfio_iommu_group_get()
> because it is only comes into actions when VFIO_NOIOMMU is
> enabled. This will also help us drop PATCH1-to-PATCH3.
I don't think that's the right answer. Whilst iommu_present has obvious
shortcomings, its intention is clear: it should tell you whether a given
*device* is upstream of an IOMMU. So the right fix is to make this
per-device, instead of per-bus. Removing it altogether is worse than leaving
it like it is.
> > 2. If a master *is* upstream of an IOMMU and you want to use no-IOMMU,
> > then the VFIO no-IOMMU code needs to be extended so that it creates
> > an IDENTITY domain on that IOMMU.
>
> The VFIO no-IOMMU mode is equivalent to Linux UIO hence having
> IDENTITY domain for VFIO no-IOMMU is not appropriate here.
Can you elaborate on this please? I don't understand the argument you're
making. It's like saying "I don't like eggs, therefore I don't drive a
car".
Will
On Thu, Jul 20, 2017 at 2:40 PM, Will Deacon <[email protected]> wrote:
> On Thu, Jul 20, 2017 at 09:32:00AM +0530, Anup Patel wrote:
>> On Wed, Jul 19, 2017 at 5:23 PM, Will Deacon <[email protected]> wrote:
>> > There are two things here:
>> >
>> > 1. iommu_present() is pretty useless, because it applies to a "bus" which
>> > doesn't actually tell you what you need to know for things like the
>> > platform_bus, where some masters might be upstream of an SMMU and
>> > others might not be.
>>
>> I agree with you. The iommu_present() check in vfio_iommu_group_get()
>> is not much useful. We only reach line which checks iommu_present()
>> when iommu_group_get() returns NULL for given "struct device *". If there
>> is no IOMMU group for a "struct device *" then it means there is no IOMMU
>> HW doing translations for such device.
>>
>> If we drop the iommu_present() check (due to above reasons) in
>> vfio_iommu_group_get() then we don't require the IOMMU_CAP_BYPASS
>> and we can happily drop PATCH1, PATCH2, and PATCH3.
>>
>> I will remove the iommu_present() check in vfio_iommu_group_get()
>> because it is only comes into actions when VFIO_NOIOMMU is
>> enabled. This will also help us drop PATCH1-to-PATCH3.
>
> I don't think that's the right answer. Whilst iommu_present has obvious
> shortcomings, its intention is clear: it should tell you whether a given
> *device* is upstream of an IOMMU. So the right fix is to make this
> per-device, instead of per-bus. Removing it altogether is worse than leaving
> it like it is.
>
>> > 2. If a master *is* upstream of an IOMMU and you want to use no-IOMMU,
>> > then the VFIO no-IOMMU code needs to be extended so that it creates
>> > an IDENTITY domain on that IOMMU.
>>
>> The VFIO no-IOMMU mode is equivalent to Linux UIO hence having
>> IDENTITY domain for VFIO no-IOMMU is not appropriate here.
>
> Can you elaborate on this please? I don't understand the argument you're
> making. It's like saying "I don't like eggs, therefore I don't drive a
> car".
>
Like I said, VFIO no-IOMMU mode for a device means device transactions
will not go through any IOMMU. That's why having IDENTITY domain for
device using VFIO no-IOMMU is not semantically correct. The analogy you
proposed does not apply here.
Anyways, this patch has nothing to do with FlexRM support for
VFIO platform hence I will drop it. Fixing VFIO no-IOMMU mode
can be a separate patchset.
Regards,
Anup
On Thu, Jul 20, 2017 at 04:38:09PM +0530, Anup Patel wrote:
> On Thu, Jul 20, 2017 at 2:40 PM, Will Deacon <[email protected]> wrote:
> > On Thu, Jul 20, 2017 at 09:32:00AM +0530, Anup Patel wrote:
> >> On Wed, Jul 19, 2017 at 5:23 PM, Will Deacon <[email protected]> wrote:
> >> > There are two things here:
> >> >
> >> > 1. iommu_present() is pretty useless, because it applies to a "bus" which
> >> > doesn't actually tell you what you need to know for things like the
> >> > platform_bus, where some masters might be upstream of an SMMU and
> >> > others might not be.
> >>
> >> I agree with you. The iommu_present() check in vfio_iommu_group_get()
> >> is not much useful. We only reach line which checks iommu_present()
> >> when iommu_group_get() returns NULL for given "struct device *". If there
> >> is no IOMMU group for a "struct device *" then it means there is no IOMMU
> >> HW doing translations for such device.
> >>
> >> If we drop the iommu_present() check (due to above reasons) in
> >> vfio_iommu_group_get() then we don't require the IOMMU_CAP_BYPASS
> >> and we can happily drop PATCH1, PATCH2, and PATCH3.
> >>
> >> I will remove the iommu_present() check in vfio_iommu_group_get()
> >> because it is only comes into actions when VFIO_NOIOMMU is
> >> enabled. This will also help us drop PATCH1-to-PATCH3.
> >
> > I don't think that's the right answer. Whilst iommu_present has obvious
> > shortcomings, its intention is clear: it should tell you whether a given
> > *device* is upstream of an IOMMU. So the right fix is to make this
> > per-device, instead of per-bus. Removing it altogether is worse than leaving
> > it like it is.
> >
> >> > 2. If a master *is* upstream of an IOMMU and you want to use no-IOMMU,
> >> > then the VFIO no-IOMMU code needs to be extended so that it creates
> >> > an IDENTITY domain on that IOMMU.
> >>
> >> The VFIO no-IOMMU mode is equivalent to Linux UIO hence having
> >> IDENTITY domain for VFIO no-IOMMU is not appropriate here.
> >
> > Can you elaborate on this please? I don't understand the argument you're
> > making. It's like saying "I don't like eggs, therefore I don't drive a
> > car".
> >
>
> Like I said, VFIO no-IOMMU mode for a device means device transactions
> will not go through any IOMMU. That's why having IDENTITY domain for
> device using VFIO no-IOMMU is not semantically correct. The analogy you
> proposed does not apply here.
If you have a master that is wired through an IOMMU, then its transactions
go through that IOMMU and you can't avoid that. What you want is a way for
the IOMMU driver to make the IOMMU appear transparent to the device. That is
achieved by creating an IDENTITY domain, which sounds perfect for what
you're trying to do.
Will
On 20/07/17 10:10, Will Deacon wrote:
> On Thu, Jul 20, 2017 at 09:32:00AM +0530, Anup Patel wrote:
>> On Wed, Jul 19, 2017 at 5:23 PM, Will Deacon <[email protected]> wrote:
>>> There are two things here:
>>>
>>> 1. iommu_present() is pretty useless, because it applies to a "bus" which
>>> doesn't actually tell you what you need to know for things like the
>>> platform_bus, where some masters might be upstream of an SMMU and
>>> others might not be.
>>
>> I agree with you. The iommu_present() check in vfio_iommu_group_get()
>> is not much useful. We only reach line which checks iommu_present()
>> when iommu_group_get() returns NULL for given "struct device *". If there
>> is no IOMMU group for a "struct device *" then it means there is no IOMMU
>> HW doing translations for such device.
>>
>> If we drop the iommu_present() check (due to above reasons) in
>> vfio_iommu_group_get() then we don't require the IOMMU_CAP_BYPASS
>> and we can happily drop PATCH1, PATCH2, and PATCH3.
>>
>> I will remove the iommu_present() check in vfio_iommu_group_get()
>> because it is only comes into actions when VFIO_NOIOMMU is
>> enabled. This will also help us drop PATCH1-to-PATCH3.
>
> I don't think that's the right answer. Whilst iommu_present has obvious
> shortcomings, its intention is clear: it should tell you whether a given
> *device* is upstream of an IOMMU. So the right fix is to make this
> per-device, instead of per-bus. Removing it altogether is worse than leaving
> it like it is.
Not really - if there is an IOMMU up and running to the point of setting
bus ops, every device it cares about can be expected to have a group
already (there are only a couple of drivers left that don't use groups,
and they're hardly relevant to VFIO). Thus iommu_group_get() already is
the de-facto per-device IOMMU check.
And having looked into it, I'm now spinning a couple of patches to
finish off making groups truly mandatory so that that can be less
de-facto ;)
Robin.
>>> 2. If a master *is* upstream of an IOMMU and you want to use no-IOMMU,
>>> then the VFIO no-IOMMU code needs to be extended so that it creates
>>> an IDENTITY domain on that IOMMU.
>>
>> The VFIO no-IOMMU mode is equivalent to Linux UIO hence having
>> IDENTITY domain for VFIO no-IOMMU is not appropriate here.
>
> Can you elaborate on this please? I don't understand the argument you're
> making. It's like saying "I don't like eggs, therefore I don't drive a
> car".
>
> Will
>
On Thu, 20 Jul 2017 12:17:12 +0100
Robin Murphy <[email protected]> wrote:
> On 20/07/17 10:10, Will Deacon wrote:
> > On Thu, Jul 20, 2017 at 09:32:00AM +0530, Anup Patel wrote:
> >> On Wed, Jul 19, 2017 at 5:23 PM, Will Deacon <[email protected]> wrote:
> >>> There are two things here:
> >>>
> >>> 1. iommu_present() is pretty useless, because it applies to a "bus" which
> >>> doesn't actually tell you what you need to know for things like the
> >>> platform_bus, where some masters might be upstream of an SMMU and
> >>> others might not be.
> >>
> >> I agree with you. The iommu_present() check in vfio_iommu_group_get()
> >> is not much useful. We only reach line which checks iommu_present()
> >> when iommu_group_get() returns NULL for given "struct device *". If there
> >> is no IOMMU group for a "struct device *" then it means there is no IOMMU
> >> HW doing translations for such device.
> >>
> >> If we drop the iommu_present() check (due to above reasons) in
> >> vfio_iommu_group_get() then we don't require the IOMMU_CAP_BYPASS
> >> and we can happily drop PATCH1, PATCH2, and PATCH3.
> >>
> >> I will remove the iommu_present() check in vfio_iommu_group_get()
> >> because it is only comes into actions when VFIO_NOIOMMU is
> >> enabled. This will also help us drop PATCH1-to-PATCH3.
> >
> > I don't think that's the right answer. Whilst iommu_present has obvious
> > shortcomings, its intention is clear: it should tell you whether a given
> > *device* is upstream of an IOMMU. So the right fix is to make this
> > per-device, instead of per-bus. Removing it altogether is worse than leaving
> > it like it is.
>
> Not really - if there is an IOMMU up and running to the point of setting
> bus ops, every device it cares about can be expected to have a group
> already (there are only a couple of drivers left that don't use groups,
> and they're hardly relevant to VFIO). Thus iommu_group_get() already is
> the de-facto per-device IOMMU check.
>
> And having looked into it, I'm now spinning a couple of patches to
> finish off making groups truly mandatory so that that can be less
> de-facto ;)
No, look at vfio-noiommu and even vfio-mdev devices for devices which
have an iommu group but there is no physical iommu supporting them.
iommu_present() is how we can distinguish these groups and therefore
not generate a segfault in trying to use the full IOMMU API on them.
Thanks,
Alex
On 24/07/17 18:16, Alex Williamson wrote:
> On Thu, 20 Jul 2017 12:17:12 +0100
> Robin Murphy <[email protected]> wrote:
>
>> On 20/07/17 10:10, Will Deacon wrote:
>>> On Thu, Jul 20, 2017 at 09:32:00AM +0530, Anup Patel wrote:
>>>> On Wed, Jul 19, 2017 at 5:23 PM, Will Deacon <[email protected]> wrote:
>>>>> There are two things here:
>>>>>
>>>>> 1. iommu_present() is pretty useless, because it applies to a "bus" which
>>>>> doesn't actually tell you what you need to know for things like the
>>>>> platform_bus, where some masters might be upstream of an SMMU and
>>>>> others might not be.
>>>>
>>>> I agree with you. The iommu_present() check in vfio_iommu_group_get()
>>>> is not much useful. We only reach line which checks iommu_present()
>>>> when iommu_group_get() returns NULL for given "struct device *". If there
>>>> is no IOMMU group for a "struct device *" then it means there is no IOMMU
>>>> HW doing translations for such device.
>>>>
>>>> If we drop the iommu_present() check (due to above reasons) in
>>>> vfio_iommu_group_get() then we don't require the IOMMU_CAP_BYPASS
>>>> and we can happily drop PATCH1, PATCH2, and PATCH3.
>>>>
>>>> I will remove the iommu_present() check in vfio_iommu_group_get()
>>>> because it is only comes into actions when VFIO_NOIOMMU is
>>>> enabled. This will also help us drop PATCH1-to-PATCH3.
>>>
>>> I don't think that's the right answer. Whilst iommu_present has obvious
>>> shortcomings, its intention is clear: it should tell you whether a given
>>> *device* is upstream of an IOMMU. So the right fix is to make this
>>> per-device, instead of per-bus. Removing it altogether is worse than leaving
>>> it like it is.
>>
>> Not really - if there is an IOMMU up and running to the point of setting
>> bus ops, every device it cares about can be expected to have a group
>> already (there are only a couple of drivers left that don't use groups,
>> and they're hardly relevant to VFIO). Thus iommu_group_get() already is
>> the de-facto per-device IOMMU check.
>>
>> And having looked into it, I'm now spinning a couple of patches to
>> finish off making groups truly mandatory so that that can be less
>> de-facto ;)
>
> No, look at vfio-noiommu and even vfio-mdev devices for devices which
> have an iommu group but there is no physical iommu supporting them.
> iommu_present() is how we can distinguish these groups and therefore
> not generate a segfault in trying to use the full IOMMU API on them.
OK, so that means that the combination of vfio-noiommu and vfio-platform
is simply unusable, because iommu_present(&platform_bus_type) can give
such dangerous false positives too.
Robin.
On Mon, 24 Jul 2017 18:23:20 +0100
Robin Murphy <[email protected]> wrote:
> On 24/07/17 18:16, Alex Williamson wrote:
> > On Thu, 20 Jul 2017 12:17:12 +0100
> > Robin Murphy <[email protected]> wrote:
> >
> >> On 20/07/17 10:10, Will Deacon wrote:
> >>> On Thu, Jul 20, 2017 at 09:32:00AM +0530, Anup Patel wrote:
> >>>> On Wed, Jul 19, 2017 at 5:23 PM, Will Deacon <[email protected]> wrote:
> >>>>> There are two things here:
> >>>>>
> >>>>> 1. iommu_present() is pretty useless, because it applies to a "bus" which
> >>>>> doesn't actually tell you what you need to know for things like the
> >>>>> platform_bus, where some masters might be upstream of an SMMU and
> >>>>> others might not be.
> >>>>
> >>>> I agree with you. The iommu_present() check in vfio_iommu_group_get()
> >>>> is not much useful. We only reach line which checks iommu_present()
> >>>> when iommu_group_get() returns NULL for given "struct device *". If there
> >>>> is no IOMMU group for a "struct device *" then it means there is no IOMMU
> >>>> HW doing translations for such device.
> >>>>
> >>>> If we drop the iommu_present() check (due to above reasons) in
> >>>> vfio_iommu_group_get() then we don't require the IOMMU_CAP_BYPASS
> >>>> and we can happily drop PATCH1, PATCH2, and PATCH3.
> >>>>
> >>>> I will remove the iommu_present() check in vfio_iommu_group_get()
> >>>> because it is only comes into actions when VFIO_NOIOMMU is
> >>>> enabled. This will also help us drop PATCH1-to-PATCH3.
> >>>
> >>> I don't think that's the right answer. Whilst iommu_present has obvious
> >>> shortcomings, its intention is clear: it should tell you whether a given
> >>> *device* is upstream of an IOMMU. So the right fix is to make this
> >>> per-device, instead of per-bus. Removing it altogether is worse than leaving
> >>> it like it is.
> >>
> >> Not really - if there is an IOMMU up and running to the point of setting
> >> bus ops, every device it cares about can be expected to have a group
> >> already (there are only a couple of drivers left that don't use groups,
> >> and they're hardly relevant to VFIO). Thus iommu_group_get() already is
> >> the de-facto per-device IOMMU check.
> >>
> >> And having looked into it, I'm now spinning a couple of patches to
> >> finish off making groups truly mandatory so that that can be less
> >> de-facto ;)
> >
> > No, look at vfio-noiommu and even vfio-mdev devices for devices which
> > have an iommu group but there is no physical iommu supporting them.
> > iommu_present() is how we can distinguish these groups and therefore
> > not generate a segfault in trying to use the full IOMMU API on them.
>
> OK, so that means that the combination of vfio-noiommu and vfio-platform
> is simply unusable, because iommu_present(&platform_bus_type) can give
> such dangerous false positives too.
Yep, this kinda falls apart since platform_bus_type doesn't really map
to a physical bus, nor does the presence of a group canonically
demonstrate that an iommu is present since anyone can create a group
for a device. We really do need to migrate to per-device iommu_ops.
Thanks,
Alex
On Tue, Jul 25, 2017 at 12:36 AM, Alex Williamson
<[email protected]> wrote:
> On Mon, 24 Jul 2017 18:23:20 +0100
> Robin Murphy <[email protected]> wrote:
>
>> On 24/07/17 18:16, Alex Williamson wrote:
>> > On Thu, 20 Jul 2017 12:17:12 +0100
>> > Robin Murphy <[email protected]> wrote:
>> >
>> >> On 20/07/17 10:10, Will Deacon wrote:
>> >>> On Thu, Jul 20, 2017 at 09:32:00AM +0530, Anup Patel wrote:
>> >>>> On Wed, Jul 19, 2017 at 5:23 PM, Will Deacon <[email protected]> wrote:
>> >>>>> There are two things here:
>> >>>>>
>> >>>>> 1. iommu_present() is pretty useless, because it applies to a "bus" which
>> >>>>> doesn't actually tell you what you need to know for things like the
>> >>>>> platform_bus, where some masters might be upstream of an SMMU and
>> >>>>> others might not be.
>> >>>>
>> >>>> I agree with you. The iommu_present() check in vfio_iommu_group_get()
>> >>>> is not much useful. We only reach line which checks iommu_present()
>> >>>> when iommu_group_get() returns NULL for given "struct device *". If there
>> >>>> is no IOMMU group for a "struct device *" then it means there is no IOMMU
>> >>>> HW doing translations for such device.
>> >>>>
>> >>>> If we drop the iommu_present() check (due to above reasons) in
>> >>>> vfio_iommu_group_get() then we don't require the IOMMU_CAP_BYPASS
>> >>>> and we can happily drop PATCH1, PATCH2, and PATCH3.
>> >>>>
>> >>>> I will remove the iommu_present() check in vfio_iommu_group_get()
>> >>>> because it is only comes into actions when VFIO_NOIOMMU is
>> >>>> enabled. This will also help us drop PATCH1-to-PATCH3.
>> >>>
>> >>> I don't think that's the right answer. Whilst iommu_present has obvious
>> >>> shortcomings, its intention is clear: it should tell you whether a given
>> >>> *device* is upstream of an IOMMU. So the right fix is to make this
>> >>> per-device, instead of per-bus. Removing it altogether is worse than leaving
>> >>> it like it is.
>> >>
>> >> Not really - if there is an IOMMU up and running to the point of setting
>> >> bus ops, every device it cares about can be expected to have a group
>> >> already (there are only a couple of drivers left that don't use groups,
>> >> and they're hardly relevant to VFIO). Thus iommu_group_get() already is
>> >> the de-facto per-device IOMMU check.
>> >>
>> >> And having looked into it, I'm now spinning a couple of patches to
>> >> finish off making groups truly mandatory so that that can be less
>> >> de-facto ;)
>> >
>> > No, look at vfio-noiommu and even vfio-mdev devices for devices which
>> > have an iommu group but there is no physical iommu supporting them.
>> > iommu_present() is how we can distinguish these groups and therefore
>> > not generate a segfault in trying to use the full IOMMU API on them.
>>
>> OK, so that means that the combination of vfio-noiommu and vfio-platform
>> is simply unusable, because iommu_present(&platform_bus_type) can give
>> such dangerous false positives too.
>
> Yep, this kinda falls apart since platform_bus_type doesn't really map
> to a physical bus, nor does the presence of a group canonically
> demonstrate that an iommu is present since anyone can create a group
> for a device. We really do need to migrate to per-device iommu_ops.
> Thanks,
Yes, per-device iommu_ops will make things much cleaner. That's why
I have dropped VFIO no-IOMMU and IOMMU_CAP_BYPASS related
patches.
Can you please have a look at FlexRM platform reset driver?
Regards,
Anup