Re: [RFC 6/7] iommu/core: add fault reporting

2011-09-07 Thread Ohad Ben-Cohen
On Mon, Sep 5, 2011 at 1:00 PM, Roedel, Joerg joerg.roe...@amd.com wrote:
 Please add a seperate function for setting the fault-handler. It is
 optional, so no need to be a value of the alloc-function.

Will do.

 Can you elaborate a bit on what the user of the api will do different
 between IOMMU_TLBMISS and IOMMU_NOPTE?
 My feeling is that those differences should be handled internally in the
 IOMMU driver, but probably I miss a use-case.

I actually agree. Moreover, since we're not planning on implementing
this (dynamic PTE/TLB loading is supported by the hardware, but we're
not really using it ATM), and I don't see any other user doing so at
this point, I'll just remove those two fault types altogether.

 Also, we need some flags to distinguish between the type of the fault
 (read, write, ...).

I'll add it (though it won't be used by OMAP since the hardware
doesn't tell us this info).

Thanks!
Ohad.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [RFC 6/7] iommu/core: add fault reporting

2011-09-05 Thread Roedel, Joerg
On Fri, Sep 02, 2011 at 01:32:35PM -0400, Ohad Ben-Cohen wrote:
 -struct iommu_domain *iommu_domain_alloc(void)
 +/**
 + * iommu_domain_alloc() - allocate and initialize a new iommu domain
 + * @handler: an optional pointer to a fault handler, or NULL if not needed
 + *
 + * Returns the new domain, or NULL on error.
 + */
 +struct iommu_domain *iommu_domain_alloc(iommu_fault_handler_t handler)

Please add a seperate function for setting the fault-handler. It is
optional, so no need to be a value of the alloc-function.

 +/**
 + * enum iommu_fault_types - iommu fault types
 + *
 + * @IOMMU_ERROR: Unrecoverable error
 + * @IOMMU_TLBMISS: TLB miss while the page table walker is disabled
 + * @IOMMU_NOPTE: TLB miss and no PTE for the requested address
 + */
 +enum iommu_fault_types {
 + IOMMU_ERROR,
 + IOMMU_TLBMISS,
 + IOMMU_NOPTE,
 +};

Can you elaborate a bit on what the user of the api will do different
between IOMMU_TLBMISS and IOMMU_NOPTE?
My feeling is that those differences should be handled internally in the
IOMMU driver, but probably I miss a use-case.
Also, we need some flags to distinguish between the type of the fault
(read, write, ...).

Joerg


-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[RFC 6/7] iommu/core: add fault reporting

2011-09-02 Thread Ohad Ben-Cohen
Add iommu fault report mechanism to the IOMMU API, so implementations
could report about mmu faults (translation errors, hardware errors,
etc..).

Fault reports can be used in several ways:
- mere logging
- reset the device that accessed the faulting address (may be necessary
  in case the device is a remote processor for example)
- implement dynamic PTE/TLB loading

Currently the fault handler is installed when allocating a new domain
(less churn for users). Alternatively, we can also add a dedicated
iommu_set_fault_handler() API (presumably with notifiers).

Adopt OMAP's iommu driver (and remove its now-redundant
omap_iommu_set_isr API) and fix existing users of iommu_domain_alloc()
to pass NULL fault handlers.

OMAP's iommu driver will currently only pass the generic IOMMU_ERROR
fault, but in principle we can support dynamic PTE/TLB loading too
in the future.

Signed-off-by: Ohad Ben-Cohen o...@wizery.com
---
 arch/arm/plat-omap/include/plat/iommu.h |3 +-
 drivers/iommu/iommu.c   |   10 +-
 drivers/iommu/omap-iommu.c  |   31 ++--
 drivers/media/video/omap3isp/isp.c  |2 +-
 include/linux/iommu.h   |   60 ++-
 virt/kvm/iommu.c|2 +-
 6 files changed, 74 insertions(+), 34 deletions(-)

diff --git a/arch/arm/plat-omap/include/plat/iommu.h 
b/arch/arm/plat-omap/include/plat/iommu.h
index 7f1df0e..a1d79ee 100644
--- a/arch/arm/plat-omap/include/plat/iommu.h
+++ b/arch/arm/plat-omap/include/plat/iommu.h
@@ -32,6 +32,7 @@ struct omap_iommu {
void __iomem*regbase;
struct device   *dev;
void*isr_priv;
+   struct iommu_domain *domain;
 
unsigned intrefcount;
spinlock_t  iommu_lock; /* global for this whole object */
@@ -48,8 +49,6 @@ struct omap_iommu {
struct list_headmmap;
struct mutexmmap_lock; /* protect mmap */
 
-   int (*isr)(struct omap_iommu *obj, u32 da, u32 iommu_errs, void *priv);
-
void *ctx; /* iommu context: registres saved area */
u32 da_start;
u32 da_end;
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index e61a9ba..c08f1a0 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -40,7 +40,13 @@ bool iommu_found(void)
 }
 EXPORT_SYMBOL_GPL(iommu_found);
 
-struct iommu_domain *iommu_domain_alloc(void)
+/**
+ * iommu_domain_alloc() - allocate and initialize a new iommu domain
+ * @handler: an optional pointer to a fault handler, or NULL if not needed
+ *
+ * Returns the new domain, or NULL on error.
+ */
+struct iommu_domain *iommu_domain_alloc(iommu_fault_handler_t handler)
 {
struct iommu_domain *domain;
int ret;
@@ -49,6 +55,8 @@ struct iommu_domain *iommu_domain_alloc(void)
if (!domain)
return NULL;
 
+   domain-handler = handler;
+
ret = iommu_ops-domain_init(domain);
if (ret)
goto out_free;
diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index bd5f606..089fddc 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -775,6 +775,7 @@ static irqreturn_t iommu_fault_handler(int irq, void *data)
u32 da, errs;
u32 *iopgd, *iopte;
struct omap_iommu *obj = data;
+   struct iommu_domain *domain = obj-domain;
 
if (!obj-refcount)
return IRQ_NONE;
@@ -786,7 +787,7 @@ static irqreturn_t iommu_fault_handler(int irq, void *data)
return IRQ_HANDLED;
 
/* Fault callback or TLB/PTE Dynamic loading */
-   if (obj-isr  !obj-isr(obj, da, errs, obj-isr_priv))
+   if (!report_iommu_fault(domain, obj-dev, da, IOMMU_ERROR))
return IRQ_HANDLED;
 
iommu_disable(obj);
@@ -904,33 +905,6 @@ static void omap_iommu_detach(struct omap_iommu *obj)
dev_dbg(obj-dev, %s: %s\n, __func__, obj-name);
 }
 
-int omap_iommu_set_isr(const char *name,
- int (*isr)(struct omap_iommu *obj, u32 da, u32 iommu_errs,
-void *priv),
- void *isr_priv)
-{
-   struct device *dev;
-   struct omap_iommu *obj;
-
-   dev = driver_find_device(omap_iommu_driver.driver, NULL, (void *)name,
-device_match_by_alias);
-   if (!dev)
-   return -ENODEV;
-
-   obj = to_iommu(dev);
-   spin_lock(obj-iommu_lock);
-   if (obj-refcount != 0) {
-   spin_unlock(obj-iommu_lock);
-   return -EBUSY;
-   }
-   obj-isr = isr;
-   obj-isr_priv = isr_priv;
-   spin_unlock(obj-iommu_lock);
-
-   return 0;
-}
-EXPORT_SYMBOL_GPL(omap_iommu_set_isr);
-
 /*
  * OMAP Device MMU(IOMMU) detection
  */
@@ -1115,6 +1089,7 @@ omap_iommu_attach_dev(struct iommu_domain *domain, struct 
device *dev)
}
 
omap_domain-iommu_dev = oiommu;
+   oiommu-domain = domain;
 
 out: