diff mbox

[v5,02/10] IOMMU: handle IOMMU mapping and unmapping failures

Message ID 1463558911-98187-3-git-send-email-quan.xu@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Quan Xu May 18, 2016, 8:08 a.m. UTC 
Treat IOMMU mapping and unmapping failures as a fatal to the DomU
If IOMMU mapping and unmapping failed, crash the DomU and propagate
the error up to the call trees.

No spamming can occur. For DomU, we avoid logging any message
for already dying domains. For Dom0, that'll still be more verbose
than we'd really like, but it at least wouldn't outright flood the
console.

Signed-off-by: Quan Xu <quan.xu@intel.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>

CC: Jan Beulich <jbeulich@suse.com>
CC: Kevin Tian <kevin.tian@intel.com>
---
 xen/drivers/passthrough/iommu.c | 32 ++++++++++++++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)

Comments

Jan Beulich May 23, 2016, 1:40 p.m. UTC | #1 
>>> On 18.05.16 at 10:08, <quan.xu@intel.com> wrote:
> No spamming can occur.

May I suggest "No spamming of the log can occur", to set some
context for what follows?

> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -240,21 +240,49 @@ int iommu_map_page(struct domain *d, unsigned long gfn, unsigned long mfn,
>                     unsigned int flags)
>  {
>      const struct domain_iommu *hd = dom_iommu(d);
> +    int rc;
>  
>      if ( !iommu_enabled || !hd->platform_ops )
>          return 0;
>  
> -    return hd->platform_ops->map_page(d, gfn, mfn, flags);
> +    rc = hd->platform_ops->map_page(d, gfn, mfn, flags);
> +
> +    if ( unlikely(rc) )

A more general word on the use of blank lines: I think their use is
well advised to separate logically (mostly) independent steps. In
cases like this, where you check the return value of a function,
the two things really belong together imo. Using too little blank
lines negatively affects readability, but using too many easily
leads to not seeing enough context anymore when looking at
some code fragment.

> +    {
> +        if ( !d->is_shutting_down && printk_ratelimit() )
> +            printk(XENLOG_ERR
> +                   "d%d: IOMMU mapping gfn %#lx mfn %#lx failed %d.",

I really dislike having to repeat this yet another time: No stops in
log messages please. Also to the reader of the log it would be
unclear what the number at the end represents. May I suggest

            printk(XENLOG_ERR
                   "d%d: IOMMU mapping gfn %#lx to mfn %#lx failed: %d",
                   d->domain_id, gfn, mfn, rc);

(arguably one might then also remove the words "gfn" and "mfn").

Apart from these cosmetic issues I think this is fine now.

Jan
Quan Xu May 24, 2016, 9:09 a.m. UTC | #2 
On May 23, 2016 9:41 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 18.05.16 at 10:08, <quan.xu@intel.com> wrote:
> > No spamming can occur.
> 
> May I suggest "No spamming of the log can occur", to set some context for
> what follows?
> 

Make sense.

> > --- a/xen/drivers/passthrough/iommu.c
> > +++ b/xen/drivers/passthrough/iommu.c
> > @@ -240,21 +240,49 @@ int iommu_map_page(struct domain *d,
> unsigned long gfn, unsigned long mfn,
> >                     unsigned int flags)  {
> >      const struct domain_iommu *hd = dom_iommu(d);
> > +    int rc;
> >
> >      if ( !iommu_enabled || !hd->platform_ops )
> >          return 0;
> >
> > -    return hd->platform_ops->map_page(d, gfn, mfn, flags);
> > +    rc = hd->platform_ops->map_page(d, gfn, mfn, flags);
> > +
> > +    if ( unlikely(rc) )
> 
> A more general word on the use of blank lines: I think their use is well advised
> to separate logically (mostly) independent steps. In cases like this, where you
> check the return value of a function, the two things really belong together
> imo. Using too little blank lines negatively affects readability, but using too
> many easily leads to not seeing enough context anymore when looking at
> some code fragment.
> 

I will also apply it to other patches.

> > +    {
> > +        if ( !d->is_shutting_down && printk_ratelimit() )
> > +            printk(XENLOG_ERR
> > +                   "d%d: IOMMU mapping gfn %#lx mfn %#lx failed %d.",
> 
> I really dislike having to repeat this yet another time: No stops in log messages
> please. Also to the reader of the log it would be unclear what the number at
> the end represents. May I suggest
> 
>             printk(XENLOG_ERR
>                    "d%d: IOMMU mapping gfn %#lx to mfn %#lx failed: %d",
>                    d->domain_id, gfn, mfn, rc);
> 
> (arguably one might then also remove the words "gfn" and "mfn").
> 

To me, we are better not to remove 'gfn' / 'mfn', but I really need to add a '\n'.. then

printk(XENLOG_ERR
       "d%d: IOMMU mapping gfn %#lx to mfn %#lx failed: %d\n",
       d->domain_id, gfn, mfn, rc);


Quan

> Apart from these cosmetic issues I think this is fine now.
diff mbox

Patch

diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 9d104d2..7c70306 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -240,21 +240,49 @@  int iommu_map_page(struct domain *d, unsigned long gfn, unsigned long mfn,
                    unsigned int flags)
 {
     const struct domain_iommu *hd = dom_iommu(d);
+    int rc;
 
     if ( !iommu_enabled || !hd->platform_ops )
         return 0;
 
-    return hd->platform_ops->map_page(d, gfn, mfn, flags);
+    rc = hd->platform_ops->map_page(d, gfn, mfn, flags);
+
+    if ( unlikely(rc) )
+    {
+        if ( !d->is_shutting_down && printk_ratelimit() )
+            printk(XENLOG_ERR
+                   "d%d: IOMMU mapping gfn %#lx mfn %#lx failed %d.",
+                   d->domain_id, gfn, mfn, rc);
+
+        if ( !is_hardware_domain(d) )
+            domain_crash(d);
+    }
+
+    return rc;
 }
 
 int iommu_unmap_page(struct domain *d, unsigned long gfn)
 {
     const struct domain_iommu *hd = dom_iommu(d);
+    int rc;
 
     if ( !iommu_enabled || !hd->platform_ops )
         return 0;
 
-    return hd->platform_ops->unmap_page(d, gfn);
+    rc = hd->platform_ops->unmap_page(d, gfn);
+
+    if ( unlikely(rc) )
+    {
+        if ( !d->is_shutting_down && printk_ratelimit() )
+            printk(XENLOG_ERR
+                   "d%d: IOMMU unmapping gfn %#lx failed %d.",
+                   d->domain_id, gfn, rc);
+
+        if ( !is_hardware_domain(d) )
+            domain_crash(d);
+    }
+
+    return rc;
 }
 
 static void iommu_free_pagetables(unsigned long unused)