Re: [PATCH v4 02/10] IOMMU: handle IOMMU mapping and unmapping failures

From: "Xu, Quan" <quan.xu@intel.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: "dario.faggioli@citrix.com" <dario.faggioli@citrix.com>,
	"Tian, Kevin" <kevin.tian@intel.com>,
	"xen-devel@lists.xen.org" <xen-devel@lists.xen.org>
Subject: Re: [PATCH v4 02/10] IOMMU: handle IOMMU mapping and unmapping failures
Date: Tue, 10 May 2016 07:53:53 +0000	[thread overview]
Message-ID: <945CA011AD5F084CBEA3E851C0AB28894B8ABBE4@SHSMSX101.ccr.corp.intel.com> (raw)
In-Reply-To: <5731A1A602000078000E9D65@prv-mh.provo.novell.com>

On May 10, 2016 2:54 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 10.05.16 at 05:41, <quan.xu@intel.com> wrote:
> > On May 10, 2016 12:14 AM, Jan Beulich <JBeulich@suse.com> wrote:
> >> >>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote:
> >> > --- a/xen/drivers/passthrough/iommu.c
> >> > +++ b/xen/drivers/passthrough/iommu.c
> >> > @@ -240,21 +240,47 @@ 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) )
> >> > +    {
> >> > +        printk(XENLOG_ERR
> >> > +               "iommu_map_page: IOMMU mapping gfn %#lx mfn %#lx
> >> > + failed for
> >> dom%d.",
> >> > +               gfn, mfn, d->domain_id);
> >> > +
> >> > +        if ( !is_hardware_domain(d) )
> >> > +            domain_crash(d);
> >> > +    }
> >>
> >> This still may spam the console in at least the case of Dom0.
> >
> > I am afraid we may need a minor trade-off. What about:
> >
> >        dprintk(XENLOG_ERR, "...");
> >
> > to print out in debug mode.
> 
> And be silent in non-debug mode? That's not what we want.
> 

Without your below suggestion, this is really my best solution.

> >>  For DomU I'd
> >> really expect you to state in the commit message why no spamming can
> >> occur (of course assuming it really can't, which I'm not convinced of).
> >>
> >
> > In this v4, I think we will still spam the console in extreme cases :(:(..
> >
> > For mapping:
> > +                ret = iommu_map_page();
> > +                if ( unlikely(ret) )
> > +                {
> > +                    while ( i-- )
> > +                        iommu_unmap_page();
> > +                }
> >
> > We'll  stop map against any error and unmapping the previous mappings.
> > The extreme case is error for unmapping the previous mappings.
> >
> > Again -- I think dprintk is a better solution. Any suggestion?
> 
> For DomU the solution seems quite obvious: Only log a message if the domain
> is not already marked crashed. For Dom0 you'll need to get a little more
> creative (but by leveraging the fact that there's only one in the system, this
> can't be too difficult a problem to solve:
> e.g. "manually" rate limit these messages - see printk_ratelimit() et al).

Amazing!!
As the comment said, printk_ratelimit() is lifted from Linux. referred to the Linux, __iiuc__ , I will fix this issue as below (a variant):

...
+    rc = hd->platform_ops->unmap_page(d, gfn);
+
+    if ( unlikely(rc) )
+    {
+        if ( printk_ratelimit() )
+            printk(XENLOG_ERR
+                   "iommu_unmap_page: IOMMU unmapping gfn %#lx failed for dom%d.",
+                   gfn, d->domain_id);
+
+        if ( !is_hardware_domain(d) )
+            domain_crash(d);
+    }
+
+    return rc;
...

Thanks!!

Quan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel