diff mbox

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

Message ID 945CA011AD5F084CBEA3E851C0AB28894B8AE02F@SHSMSX101.ccr.corp.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Quan Xu May 13, 2016, 8:04 a.m. UTC 
On May 12, 2016 11:06 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 12.05.16 at 16:28, <quan.xu@intel.com> wrote:
> > 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:

Jan,

Try it again, I hope I have got it. If not, could you write some sample code for me as an exception? :)

Thanks again!!
Quan

Comments

Jan Beulich May 13, 2016, 9:08 a.m. UTC | #1 
>>> On 13.05.16 at 10:04, <quan.xu@intel.com> wrote:
> On May 12, 2016 11:06 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> >>> On 12.05.16 at 16:28, <quan.xu@intel.com> wrote:
>> > 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:
> Try it again, I hope I have got it. If not, could you write some sample code 
> for me as an exception? :)

Well, this would be acceptable (albeit not ideal) to me as a first
cut (and instead of continuing this apparently fruitless discussion
I'd then see to provide a follow-up patch improving it), but won't
(I'm afraid) please Kevin: You now again don't log anything for
DomU-s. That's one half of the "not ideal" part; the other is that
of two far apart events for Dom0, only the first one would get
logged.

In any event there's stylistic cleanup necessary:

> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -240,21 +240,63 @@ int iommu_map_page(struct domain *d, unsigned long gfn, unsigned long mfn,
>                     unsigned int flags)
>  {
>      const struct domain_iommu *hd = dom_iommu(d);
> +    static int printed = 0;

This doesn't need an initializer, should be bool_t, and should get
moved ...

> +    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 ( is_hardware_domain(d) )
> +        {

... here.

> +            if ( !printed )
> +            {
> +                printk(XENLOG_ERR
> +                       "d%d: IOMMU mapping gfn %#lx mfn %#lx failed %d.",
> +                       d->domain_id, gfn, mfn, rc);
> +
> +                printed = 1;
> +            }
> +        }
> +        else
> +            domain_crash(d);
> +    }

What I think might at least come close to what Kevin and I would
like to see is something like

	if ( !d->is_shutting_down && printk_ratelimit() )
		printk(...);
	if ( !is_hardware_domain(d) )
		domain_crash(d);

For Dom0 that'll still be more verbose than we'd really like, but it
at least wouldn't outright flood the console.

Jan
Quan Xu May 13, 2016, 9:20 a.m. UTC | #2 
On May 13, 2016 5:09 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 13.05.16 at 10:04, <quan.xu@intel.com> wrote:
> > On May 12, 2016 11:06 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >> >>> On 12.05.16 at 16:28, <quan.xu@intel.com> wrote:
> >> > 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:
> What I think might at least come close to what Kevin and I would like to see is
> something like
> 
> 	if ( !d->is_shutting_down && printk_ratelimit() )
> 		printk(...);
> 	if ( !is_hardware_domain(d) )
> 		domain_crash(d);
> 
> For Dom0 that'll still be more verbose than we'd really like, but it at least
> wouldn't outright flood the console.
> 

Thanks!!
It is really better to come close to Kevin's previous suggestion. I'll follow it for v5.

Quan
diff mbox

Patch

--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -240,21 +240,63 @@  int iommu_map_page(struct domain *d, unsigned long gfn, unsigned long mfn,
                    unsigned int flags)
 {
     const struct domain_iommu *hd = dom_iommu(d);
+    static int printed = 0;
+    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 ( is_hardware_domain(d) )
+        {
+            if ( !printed )
+            {
+                printk(XENLOG_ERR
+                       "d%d: IOMMU mapping gfn %#lx mfn %#lx failed %d.",
+                       d->domain_id, gfn, mfn, rc);
+
+                printed = 1;
+            }
+        }
+        else
+            domain_crash(d);
+    }
+
+    return rc;
 }

 int iommu_unmap_page(struct domain *d, unsigned long gfn)
 {
     const struct domain_iommu *hd = dom_iommu(d);
+    static int printed = 0;
+    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 ( is_hardware_domain(d) )
+        {
+            if ( !printed )
+            {
+                printk(XENLOG_ERR
+                       "d%d: IOMMU unmapping gfn %#lx failed %d.",
+                       d->domain_id, gfn, rc);
+
+                printed = 1;
+            }
+        }
+        else
+            domain_crash(d);
+    }
+
+    return rc;
 }