Re: [Xen-devel] [PATCH v4 03/10] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping

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

On May 10, 2016 10:45 PM, George Dunlap <George.Dunlap@xxxxxxxxxxxxx> wrote:
> On Tue, May 10, 2016 at 9:44 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
> >>>> On 06.05.16 at 10:54, <quan.xu@xxxxxxxxx> wrote:
> >> When IOMMU mapping is failed, we issue a best effort rollback,
> >> stopping IOMMU mapping, unmapping the previous IOMMU maps and
> then
> >> reporting the error up to the call trees. When rollback is not
> >> feasible (in early initialization phase or trade-off of complexity)
> >> for the hardware domain, we do things on a best effort basis, only
> throwing out an error message.
> >>
> >> IOMMU unmapping should perhaps continue despite an error, in an
> >> attempt to do best effort cleanup.
> >>
> >> --- a/xen/arch/x86/mm/p2m.c
> >> +++ b/xen/arch/x86/mm/p2m.c
> >> @@ -638,13 +638,20 @@ p2m_remove_page(struct p2m_domain *p2m,
> unsigned long gfn, unsigned long mfn,
> >>      mfn_t mfn_return;
> >>      p2m_type_t t;
> >>      p2m_access_t a;
> >> +    int rc = 0, ret;
> >>
> >>      if ( !paging_mode_translate(p2m->domain) )
> >>      {
> >>          if ( need_iommu(p2m->domain) )
> >>              for ( i = 0; i < (1 << page_order); i++ )
> >> -                iommu_unmap_page(p2m->domain, mfn + i);
> >> -        return 0;
> >> +            {
> >> +               ret = iommu_unmap_page(p2m->domain, mfn + i);
> >> +
> >> +               if ( !rc )
> >> +                   rc = ret;
> >> +            }
> >> +
> >> +        return rc;
> >>      }
> >
> > In code like this, btw., restricting the scope of "ret" to the
> > innermost block would help future readers see immediately that the
> > value of "ret" is of no further interest outside of that block.
> 
> I wouldn't ask for re-send just for this, but...
> 
> > Having reached the end of the patch, I'm missing the __must_check
> > additions that you said you would do in this new iteration. Is there
> > any reason for their absence? Did I overlook something?
> 
> If it's going to be re-sent anyway, moving the ret declaration inside the loop
> might as well be done.
> 
> Other than that, it looks good to me, thanks.
> 


I'll fix and re-send it in next v5.

Quan

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