Re: [RFC PATCH 3/7] vfio: add spimdev support
On Mon, 6 Aug 2018 09:34:28 -0700 "Raj, Ashok" wrote: > On Mon, Aug 06, 2018 at 09:49:40AM -0600, Alex Williamson wrote: > > On Mon, 6 Aug 2018 09:40:04 +0800 > > Kenneth Lee wrote: > > > > > > 1. It supports thousands of processes. Take zip accelerator as an > > > example, any > > > application need data compression/decompression will need to interact > > > with the > > > accelerator. To support that, you have to create tens of thousands of > > > mdev for > > > their usage. I don't think it is a good idea to have so many devices in > > > the > > > system. > > > > Each mdev is a device, regardless of whether there are hardware > > resources committed to the device, so I don't understand this argument. > > > > > 2. The application does not want to own the mdev for long. It just need an > > > access point for the hardware service. If it has to interact with an > > > management > > > agent for allocation and release, this makes the problem complex. > > > > I don't see how the length of the usage plays a role here either. Are > > you concerned that the time it takes to create and remove an mdev is > > significant compared to the usage time? Userspace is certainly welcome > > to create a pool of devices, but why should it be the kernel's > > responsibility to dynamically assign resources to an mdev? What's the > > usage model when resources are unavailable? It seems there's > > complexity in either case, but it's generally userspace's responsibility > > to impose a policy. > > > > Can vfio dev's created representing an mdev be shared between several > processes? It doesn't need to be exclusive. > > The path to hardware is established by the processes binding to SVM and > IOMMU ensuring that the PASID is plummed properly. One can think the > same hardware is shared between several processes, hardware knows the > isolation is via the PASID. > > For these cases it isn't required to create a dev per process. The iommu group is the unit of ownership, a vfio group mirrors an iommu group, therefore a vfio group only allows a single open(2). A group also represents the minimum isolation set of devices, therefore devices within a group are not considered isolated and must share the same address space represented by the vfio container. Beyond that, it is possible to share devices among processes, but (I think) it generally implies a hierarchical rather than peer relationship between processes. Thanks, Alex ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH 3/7] vfio: add spimdev support
On Mon, Aug 06, 2018 at 09:49:40AM -0600, Alex Williamson wrote: > On Mon, 6 Aug 2018 09:40:04 +0800 > Kenneth Lee wrote: > > > > 1. It supports thousands of processes. Take zip accelerator as an example, > > any > > application need data compression/decompression will need to interact with > > the > > accelerator. To support that, you have to create tens of thousands of mdev > > for > > their usage. I don't think it is a good idea to have so many devices in the > > system. > > Each mdev is a device, regardless of whether there are hardware > resources committed to the device, so I don't understand this argument. > > > 2. The application does not want to own the mdev for long. It just need an > > access point for the hardware service. If it has to interact with an > > management > > agent for allocation and release, this makes the problem complex. > > I don't see how the length of the usage plays a role here either. Are > you concerned that the time it takes to create and remove an mdev is > significant compared to the usage time? Userspace is certainly welcome > to create a pool of devices, but why should it be the kernel's > responsibility to dynamically assign resources to an mdev? What's the > usage model when resources are unavailable? It seems there's > complexity in either case, but it's generally userspace's responsibility > to impose a policy. > Can vfio dev's created representing an mdev be shared between several processes? It doesn't need to be exclusive. The path to hardware is established by the processes binding to SVM and IOMMU ensuring that the PASID is plummed properly. One can think the same hardware is shared between several processes, hardware knows the isolation is via the PASID. For these cases it isn't required to create a dev per process. Cheers, Ashok ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH 0/7] A General Accelerator Framework, WarpDrive
On Mon, Aug 06, 2018 at 11:12:52AM +0800, Kenneth Lee wrote: > On Fri, Aug 03, 2018 at 10:39:44AM -0400, Jerome Glisse wrote: > > On Fri, Aug 03, 2018 at 11:47:21AM +0800, Kenneth Lee wrote: > > > On Thu, Aug 02, 2018 at 10:22:43AM -0400, Jerome Glisse wrote: > > > > On Thu, Aug 02, 2018 at 12:05:57PM +0800, Kenneth Lee wrote: > > > > > On Thu, Aug 02, 2018 at 02:33:12AM +, Tian, Kevin wrote: > > > > > > > On Wed, Aug 01, 2018 at 06:22:14PM +0800, Kenneth Lee wrote: > > > > > > > > [...] > > > > > But doorbell is just a notification. Except for DOS (to make hardware > > > > > busy) it > > > > > cannot actually take or change anything from the kernel space. And > > > > > the DOS > > > > > problem can be always taken as the problem that a group of processes > > > > > share the > > > > > same kernel entity. > > > > > > > > > > In the coming HIP09 hardware, the doorbell will come with a random > > > > > number so > > > > > only the process who allocated the queue can knock it correctly. > > > > > > > > When doorbell is ring the hardware start fetching commands from > > > > the queue and execute them ? If so than a rogue process B might > > > > ring the doorbell of process A which would starts execution of > > > > random commands (ie whatever random memory value there is left > > > > inside the command buffer memory, could be old commands i guess). > > > > > > > > If this is not how this doorbell works then, yes it can only do > > > > a denial of service i guess. Issue i have with doorbell is that > > > > i have seen 10 differents implementations in 10 differents hw > > > > and each are different as to what ringing or value written to the > > > > doorbell does. It is painfull to track what is what for each hw. > > > > > > > > > > In our implementation, doorbell is simply a notification, just like an > > > interrupt > > > to the accelerator. The command is all about what's in the queue. > > > > > > I agree that there is no simple and standard way to track the shared IO > > > space. > > > But I think we have to trust the driver in some way. If the driver is > > > malicious, > > > even a simple ioctl can become an attack. > > > > Trusting kernel space driver is fine, trusting user space driver is > > not in my view. AFAICT every driver developer so far always made > > sure that someone could not abuse its device to do harmfull thing to > > other process. > > > > Fully agree. That is why this driver shares only the doorbell space. There is > only the doorbell is shared in the whole page, nothing else. > > Maybe you are concerning the user driver will give malicious command to the > hardware? But these commands cannot influence the other process. If we can > trust > the hardware design, the process cannot do any harm. My questions was what happens if a process B ring the doorbell of process A. On some hardware the value written in the doorbell is use as an index in command buffer. On other it just wakeup the hardware to go look at a structure private to the process. They are other variations of those themes. If it is the former ie the value is use to advance in the command buffer then a rogue process can force another process to advance its command buffer and what is in the command buffer can be some random old memory values which can be more harmfull than just Denial Of Service. > > > > > > > My more general question is do we want to grow VFIO to become > > > > > > > a more generic device driver API. This patchset adds a command > > > > > > > queue concept to it (i don't think it exist today but i have > > > > > > > not follow VFIO closely). > > > > > > > > > > > > > > > > > The thing is, VFIO is the only place to support DMA from user land. > > > > > If we don't > > > > > put it here, we have to create another similar facility to support > > > > > the same. > > > > > > > > No it is not, network device, GPU, block device, ... they all do > > > > support DMA. The point i am trying to make here is that even in > > > > > > Sorry, wait a minute, are we talking the same thing? I meant "DMA from > > > user > > > land", not "DMA from kernel driver". To do that we have to manipulate the > > > IOMMU(Unit). I think it can only be done by default_domain or vfio > > > domain. Or > > > the user space have to directly access the IOMMU. > > > > GPU do DMA in the sense that you pass to the kernel a valid > > virtual address (kernel driver do all the proper check) and > > then you can use the GPU to copy from or to that range of > > virtual address. Exactly how you want to use this compression > > engine. It does not rely on SVM but SVM going forward would > > still be the prefered option. > > > > No, SVM is not the reason why we rely on Jean's SVM(SVA) series. We rely on > Jean's series because of multi-process (PASID or substream ID) support. > > But of couse, WarpDrive can still benefit from the SVM feature. We are getting side tracked here. PASID/ID do not require VFIO. > > > > your
[PATCH] iommu/omap: Fix cache flushes on L2 table entries
The base address used for DMA operations on the second-level table
did incorrectly include the offset for the table entry. The offset
was then added again which lead to incorrect behavior.
Operations on the L1 table are not affected.
The calculation of the base address is changed to point to the
beginning of the L2 table.
Fixes: bfee0cf0ee1d ("iommu/omap: Use DMA-API for performing cache flushes")
Acked-by: Suman Anna
Signed-off-by: Ralf Goebel
---
Changes in v2: Rephrase patch summary and description
drivers/iommu/omap-iommu.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index af4a8e7fcd27..3b05117118c3 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -550,7 +550,7 @@ static u32 *iopte_alloc(struct omap_iommu *obj, u32 *iopgd,
pte_ready:
iopte = iopte_offset(iopgd, da);
- *pt_dma = virt_to_phys(iopte);
+ *pt_dma = iopgd_page_paddr(iopgd);
dev_vdbg(obj->dev,
"%s: da:%08x pgd:%p *pgd:%08x pte:%p *pte:%08x\n",
__func__, da, iopgd, *iopgd, iopte, *iopte);
@@ -738,7 +738,7 @@ static size_t iopgtable_clear_entry_core(struct omap_iommu
*obj, u32 da)
}
bytes *= nent;
memset(iopte, 0, nent * sizeof(*iopte));
- pt_dma = virt_to_phys(iopte);
+ pt_dma = iopgd_page_paddr(iopgd);
flush_iopte_range(obj->dev, pt_dma, pt_offset, nent);
/*
--
2.11.0
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/rockchip: Handle errors returned from PM framework
Hi Marc,
Am Sonntag, 5. August 2018, 14:46:16 CEST schrieb Marc Zyngier:
> pm_runtime_get_if_in_use can fail: either PM has been disabled
> altogether (-EINVAL), or the device hasn't been enabled yet (0).
> Sadly, we ignore the first case at the moment, and end up treating
> it as if we had received a valid interrupt.
>
> The first case could happen because the interrupt line is shared
> (with the VOP for example), and although that device hasn't been
> programmed yet, an interrupt may be pending (think kexec/kdump).
>
> The second case means that we've enabled the IOMMU, but it is
> not powered-on just yet. This could be another instance of the
> above, but as it deserves investigation, let's output a single
> warning (instead of flodding the console).
>
> In both cases, bail with an IRQ_NONE.
>
> Fixes: 0f181d3cf7d98 ("iommu/rockchip: Add runtime PM support")
> Signed-off-by: Marc Zyngier
Hmm, maybe my thinking is flawed, but to me it looks a bit different.
I.e. the iommu, as well as the vop have the capability to check if the irq
is for them via their status registers (INT_STATUS and MMU_STATUS).
For this to happen the power-domain must be active and the device clocked.
Clock handling is done on both the vop and iommu and in the !CONFIG_PM
case all power-domains are left on.
Right now a !CONFIG_PM just passes through the driver unnoticed but with
this change, we would actually make it depend on PM?
What am I missing?
Also there are quite a bit more instances of pm_runtime_get_if_in_use
present in the iommu driver.
Heiko
> drivers/iommu/rockchip-iommu.c | 7 ---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
> index 054cd2c8e9c8..9d21495a8433 100644
> --- a/drivers/iommu/rockchip-iommu.c
> +++ b/drivers/iommu/rockchip-iommu.c
> @@ -521,10 +521,11 @@ static irqreturn_t rk_iommu_irq(int irq, void *dev_id)
> u32 int_status;
> dma_addr_t iova;
> irqreturn_t ret = IRQ_NONE;
> - int i;
> + int i, err;
>
> - if (WARN_ON(!pm_runtime_get_if_in_use(iommu->dev)))
> - return 0;
> + err = pm_runtime_get_if_in_use(iommu->dev);
> + if ((err < 0) || WARN_ON_ONCE(!err))
> + return ret;
>
> if (WARN_ON(clk_bulk_enable(iommu->num_clocks, iommu->clocks)))
> goto out;
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/rockchip: Handle errors returned from PM framework
Hi Heiko,
On 06/08/18 13:09, Heiko Stuebner wrote:
> Hi Marc,
>
> Am Sonntag, 5. August 2018, 14:46:16 CEST schrieb Marc Zyngier:
>> pm_runtime_get_if_in_use can fail: either PM has been disabled
>> altogether (-EINVAL), or the device hasn't been enabled yet (0).
>> Sadly, we ignore the first case at the moment, and end up treating
>> it as if we had received a valid interrupt.
>>
>> The first case could happen because the interrupt line is shared
>> (with the VOP for example), and although that device hasn't been
>> programmed yet, an interrupt may be pending (think kexec/kdump).
>>
>> The second case means that we've enabled the IOMMU, but it is
>> not powered-on just yet. This could be another instance of the
>> above, but as it deserves investigation, let's output a single
>> warning (instead of flodding the console).
>>
>> In both cases, bail with an IRQ_NONE.
>>
>> Fixes: 0f181d3cf7d98 ("iommu/rockchip: Add runtime PM support")
>> Signed-off-by: Marc Zyngier
>
> Hmm, maybe my thinking is flawed, but to me it looks a bit different.
>
> I.e. the iommu, as well as the vop have the capability to check if the irq
> is for them via their status registers (INT_STATUS and MMU_STATUS).
>
> For this to happen the power-domain must be active and the device clocked.
> Clock handling is done on both the vop and iommu and in the !CONFIG_PM
> case all power-domains are left on.
>
> Right now a !CONFIG_PM just passes through the driver unnoticed but with
> this change, we would actually make it depend on PM?
>
> What am I missing?
I'm not sure you're missing anything, except that what I'm reporting
here has nothing to do with !CONFIG_PM, but with the state of runtime PM
when an interrupt gets delivered.
> Also there are quite a bit more instances of pm_runtime_get_if_in_use
> present in the iommu driver.
Indeed. Most of the are mishandling the return value by ignoring the
error cases. And the more I look at it, the more I think I'm just fixing
an effect, and not the root cause. See when the IOMMU interrupts get
requested, long before pm_runtime_enable() is called.
If you get an interrupt at that point (which is perfectly likely given
that the VOP shares the IRQ line), you handle it in an unexpected
context. I should have spotted that yesterday...
I've quickly hacked the (untested) following patch, which should do the
trick (I'll test it tonight).
Thanks,
M.
diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index 054cd2c8e9c8..94942712959d 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -1152,17 +1152,6 @@ static int rk_iommu_probe(struct platform_device *pdev)
if (iommu->num_mmu == 0)
return PTR_ERR(iommu->bases[0]);
- i = 0;
- while ((irq = platform_get_irq(pdev, i++)) != -ENXIO) {
- if (irq < 0)
- return irq;
-
- err = devm_request_irq(iommu->dev, irq, rk_iommu_irq,
- IRQF_SHARED, dev_name(dev), iommu);
- if (err)
- return err;
- }
-
iommu->reset_disabled = device_property_read_bool(dev,
"rockchip,disable-mmu-reset");
@@ -1219,6 +1208,19 @@ static int rk_iommu_probe(struct platform_device *pdev)
pm_runtime_enable(dev);
+ i = 0;
+ while ((irq = platform_get_irq(pdev, i++)) != -ENXIO) {
+ if (irq < 0)
+ return irq;
+
+ err = devm_request_irq(iommu->dev, irq, rk_iommu_irq,
+ IRQF_SHARED, dev_name(dev), iommu);
+ if (err) {
+ pm_runtime_disable(dev);
+ goto err_remove_sysfs;
+ }
+ }
+
return 0;
err_remove_sysfs:
iommu_device_sysfs_remove(>iommu);
--
Jazz is not dead. It just smells funny...
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 1/1] iommu/arm-smmu-v3: fix unexpected CMD_SYNC timeout
The condition "(int)(VAL - sync_idx) >= 0" to break loop in function
__arm_smmu_sync_poll_msi requires that sync_idx must be increased
monotonously according to the sequence of the CMDs in the cmdq.
But ".msidata = atomic_inc_return_relaxed(>sync_nr)" is not protected
by spinlock, so the following scenarios may appear:
cpu0cpu1
msidata=0
msidata=1
insert cmd1
insert cmd0
smmu execute cmd1
smmu execute cmd0
poll timeout, because msidata=1 is overridden by
cmd0, that means VAL=0, sync_idx=1.
Signed-off-by: Zhen Lei
---
drivers/iommu/arm-smmu-v3.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 1d64710..4810f61 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -566,7 +566,7 @@ struct arm_smmu_device {
int gerr_irq;
int combined_irq;
- atomic_tsync_nr;
+ u32 sync_nr;
unsigned long ias; /* IPA */
unsigned long oas; /* PA */
@@ -836,7 +836,6 @@ static int arm_smmu_cmdq_build_cmd(u64 *cmd, struct
arm_smmu_cmdq_ent *ent)
cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_CS,
CMDQ_SYNC_0_CS_SEV);
cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_MSH, ARM_SMMU_SH_ISH);
cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_MSIATTR,
ARM_SMMU_MEMATTR_OIWB);
- cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_MSIDATA, ent->sync.msidata);
cmd[1] |= ent->sync.msiaddr & CMDQ_SYNC_1_MSIADDR_MASK;
break;
default:
@@ -947,7 +946,6 @@ static int __arm_smmu_cmdq_issue_sync_msi(struct
arm_smmu_device *smmu)
struct arm_smmu_cmdq_ent ent = {
.opcode = CMDQ_OP_CMD_SYNC,
.sync = {
- .msidata = atomic_inc_return_relaxed(>sync_nr),
.msiaddr = virt_to_phys(>sync_count),
},
};
@@ -955,6 +953,8 @@ static int __arm_smmu_cmdq_issue_sync_msi(struct
arm_smmu_device *smmu)
arm_smmu_cmdq_build_cmd(cmd, );
spin_lock_irqsave(>cmdq.lock, flags);
+ ent.sync.msidata = ++smmu->sync_nr;
+ cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_MSIDATA, ent.sync.msidata);
arm_smmu_cmdq_insert_cmd(smmu, cmd);
spin_unlock_irqrestore(>cmdq.lock, flags);
@@ -2179,7 +2179,6 @@ static int arm_smmu_init_structures(struct
arm_smmu_device *smmu)
{
int ret;
- atomic_set(>sync_nr, 0);
ret = arm_smmu_init_queues(smmu);
if (ret)
return ret;
--
1.8.3
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v4 3/5] iommu/io-pgtable-arm: add support for non-strict mode
To support the non-strict mode, now we only tlbi and sync for the strict
mode. But for the non-leaf case, always follow strict mode.
Signed-off-by: Zhen Lei
---
drivers/iommu/io-pgtable-arm.c | 27 ++-
drivers/iommu/io-pgtable.h | 3 +++
2 files changed, 21 insertions(+), 9 deletions(-)
diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
index 010a254..bb61bef 100644
--- a/drivers/iommu/io-pgtable-arm.c
+++ b/drivers/iommu/io-pgtable-arm.c
@@ -292,7 +292,7 @@ static void __arm_lpae_set_pte(arm_lpae_iopte *ptep,
arm_lpae_iopte pte,
static size_t __arm_lpae_unmap(struct arm_lpae_io_pgtable *data,
unsigned long iova, size_t size, int lvl,
- arm_lpae_iopte *ptep);
+ arm_lpae_iopte *ptep, bool strict);
static void __arm_lpae_init_pte(struct arm_lpae_io_pgtable *data,
phys_addr_t paddr, arm_lpae_iopte prot,
@@ -319,6 +319,7 @@ static int arm_lpae_init_pte(struct arm_lpae_io_pgtable
*data,
arm_lpae_iopte prot, int lvl,
arm_lpae_iopte *ptep)
{
+ size_t unmapped;
arm_lpae_iopte pte = *ptep;
if (iopte_leaf(pte, lvl)) {
@@ -334,7 +335,8 @@ static int arm_lpae_init_pte(struct arm_lpae_io_pgtable
*data,
size_t sz = ARM_LPAE_BLOCK_SIZE(lvl, data);
tblp = ptep - ARM_LPAE_LVL_IDX(iova, lvl, data);
- if (WARN_ON(__arm_lpae_unmap(data, iova, sz, lvl, tblp) != sz))
+ unmapped = __arm_lpae_unmap(data, iova, sz, lvl, tblp, true);
+ if (WARN_ON(unmapped != sz))
return -EINVAL;
}
@@ -576,15 +578,17 @@ static size_t arm_lpae_split_blk_unmap(struct
arm_lpae_io_pgtable *data,
}
if (unmap_idx < 0)
- return __arm_lpae_unmap(data, iova, size, lvl, tablep);
+ return __arm_lpae_unmap(data, iova, size, lvl, tablep, true);
io_pgtable_tlb_add_flush(>iop, iova, size, size, true);
+ io_pgtable_tlb_sync(>iop);
+
return size;
}
static size_t __arm_lpae_unmap(struct arm_lpae_io_pgtable *data,
unsigned long iova, size_t size, int lvl,
- arm_lpae_iopte *ptep)
+ arm_lpae_iopte *ptep, bool strict)
{
arm_lpae_iopte pte;
struct io_pgtable *iop = >iop;
@@ -609,7 +613,7 @@ static size_t __arm_lpae_unmap(struct arm_lpae_io_pgtable
*data,
io_pgtable_tlb_sync(iop);
ptep = iopte_deref(pte, data);
__arm_lpae_free_pgtable(data, lvl + 1, ptep);
- } else {
+ } else if (strict) {
io_pgtable_tlb_add_flush(iop, iova, size, size, true);
}
@@ -625,12 +629,13 @@ static size_t __arm_lpae_unmap(struct arm_lpae_io_pgtable
*data,
/* Keep on walkin' */
ptep = iopte_deref(pte, data);
- return __arm_lpae_unmap(data, iova, size, lvl + 1, ptep);
+ return __arm_lpae_unmap(data, iova, size, lvl + 1, ptep, strict);
}
static size_t arm_lpae_unmap(struct io_pgtable_ops *ops, unsigned long iova,
size_t size)
{
+ bool strict;
struct arm_lpae_io_pgtable *data = io_pgtable_ops_to_data(ops);
arm_lpae_iopte *ptep = data->pgd;
int lvl = ARM_LPAE_START_LVL(data);
@@ -638,7 +643,9 @@ static size_t arm_lpae_unmap(struct io_pgtable_ops *ops,
unsigned long iova,
if (WARN_ON(iova >= (1ULL << data->iop.cfg.ias)))
return 0;
- return __arm_lpae_unmap(data, iova, size, lvl, ptep);
+ strict = !(data->iop.cfg.quirks & IO_PGTABLE_QUIRK_NON_STRICT);
+
+ return __arm_lpae_unmap(data, iova, size, lvl, ptep, strict);
}
static phys_addr_t arm_lpae_iova_to_phys(struct io_pgtable_ops *ops,
@@ -771,7 +778,8 @@ static void arm_lpae_restrict_pgsizes(struct io_pgtable_cfg
*cfg)
u64 reg;
struct arm_lpae_io_pgtable *data;
- if (cfg->quirks & ~(IO_PGTABLE_QUIRK_ARM_NS | IO_PGTABLE_QUIRK_NO_DMA))
+ if (cfg->quirks & ~(IO_PGTABLE_QUIRK_ARM_NS | IO_PGTABLE_QUIRK_NO_DMA |
+ IO_PGTABLE_QUIRK_NON_STRICT))
return NULL;
data = arm_lpae_alloc_pgtable(cfg);
@@ -863,7 +871,8 @@ static void arm_lpae_restrict_pgsizes(struct io_pgtable_cfg
*cfg)
struct arm_lpae_io_pgtable *data;
/* The NS quirk doesn't apply at stage 2 */
- if (cfg->quirks & ~IO_PGTABLE_QUIRK_NO_DMA)
+ if (cfg->quirks & ~(IO_PGTABLE_QUIRK_NO_DMA |
+ IO_PGTABLE_QUIRK_NON_STRICT))
return NULL;
data = arm_lpae_alloc_pgtable(cfg);
diff --git a/drivers/iommu/io-pgtable.h b/drivers/iommu/io-pgtable.h
index 2df7909..beb14a3 100644
--- a/drivers/iommu/io-pgtable.h
+++ b/drivers/iommu/io-pgtable.h
@@
[PATCH v4 5/5] iommu/arm-smmu-v3: add bootup option "arm_iommu"
Add a bootup option to make the system manager can choose which mode to
be used. The default mode is strict.
Signed-off-by: Zhen Lei
---
Documentation/admin-guide/kernel-parameters.txt | 9 +
drivers/iommu/arm-smmu-v3.c | 17 -
2 files changed, 25 insertions(+), 1 deletion(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt
b/Documentation/admin-guide/kernel-parameters.txt
index 533ff5c..426e989 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1720,6 +1720,15 @@
nobypass[PPC/POWERNV]
Disable IOMMU bypass, using IOMMU for PCI devices.
+ arm_iommu= [ARM64]
+ non-strict [Default Off]
+ Put off TLBs invalidation and release memory first.
+ It's good for scatter-gather performance but lacks full
+ isolation, an untrusted device can access the reused
+ memory because the TLBs may still valid. Please take
+ full consideration before choosing this mode.
+ Note that, VFIO will always use strict mode.
+
iommu.passthrough=
[ARM64] Configure DMA to bypass the IOMMU by default.
Format: { "0" | "1" }
diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 904bc1e..9a30892 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -631,6 +631,21 @@ struct arm_smmu_option_prop {
{ 0, NULL},
};
+static u32 smmu_non_strict __read_mostly;
+
+static int __init arm_smmu_setup(char *str)
+{
+ if (!strncmp(str, "non-strict", 10)) {
+ smmu_non_strict = 1;
+ pr_warn("WARNING: iommu non-strict mode is chosen.\n"
+ "It's good for scatter-gather performance but lacks
full isolation\n");
+ add_taint(TAINT_WARN, LOCKDEP_STILL_OK);
+ }
+
+ return 0;
+}
+early_param("arm_iommu", arm_smmu_setup);
+
static inline void __iomem *arm_smmu_page1_fixup(unsigned long offset,
struct arm_smmu_device *smmu)
{
@@ -1622,7 +1637,7 @@ static int arm_smmu_domain_finalise(struct iommu_domain
*domain)
if (smmu->features & ARM_SMMU_FEAT_COHERENCY)
pgtbl_cfg.quirks = IO_PGTABLE_QUIRK_NO_DMA;
- if (domain->type == IOMMU_DOMAIN_DMA) {
+ if ((domain->type == IOMMU_DOMAIN_DMA) && smmu_non_strict) {
domain->non_strict = 1;
pgtbl_cfg.quirks |= IO_PGTABLE_QUIRK_NON_STRICT;
}
--
1.8.3
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v4 4/5] iommu/arm-smmu-v3: add support for non-strict mode
Dynamically choose strict or non-strict mode for page table config based
on the iommu domain type.
Signed-off-by: Zhen Lei
---
drivers/iommu/arm-smmu-v3.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 2f1304b..904bc1e 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -1622,6 +1622,11 @@ static int arm_smmu_domain_finalise(struct iommu_domain
*domain)
if (smmu->features & ARM_SMMU_FEAT_COHERENCY)
pgtbl_cfg.quirks = IO_PGTABLE_QUIRK_NO_DMA;
+ if (domain->type == IOMMU_DOMAIN_DMA) {
+ domain->non_strict = 1;
+ pgtbl_cfg.quirks |= IO_PGTABLE_QUIRK_NON_STRICT;
+ }
+
pgtbl_ops = alloc_io_pgtable_ops(fmt, _cfg, smmu_domain);
if (!pgtbl_ops)
return -ENOMEM;
@@ -1782,7 +1787,7 @@ static void arm_smmu_iotlb_sync(struct iommu_domain
*domain)
{
struct arm_smmu_device *smmu = to_smmu_domain(domain)->smmu;
- if (smmu)
+ if (smmu && !domain->non_strict)
__arm_smmu_tlb_sync(smmu);
}
--
1.8.3
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v4 2/5] iommu/dma: add support for non-strict mode
1. Save the related domain pointer in struct iommu_dma_cookie, make iovad
capable call domain->ops->flush_iotlb_all to flush TLB.
2. During the iommu domain initialization phase, base on domain->non_strict
field to check whether non-strict mode is supported or not. If so, call
init_iova_flush_queue to register iovad->flush_cb callback.
3. All unmap(contains iova-free) APIs will finally invoke __iommu_dma_unmap
-->iommu_dma_free_iova. If the domain is non-strict, call queue_iova to
put off iova freeing.
Signed-off-by: Zhen Lei
---
drivers/iommu/dma-iommu.c | 23 +++
drivers/iommu/iommu.c | 1 +
include/linux/iommu.h | 1 +
3 files changed, 25 insertions(+)
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index ddcbbdb..213e62a 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -55,6 +55,7 @@ struct iommu_dma_cookie {
};
struct list_headmsi_page_list;
spinlock_t msi_lock;
+ struct iommu_domain *domain;
};
static inline size_t cookie_msi_granule(struct iommu_dma_cookie *cookie)
@@ -257,6 +258,17 @@ static int iova_reserve_iommu_regions(struct device *dev,
return ret;
}
+static void iommu_dma_flush_iotlb_all(struct iova_domain *iovad)
+{
+ struct iommu_dma_cookie *cookie;
+ struct iommu_domain *domain;
+
+ cookie = container_of(iovad, struct iommu_dma_cookie, iovad);
+ domain = cookie->domain;
+
+ domain->ops->flush_iotlb_all(domain);
+}
+
/**
* iommu_dma_init_domain - Initialise a DMA mapping domain
* @domain: IOMMU domain previously prepared by iommu_get_dma_cookie()
@@ -308,6 +320,14 @@ int iommu_dma_init_domain(struct iommu_domain *domain,
dma_addr_t base,
}
init_iova_domain(iovad, 1UL << order, base_pfn);
+
+ if (domain->non_strict) {
+ BUG_ON(!domain->ops->flush_iotlb_all);
+
+ cookie->domain = domain;
+ init_iova_flush_queue(iovad, iommu_dma_flush_iotlb_all, NULL);
+ }
+
if (!dev)
return 0;
@@ -390,6 +410,9 @@ static void iommu_dma_free_iova(struct iommu_dma_cookie
*cookie,
/* The MSI case is only ever cleaning up its most recent allocation */
if (cookie->type == IOMMU_DMA_MSI_COOKIE)
cookie->msi_iova -= size;
+ else if (cookie->domain && cookie->domain->non_strict)
+ queue_iova(iovad, iova_pfn(iovad, iova),
+ size >> iova_shift(iovad), 0);
else
free_iova_fast(iovad, iova_pfn(iovad, iova),
size >> iova_shift(iovad));
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 63b3756..7811fde 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1263,6 +1263,7 @@ static struct iommu_domain *__iommu_domain_alloc(struct
bus_type *bus,
domain->ops = bus->iommu_ops;
domain->type = type;
+ domain->non_strict = 0;
/* Assume all sizes by default; the driver may override this later */
domain->pgsize_bitmap = bus->iommu_ops->pgsize_bitmap;
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 19938ee..0a0fb48 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -88,6 +88,7 @@ struct iommu_domain_geometry {
struct iommu_domain {
unsigned type;
+ int non_strict;
const struct iommu_ops *ops;
unsigned long pgsize_bitmap;/* Bitmap of page sizes in use */
iommu_fault_handler_t handler;
--
1.8.3
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v4 1/5] iommu/arm-smmu-v3: fix the implementation of flush_iotlb_all hook
.flush_iotlb_all can not just wait for previous tlbi operations to be
completed, but should also invalid all TLBs of the related domain.
Signed-off-by: Zhen Lei
---
drivers/iommu/arm-smmu-v3.c | 10 +-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 4810f61..2f1304b 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -1770,6 +1770,14 @@ static int arm_smmu_map(struct iommu_domain *domain,
unsigned long iova,
return ops->unmap(ops, iova, size);
}
+static void arm_smmu_flush_iotlb_all(struct iommu_domain *domain)
+{
+ struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
+
+ if (smmu_domain->smmu)
+ arm_smmu_tlb_inv_context(smmu_domain);
+}
+
static void arm_smmu_iotlb_sync(struct iommu_domain *domain)
{
struct arm_smmu_device *smmu = to_smmu_domain(domain)->smmu;
@@ -1998,7 +2006,7 @@ static void arm_smmu_put_resv_regions(struct device *dev,
.map= arm_smmu_map,
.unmap = arm_smmu_unmap,
.map_sg = default_iommu_map_sg,
- .flush_iotlb_all= arm_smmu_iotlb_sync,
+ .flush_iotlb_all= arm_smmu_flush_iotlb_all,
.iotlb_sync = arm_smmu_iotlb_sync,
.iova_to_phys = arm_smmu_iova_to_phys,
.add_device = arm_smmu_add_device,
--
1.8.3
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v4 0/5] add non-strict mode support for arm-smmu-v3
v3 -> v4: 1. Add a new member "non_strict" in struct iommu_domain to mark whether that domain use non-strict mode or not. This can help us to remove the capability which was added in prior version. 2. Add a new quirk IO_PGTABLE_QUIRK_NON_STRICT, so that we can get "strict mode" in io-pgtable-arm.c according to data->iop.cfg.quirks. 3. rename the new boot option to "arm_iommu". Thanks for Robin's review comments. v2 -> v3: Add a bootup option "iommu_strict_mode" to make the manager can choose which mode to be used. The first 5 patches have not changed. + iommu_strict_mode= [arm-smmu-v3] + 0 - strict mode (default) + 1 - non-strict mode v1 -> v2: Use the lowest bit of the io_pgtable_ops.unmap's iova parameter to pass the strict mode: 0, IOMMU_STRICT; 1, IOMMU_NON_STRICT; Treat 0 as IOMMU_STRICT, so that the unmap operation can compatible with other IOMMUs which still use strict mode. In other words, this patch series will not impact other IOMMU drivers. I tried add a new quirk IO_PGTABLE_QUIRK_NON_STRICT in io_pgtable_cfg.quirks, but it can not pass the strict mode of the domain from SMMUv3 driver to io-pgtable module. Add a new member domain_non_strict in struct iommu_dma_cookie, this member will only be initialized when the related domain and IOMMU driver support non-strict mode. v1: In common, a IOMMU unmap operation follow the below steps: 1. remove the mapping in page table of the specified iova range 2. execute tlbi command to invalid the mapping which is cached in TLB 3. wait for the above tlbi operation to be finished 4. free the IOVA resource 5. free the physical memory resource This maybe a problem when unmap is very frequently, the combination of tlbi and wait operation will consume a lot of time. A feasible method is put off tlbi and iova-free operation, when accumulating to a certain number or reaching a specified time, execute only one tlbi_all command to clean up TLB, then free the backup IOVAs. Mark as non-strict mode. But it must be noted that, although the mapping has already been removed in the page table, it maybe still exist in TLB. And the freed physical memory may also be reused for others. So a attacker can persistent access to memory based on the just freed IOVA, to obtain sensible data or corrupt memory. So the VFIO should always choose the strict mode. Some may consider put off physical memory free also, that will still follow strict mode. But for the map_sg cases, the memory allocation is not controlled by IOMMU APIs, so it is not enforceable. Fortunately, Intel and AMD have already applied the non-strict mode, and put queue_iova() operation into the common file dma-iommu.c., and my work is based on it. The difference is that arm-smmu-v3 driver will call IOMMU common APIs to unmap, but Intel and AMD IOMMU drivers are not. Below is the performance data of strict vs non-strict for NVMe device: Randomly Read IOPS: 146K(strict) vs 573K(non-strict) Randomly Write IOPS: 143K(strict) vs 513K(non-strict) Zhen Lei (5): iommu/arm-smmu-v3: fix the implementation of flush_iotlb_all hook iommu/dma: add support for non-strict mode iommu/io-pgtable-arm: add support for non-strict mode iommu/arm-smmu-v3: add support for non-strict mode iommu/arm-smmu-v3: add bootup option "arm_iommu" Documentation/admin-guide/kernel-parameters.txt | 9 +++ drivers/iommu/arm-smmu-v3.c | 32 +++-- drivers/iommu/dma-iommu.c | 23 ++ drivers/iommu/io-pgtable-arm.c | 27 ++--- drivers/iommu/io-pgtable.h | 3 +++ drivers/iommu/iommu.c | 1 + include/linux/iommu.h | 1 + 7 files changed, 85 insertions(+), 11 deletions(-) -- 1.8.3 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH 1/7] vfio/spimdev: Add documents for WarpDrive framework
Hi! > WarpDrive is a common user space accelerator framework. Its main component > in Kernel is called spimdev, Share Parent IOMMU Mediated Device. It exposes spimdev is really unfortunate name. It looks like it has something to do with SPI, but it does not. > +++ b/Documentation/warpdrive/warpdrive.rst > @@ -0,0 +1,153 @@ > +Introduction of WarpDrive > += > + > +*WarpDrive* is a general accelerator framework built on top of vfio. > +It can be taken as a light weight virtual function, which you can use without > +*SR-IOV* like facility and can be shared among multiple processes. > + > +It can be used as the quick channel for accelerators, network adaptors or > +other hardware in user space. It can make some implementation simpler. E.g. > +you can reuse most of the *netdev* driver and just share some ring buffer to > +the user space driver for *DPDK* or *ODP*. Or you can combine the RSA > +accelerator with the *netdev* in the user space as a Web reversed proxy, etc. What is DPDK? ODP? > +How does it work > + > + > +*WarpDrive* takes the Hardware Accelerator as a heterogeneous processor which > +can share some load for the CPU: > + > +.. image:: wd.svg > +:alt: This is a .svg image, if your browser cannot show it, > +try to download and view it locally > + > +So it provides the capability to the user application to: > + > +1. Send request to the hardware > +2. Share memory with the application and other accelerators > + > +These requirements can be fulfilled by VFIO if the accelerator can serve each > +application with a separated Virtual Function. But a *SR-IOV* like VF (we > will > +call it *HVF* hereinafter) design is too heavy for the accelerator which > +service thousands of processes. VFIO? VF? HVF? Also "gup" might be worth spelling out. > +References > +== > +.. [1] Accroding to the comment in in mm/gup.c, The *gup* is only safe within > + a syscall. Because it can only keep the physical memory in place > + without making sure the VMA will always point to it. Maybe we should > + raise the VM_PINNED patchset (see > + https://lists.gt.net/linux/kernel/1931993) again to solve this probl I went through the docs, but I still don't know what it does. Pavel ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 0/7] Stop losing firmware-set DMA masks
On Mon, Aug 6, 2018 at 2:08 PM Christoph Hellwig wrote: > > On Mon, Aug 06, 2018 at 12:01:34PM +0200, Arnd Bergmann wrote: > > There are a few subtle corner cases here, in particular in which cases > > the new dma_set_mask() behavior on arm64 reports success or > > failure when truncating the mask to the bus_dma_mask. > > Going forward my plan was to make dma_set_mask() never fail. The idea > is that it sets the mask that the device is capable of, and the core > dma code is responsible for also looking at bus_dma_mask and otherwise > make things just work. > > Robin brought up the case where a platform can't handle a given limitation > ever, e.g. a PCI(e) device with a 24-bit dma mask on a device with a dma > offset that means we'll never have any physical memory reachable in that > range. So we'll either still need to allow it to fail for such corner > cases or delay such error until later, e.g. when dma_alloc_* (or in the > corner case of the corner case dma_map_*) is called. I'm still undecided > which way to go, but not allowing error returns from dma_set_mask and > its variants sounds very tempting. Another case that I think can happen in practice are devices that need a DMA mask smaller than 32-bit wide (either asked for by the driver or according to bus limitations) on a platform that has no ZONE_DMA. However, IIRC there was no reason to ever fail a dma_set_mask() to something wider than 32 bit even if the resulting mask still stays at the default 32-bit mask or something inbetween. Arnd ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 0/7] Stop losing firmware-set DMA masks
On Mon, Aug 06, 2018 at 12:01:34PM +0200, Arnd Bergmann wrote: > There are a few subtle corner cases here, in particular in which cases > the new dma_set_mask() behavior on arm64 reports success or > failure when truncating the mask to the bus_dma_mask. Going forward my plan was to make dma_set_mask() never fail. The idea is that it sets the mask that the device is capable of, and the core dma code is responsible for also looking at bus_dma_mask and otherwise make things just work. Robin brought up the case where a platform can't handle a given limitation ever, e.g. a PCI(e) device with a 24-bit dma mask on a device with a dma offset that means we'll never have any physical memory reachable in that range. So we'll either still need to allow it to fail for such corner cases or delay such error until later, e.g. when dma_alloc_* (or in the corner case of the corner case dma_map_*) is called. I'm still undecided which way to go, but not allowing error returns from dma_set_mask and its variants sounds very tempting. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 0/7] Stop losing firmware-set DMA masks
On Tue, Jul 31, 2018 at 1:30 PM Robin Murphy wrote: > On 29/07/18 13:32, Arnd Bergmann wrote: > > On Tue, Jul 24, 2018 at 12:16 AM, Robin Murphy wrote: > > Thanks a lot for working on this, this has bugged me for many years, > > and I've discussed possible solutions with lots of people over time. > > > > I /think/ all your patches are good, but I'm currently travelling and don't > > have a chance to review the resulting overall implementation. > > Could you summarize what happens in the following corner cases of > > DT dma-ranges after your changes (with a driver not setting a mask, > > setting a 64-bit mask and setting a 24-bit mask, respectively)? > > > > a) a device with no dma-ranges property anywhere in its parents > > b) a device with with a 64-bit dma-ranges translation in its parent > > but none in its grandparent > > c) a device with no dma-ranges in its parent but a 64-bit mask > > in its grandparent > > d) a device with a 24-bit mask in its parent. > > In terms of the actual dma-ranges parsing, nothing should be changed by > these patches, so the weirdness and inconsistency that I'm pretty sure > exists for some of those cases will still be there for the moment - I'm > starting on actually fixing of_dma_get_range() now. Right, but I'm interested in the combination of of_dma_get_range() with dma_set_mask(), which is the part that was traditionally broken on arm64 and should be fixed now. There are a few subtle corner cases here, in particular in which cases the new dma_set_mask() behavior on arm64 reports success or failure when truncating the mask to the bus_dma_mask. > The effect after these patches is that a device with a "valid" (per the > current of_dma_get_range() implementation) dma-ranges translation gets > it bus_dma_mask set to cover the given range, whereas a device with no > valid dma-ranges effectively gets a 32-bit bus_dma_mask. That's slightly > different from the ACPI default behaviour, due to subtle spec > differences, but I think it's in line with what you've proposed before > for DT, and it's certainly still flexible if anyone has a different > view. The bus_dma_mask in itself should also be low-impact, since it > will only currently be enforced in the generic dma-direct and iommu-dma > paths, so the likes of powerpc shouldn't see any change at all just yet. Ok. Arnd ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
