Re: [Xen-devel] [PATCH v2 02/11] IOMMU: handle IOMMU mapping and unmapping failures

["Xu, Quan" <quan.xu@xxxxxxxxx>]

On April 25, 2016 5:27 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
> >>> On 18.04.16 at 16:00, <quan.xu@xxxxxxxxx> wrote:
> > --- a/xen/drivers/passthrough/iommu.c
> > +++ b/xen/drivers/passthrough/iommu.c
> > @@ -243,21 +243,33 @@ int iommu_map_page(struct domain *d,
> unsigned long gfn, unsigned long mfn,
> >                     unsigned int flags)  {
> >      struct hvm_iommu *hd = domain_hvm_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 ( rc && !is_hardware_domain(d) )
> > +        domain_crash(d);
> > +
> > +    return rc;
> >  }
> 
> As said before - letting this go completely silently for the hardware domain 
> is
> bad. At least the first instance of such an event needs a message to be 
> logged.
> Advanced variants where a message gets logged once in a while if the issue re-
> occurs would be nice, but aren't strictly necessary imo. And note that even
> logging all occurrences would not be a security issue, but just a usability 
> one
> (but I still recommend against this).
> 

IMO the IOMMU map/unmap failures  are the small probability events.  I think 
the log message wouldn't 
spam log message.

I'd like modify it as following:
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -243,11 +243,24 @@ int iommu_map_page(struct domain *d, unsigned long gfn, 
unsigned long mfn,
                    unsigned int flags)
 {
     struct hvm_iommu *hd = domain_hvm_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 ( 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);
+    }
+
+    return rc;
 }

 int iommu_unmap_page(struct domain *d, unsigned long gfn)



btw, in case  I have print-out for mapping failure, I think it is no need  to 
print out an extra message for the hardware domain.
If this is still not what you want, I'll continue to enhance it. Also I am not 
good at description, so I try to send out code modification directly. 


Ditto for iommu_unmap_page().


Quan







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