Re: [Patch v6 04/11] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU mapping (top level ones)

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

On June 07, 2016 4:19 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 07.06.16 at 09:51, <quan.xu@intel.com> wrote:
> > I still have one question. If I add __must_check annotation to
> > iommu_unmap_page().
> > How to fix this -- unmapping the previous IOMMU maps when IOMMU
> > mapping is failed..
> > e.g.,
> >
> > ept_set_entry()
> > {
> > ...
> >                 for ( i = 0; i < (1 << order); i++ )
> >                 {
> >                     rc = iommu_map_page(d, gfn + i, mfn_x(mfn) + i, iommu_flags);
> >                     if ( unlikely(rc) )
> >                     {
> >                         while ( i-- )
> >                             iommu_unmap_page(p2m->domain, gfn + i);
> >
> >                         break;
> >                     }
> >                 }
> > ...
> > }
> >
> >
> > If we leave it as is, it leads to compilation errors as __must_check
> > annotation. Also we return the first error, so we are no need to
> > cumulate the error of iommu_unmap_page().
> > That's also why I hesitated to add __must_check annotation to
> > iommu_unmap_page().
> 
> Well, with there already being a message logged down the call tree in case of
> error, I think the return value should simply be latched into a local variable,
> and nothing really be done with it (which should satisfy the compiler afaict,
> but it may be that Coverity would grumble about such). We really don't want
> to omit the __must_check just because there are a few cases where the error
> is of no further relevance; the annotation gets put there to make sure we
> catch all (current and future) cases where errors must not be ignored.
> 

Agreed. We really need to add __must_check annotation to iommu_map_page() / iommu_unmap_page().


Along with discussion of  a local variable for _for()_ loop,  I need to define this local variable into _while()_ loop:
e.g., as above case:
 
+while ( i-- )
+{
+    int iommu_ret = iommu_unmap_page(p2m->domain, gfn + i);
+
+    break;
+}

,  right?


But I really like this way:

+ int iommu_ret;
+
+while ( i-- )
+{
+    iommu_ret = iommu_unmap_page(p2m->domain, gfn + i);
+
+    break;
+}

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