[PATCH] iommu/amd: Fix for L2 race with VM invalidation
From: Jay Cornwall
Do not disassociate the process page tables from a PASID during VM
invalidation. Invalidate the IOMMU TLB and IOTLBs before invalidation.
L2 translations may fail during VM range invalidation. The current
implementation associates an empty page table with a PASID within
the critical section to avoid races with the VM. This causes
unconditional failure of all translations during this period.
A low probability race exists with this fix. Translations received
within the critical section to PTEs which are concurrently being
invalidated may resolve to stale mappings.
Signed-off-by: Jay Cornwall
Signed-off-by: Suravee Suthikulpanit
---
drivers/iommu/amd_iommu.c | 40 +++
drivers/iommu/amd_iommu_proto.h |2 ++
drivers/iommu/amd_iommu_v2.c| 40 ++-
3 files changed, 44 insertions(+), 38 deletions(-)
diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index c949520..da43985 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -3667,12 +3667,12 @@ out:
return ret;
}
-static int __amd_iommu_flush_page(struct protection_domain *domain, int pasid,
- u64 address)
+static int __amd_iommu_flush_pages(struct protection_domain *domain, int pasid,
+ u64 address, bool size)
{
INC_STATS_COUNTER(invalidate_iotlb);
- return __flush_pasid(domain, pasid, address, false);
+ return __flush_pasid(domain, pasid, address, size);
}
int amd_iommu_flush_page(struct iommu_domain *dom, int pasid,
@@ -3683,13 +3683,45 @@ int amd_iommu_flush_page(struct iommu_domain *dom, int
pasid,
int ret;
spin_lock_irqsave(&domain->lock, flags);
- ret = __amd_iommu_flush_page(domain, pasid, address);
+ ret = __amd_iommu_flush_pages(domain, pasid, address, 0);
spin_unlock_irqrestore(&domain->lock, flags);
return ret;
}
EXPORT_SYMBOL(amd_iommu_flush_page);
+int amd_iommu_flush_page_range(struct iommu_domain *dom, int pasid,
+ u64 start, u64 end)
+{
+ struct protection_domain *domain = dom->priv;
+ unsigned long flags;
+ unsigned long pages;
+ int ret;
+ u64 addr;
+ bool size;
+
+ pages = iommu_num_pages(start, end - start, PAGE_SIZE);
+
+ /*
+* If we have to flush more than one page, flush all
+* pages for this PASID
+*/
+ if (pages > 1) {
+ addr = CMD_INV_IOMMU_ALL_PAGES_ADDRESS;
+ size = 1;
+ } else {
+ addr = start;
+ size = 0;
+ }
+
+ spin_lock_irqsave(&domain->lock, flags);
+ ret = __amd_iommu_flush_pages(domain, pasid, addr, size);
+ spin_unlock_irqrestore(&domain->lock, flags);
+
+ return ret;
+}
+EXPORT_SYMBOL(amd_iommu_flush_page_range);
+
static int __amd_iommu_flush_tlb(struct protection_domain *domain, int pasid)
{
INC_STATS_COUNTER(invalidate_iotlb_all);
diff --git a/drivers/iommu/amd_iommu_proto.h b/drivers/iommu/amd_iommu_proto.h
index 95ed6de..3919dfc 100644
--- a/drivers/iommu/amd_iommu_proto.h
+++ b/drivers/iommu/amd_iommu_proto.h
@@ -50,6 +50,8 @@ extern void amd_iommu_domain_direct_map(struct iommu_domain
*dom);
extern int amd_iommu_domain_enable_v2(struct iommu_domain *dom, int pasids);
extern int amd_iommu_flush_page(struct iommu_domain *dom, int pasid,
u64 address);
+extern int amd_iommu_flush_page_range(struct iommu_domain *dom, int pasid,
+ u64 start, u64 end);
extern int amd_iommu_flush_tlb(struct iommu_domain *dom, int pasid);
extern int amd_iommu_domain_set_gcr3(struct iommu_domain *dom, int pasid,
unsigned long cr3);
diff --git a/drivers/iommu/amd_iommu_v2.c b/drivers/iommu/amd_iommu_v2.c
index 5208828..592de33f 100644
--- a/drivers/iommu/amd_iommu_v2.c
+++ b/drivers/iommu/amd_iommu_v2.c
@@ -90,13 +90,6 @@ static DEFINE_SPINLOCK(ps_lock);
static struct workqueue_struct *iommu_wq;
-/*
- * Empty page table - Used between
- * mmu_notifier_invalidate_range_start and
- * mmu_notifier_invalidate_range_end
- */
-static u64 *empty_page_table;
-
static void free_pasid_states(struct device_state *dev_state);
static void unbind_pasid(struct device_state *dev_state, int pasid);
static int task_exit(struct notifier_block *nb, unsigned long e, void *data);
@@ -443,22 +436,12 @@ static void mn_invalidate_range_start(struct mmu_notifier
*mn,
pasid_state = mn_to_state(mn);
dev_state = pasid_state->device_state;
- amd_iommu_domain_set_gcr3(dev_state->domain, pasid_state->pasid,
- __pa(empty_page_table));
-}
-
-static void mn_invalidate_range_end(struct mmu_notifier *mn,
- struct mm_struct *mm,
- unsigned long sta
Re: [PATCH v13 00/19] iommu/exynos: Fixes and Enhancements of System MMU driver with DT
On Tue, May 13, 2014 at 10:50 PM, Joerg Roedel wrote: > On Mon, May 12, 2014 at 11:44:45AM +0530, Shaik Ameer Basha wrote: >> Cho KyongHo (18): >> iommu/exynos: fix build errors >> iommu/exynos: change error handling when page table update is failed >> iommu/exynos: allocate lv2 page table from own slab >> iommu/exynos: fix L2TLB invalidation >> iommu/exynos: remove prefetch buffer setting >> iommu/exynos: add missing cache flush for removed page table entries >> iommu/exynos: always enable runtime PM >> iommu/exynos: remove dbgname from drvdata of a System MMU >> iommu/exynos: use managed device helper functions >> iommu/exynos: gating clocks of master H/W >> iommu/exynos: remove custom fault handler >> iommu/exynos: change rwlock to spinlock >> iommu/exynos: use exynos-iommu specific typedef >> iommu/exynos: enhanced error messages >> documentation: iommu: add binding document of Exynos System MMU >> iommu/exynos: support for device tree >> iommu/exynos: turn on useful configuration options >> iommu/exynos: apply workaround of caching fault page table entries >> >> .../devicetree/bindings/iommu/samsung,sysmmu.txt | 65 ++ >> drivers/iommu/exynos-iommu.c | 1035 >> >> 2 files changed, 677 insertions(+), 423 deletions(-) >> create mode 100644 >> Documentation/devicetree/bindings/iommu/samsung,sysmmu.txt > > Applied, thanks. Please send another patch to update the documentation > as requested by Arnd. Hi Joerg, Thanks for applying the series. I posted one patch addressing 'Arnd' comments. Please apply it on top of this series. -- documentation/iommu: Add note on existing DT binding status Regards, Shaik > > > Joerg > > > ___ > iommu mailing list > iommu@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/iommu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH] documentation/iommu: Add note on existing DT binding status
The current dt binding for Exynos System MMU can be changed, if found incompatible with the support for "Generic IOMMU Binding". This patch adds a note to the binding documentation stating the same. Signed-off-by: Shaik Ameer Basha --- .../devicetree/bindings/iommu/samsung,sysmmu.txt |5 + 1 file changed, 5 insertions(+) diff --git a/Documentation/devicetree/bindings/iommu/samsung,sysmmu.txt b/Documentation/devicetree/bindings/iommu/samsung,sysmmu.txt index 15b2a2b..6fa4c73 100644 --- a/Documentation/devicetree/bindings/iommu/samsung,sysmmu.txt +++ b/Documentation/devicetree/bindings/iommu/samsung,sysmmu.txt @@ -27,6 +27,11 @@ The drivers must consider how to handle those System MMUs. One of the idea is to implement child devices or sub-devices which are the client devices of the System MMU. +Note: +The current DT binding for the Exynos System MMU is incomplete. +The following properties can be removed or changed, if found incompatible with +the "Generic IOMMU Binding" support for attaching devices to the IOMMU. + Required properties: - compatible: Should be "samsung,exynos-sysmmu" - reg: A tuple of base address and size of System MMU registers. -- 1.7.9.5 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3] iommu: Add driver for Renesas VMSA-compatible IPMMU
On Tue, May 13, 2014 at 11:04:10PM +0200, Laurent Pinchart wrote: > > Isn't this the same as ipmmu_tlb_invalidate()? > > ipmmu_tlb_invalidate() performs a read-update-write operation on the IMCTR > register to set the FLUSH bit without modifying the other bits, while this > function writes the FLUSH bit and sets all other bits (including the enable > bit) to zero. The difference is thus important. Ah ok, I've seen that, but wasn't aware that it makes a difference. > > Why not? This is something the IOMMU-API basically supports (multiple > > devices behind different IOMMUs in the same domain). Can't you just use > > the same page-table for different IOMMUs? > > I might be able to (I'll need to check first though), but I don't really see > what the use cases for sharing a common page table between separate IOMMUs > would be. Could you please elaborate a bit ? Given that we're getting close > to > the v3.16 merge window, would it be acceptable to fix (if needed) that as a > follow-up patch, as the driver is already usable as-is ? The typical use-case is device-assignment in KVM, when you want to assign multiple devices to the same guest it is desireable to only have one domain with one page-table. At least this is where this requirement comes from in the IOMMU-API, and I like the IOMMU drivers to be consistent in their implementation of the API. I think given that otherwise the code looks good overall it is ok if you do this as a follow-on patch to this one. I can put it in my tree for the time being so that you can make a patch against the feature branch. Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3] iommu: Add driver for Renesas VMSA-compatible IPMMU
Hi Joerg,
On Tuesday 13 May 2014 19:55:29 Joerg Roedel wrote:
> Hi Laurent,
>
> Sorry for taking so long with this.
No worries. As long as the driver gets in v3.16 (wink wink :-)) that's fine
> The code looks good and clean overall, besides my second comment.
>
> On Wed, Apr 02, 2014 at 12:47:37PM +0200, Laurent Pinchart wrote:
> > +static void ipmmu_domain_destroy_context(struct ipmmu_vmsa_domain
> > *domain)
> > +{
> > + /*
> > +* Disable the context. Flush the TLB as required when modifying the
> > +* context registers.
> > +*
> > +* TODO: Is TLB flush really needed ?
> > +*/
> > + ipmmu_ctx_write(domain, IMCTR, IMCTR_FLUSH);
> > + ipmmu_tlb_sync(domain);
>
> Isn't this the same as ipmmu_tlb_invalidate()?
ipmmu_tlb_invalidate() performs a read-update-write operation on the IMCTR
register to set the FLUSH bit without modifying the other bits, while this
function writes the FLUSH bit and sets all other bits (including the enable
bit) to zero. The difference is thus important.
> > +static int ipmmu_attach_device(struct iommu_domain *io_domain,
> > + struct device *dev)
> > +{
> > + struct ipmmu_vmsa_device *mmu = dev->archdata.iommu;
> > + struct ipmmu_vmsa_domain *domain = io_domain->priv;
> > + const struct ipmmu_vmsa_master *master;
> > + unsigned long flags;
> > + int ret = 0;
> > +
> > + if (!mmu) {
> > + dev_err(dev, "Cannot attach to IPMMU\n");
> > + return -ENXIO;
> > + }
> > +
> > + spin_lock_irqsave(&domain->lock, flags);
> > +
> > + if (!domain->mmu) {
> > + /* The domain hasn't been used yet, initialize it. */
> > + domain->mmu = mmu;
> > + ret = ipmmu_domain_init_context(domain);
> > + } else if (domain->mmu != mmu) {
> > + /*
> > +* Something is wrong, we can't attach two devices using
> > +* different IOMMUs to the same domain.
> > +*/
>
> Why not? This is something the IOMMU-API basically supports (multiple
> devices behind different IOMMUs in the same domain). Can't you just use
> the same page-table for different IOMMUs?
I might be able to (I'll need to check first though), but I don't really see
what the use cases for sharing a common page table between separate IOMMUs
would be. Could you please elaborate a bit ? Given that we're getting close to
the v3.16 merge window, would it be acceptable to fix (if needed) that as a
follow-up patch, as the driver is already usable as-is ?
--
Regards,
Laurent Pinchart
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3] iommu: Add driver for Renesas VMSA-compatible IPMMU
Hi Laurent,
Sorry for taking so long with this. The code looks good and clean
overall, besides my second comment.
On Wed, Apr 02, 2014 at 12:47:37PM +0200, Laurent Pinchart wrote:
> +static void ipmmu_domain_destroy_context(struct ipmmu_vmsa_domain *domain)
> +{
> + /*
> + * Disable the context. Flush the TLB as required when modifying the
> + * context registers.
> + *
> + * TODO: Is TLB flush really needed ?
> + */
> + ipmmu_ctx_write(domain, IMCTR, IMCTR_FLUSH);
> + ipmmu_tlb_sync(domain);
Isn't this the same as ipmmu_tlb_invalidate()?
> +static int ipmmu_attach_device(struct iommu_domain *io_domain,
> +struct device *dev)
> +{
> + struct ipmmu_vmsa_device *mmu = dev->archdata.iommu;
> + struct ipmmu_vmsa_domain *domain = io_domain->priv;
> + const struct ipmmu_vmsa_master *master;
> + unsigned long flags;
> + int ret = 0;
> +
> + if (!mmu) {
> + dev_err(dev, "Cannot attach to IPMMU\n");
> + return -ENXIO;
> + }
> +
> + spin_lock_irqsave(&domain->lock, flags);
> +
> + if (!domain->mmu) {
> + /* The domain hasn't been used yet, initialize it. */
> + domain->mmu = mmu;
> + ret = ipmmu_domain_init_context(domain);
> + } else if (domain->mmu != mmu) {
> + /*
> + * Something is wrong, we can't attach two devices using
> + * different IOMMUs to the same domain.
> + */
Why not? This is something the IOMMU-API basically supports (multiple
devices behind different IOMMUs in the same domain). Can't you just use
the same page-table for different IOMMUs?
Joerg
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v13 00/19] iommu/exynos: Fixes and Enhancements of System MMU driver with DT
On Mon, May 12, 2014 at 11:44:45AM +0530, Shaik Ameer Basha wrote: > Cho KyongHo (18): > iommu/exynos: fix build errors > iommu/exynos: change error handling when page table update is failed > iommu/exynos: allocate lv2 page table from own slab > iommu/exynos: fix L2TLB invalidation > iommu/exynos: remove prefetch buffer setting > iommu/exynos: add missing cache flush for removed page table entries > iommu/exynos: always enable runtime PM > iommu/exynos: remove dbgname from drvdata of a System MMU > iommu/exynos: use managed device helper functions > iommu/exynos: gating clocks of master H/W > iommu/exynos: remove custom fault handler > iommu/exynos: change rwlock to spinlock > iommu/exynos: use exynos-iommu specific typedef > iommu/exynos: enhanced error messages > documentation: iommu: add binding document of Exynos System MMU > iommu/exynos: support for device tree > iommu/exynos: turn on useful configuration options > iommu/exynos: apply workaround of caching fault page table entries > > .../devicetree/bindings/iommu/samsung,sysmmu.txt | 65 ++ > drivers/iommu/exynos-iommu.c | 1035 > > 2 files changed, 677 insertions(+), 423 deletions(-) > create mode 100644 Documentation/devicetree/bindings/iommu/samsung,sysmmu.txt Applied, thanks. Please send another patch to update the documentation as requested by Arnd. Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v13 00/19] iommu/exynos: Fixes and Enhancements of System MMU driver with DT
On Mon, May 12, 2014 at 3:37 PM, Arnd Bergmann wrote: > On Monday 12 May 2014 11:44:45 Shaik Ameer Basha wrote: >> This is the subset of previous v12 series and includes only the fixes and >> enhancements, leaving out the private DT bindings as discussed in the below >> thread. >> -- http://www.gossamer-threads.com/lists/linux/kernel/1918178 >> >> This patch series includes, >> 1] fixes for exynos-iommu driver build break >> 2] includes several bug fixes and enhancements for the exynos-iommu driver >> 3] code to handle multiple exynos sysmmu versions >> 4] adding support for device tree >> Documentation/devicetree/bindings/iommu/samsung,sysmmu.txt > > The patches look good to me, but please add a note into the samsung,sysmmu.txt > file explaining that the binding is incomplete and that it's possible to > change in incompatible ways when we add support for attaching devices to > the IOMMU through the generic IOMMU binding. Hi Arnd, Thank you. If there are no more comments on this series, I can send one more incremental patch with the note mentioned in your review comment. Hi Joerg Roedel, Can you please add this series to your tree. Regards, Shaik Ameer Basha > > Arnd > ___ > iommu mailing list > iommu@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/iommu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v1 1/1] iommu/amd: fix enabling exclusion range for an exact device
On Wed, May 07, 2014 at 01:54:52PM +0800, Su, Friendy wrote: > > From: Su Friendy > > set_device_exclusion_range(u16 devid, struct ivmd_header *m) enables > exclusion range for ONE device. IOMMU does not translate the access > to the exclusion range from the device. > > The device is specified by input argument 'devid'. But 'devid' is not > passed to the actual set function set_dev_entry_bit(), instead > 'm->devid' is passed. 'm->devid' does not specify the exact device > which needs enable the exclusion range. 'm->devid' represents DeviceID > field of IVMD, which has different meaning depends on IVMD type. > > The caller init_exclusion_range() sets 'devid' for ONE device. When > m->type is equal to ACPI_IVMD_TYPE_ALL or ACPI_IVMD_TYPE_RANGE, > 'm->devid' is not equal to 'devid'. > > This patch fixes 'm->devid' to 'devid'. > > Signed-off-by: Su Friendy > Signed-off-by: Tamori Masahiro Applied, thanks. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
