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

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

On May 10, 2016 11:00 PM, George Dunlap <George.Dunlap@eu.citrix.com> wrote:
> On Tue, May 10, 2016 at 3:45 PM, George Dunlap
> <George.Dunlap@eu.citrix.com> wrote:
> > On Tue, May 10, 2016 at 9:44 AM, Jan Beulich <JBeulich@suse.com> wrote:
> >>>>> On 06.05.16 at 10:54, <quan.xu@intel.com> 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.
> >>>
> >>> Signed-off-by: Quan Xu <quan.xu@intel.com>
> >>>
> >>> CC: Keir Fraser <keir@xen.org>
> >>> CC: Jan Beulich <jbeulich@suse.com>
> >>> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> >>> CC: Jun Nakajima <jun.nakajima@intel.com>
> >>> CC: Kevin Tian <kevin.tian@intel.com>
> >>> CC: George Dunlap <george.dunlap@eu.citrix.com>
> >>> ---
> >>
> >> Somewhere here I continue to miss a summary on what has changed
> >> compared to the previous version. For review especially of larger
> >> patches (where preferably one wouldn't want to re-review the entire
> >> thing) this is more than just a nice-to-have.
> >>
> >>> @@ -812,17 +813,22 @@ ept_set_entry(struct p2m_domain *p2m,
> unsigned long gfn, mfn_t mfn,
> >>>      rc = atomic_write_ept_entry(ept_entry, new_entry, target);
> >>>      if ( unlikely(rc) )
> >>>          old_entry.epte = 0;
> >>> -    else if ( p2mt != p2m_invalid &&
> >>> -              (gfn + (1UL << order) - 1 > p2m->max_mapped_pfn) )
> >>> -        /* Track the highest gfn for which we have ever had a valid mapping
> */
> >>> -        p2m->max_mapped_pfn = gfn + (1UL << order) - 1;
> >>> +    else
> >>> +    {
> >>> +        entry_written = 1;
> >>> +
> >>> +        if ( p2mt != p2m_invalid &&
> >>> +             (gfn + (1UL << order) - 1 > p2m->max_mapped_pfn) )
> >>> +            /* Track the highest gfn for which we have ever had a valid
> mapping */
> >>> +            p2m->max_mapped_pfn = gfn + (1UL << order) - 1;
> >>> +    }
> >>>
> >>>  out:
> >>>      if ( needs_sync )
> >>>          ept_sync_domain(p2m);
> >>>
> >>>      /* For host p2m, may need to change VT-d page table.*/
> >>> -    if ( rc == 0 && p2m_is_hostp2m(p2m) && need_iommu(d) &&
> >>> +    if ( entry_written && p2m_is_hostp2m(p2m) && need_iommu(d) &&
> >>>           need_modify_vtd_table )
> >>>      {
> >>
> >> I'd prefer this conditional to remain untouched, but I'll leave the
> >> decision to the maintainers of the file.
> >
> > Any particular reason you think it would be better untouched?
> >
> > I asked for it to be changed to "entry_written", because it seemed to
> > me that's what was actually wanted (i.e., you're checking whether rc
> > == 0 to determine whether the entry was written or not).  At the
> > moment the checks will be identical, but if someone changed something
> > later, rc might be non-zero even though the entry had been written, in
> > which case (I think) you'd want the iommu update to happen.
> >
> > It's not that big a deal to me, but I do prefer it this way (unless
> > I've misunderstood something).
> 
> See the discussion on patch 8 regarding why I now think Jan is probably right.
> 

Agreed. Thanks for your careful checking.

Check it again --
1. then I am no need to check 'rc' as below:

      if ( rc == 0 && p2m_is_hostp2m(p2m) && need_iommu(d) &&
           need_modify_vtd_table )
     {
+                        if ( !rc ) 
+                            rc = ret;
...

+                    if ( !rc && unlikely(ret) )
+                        rc = ret;
     }


2.  leave the below as is:

-    if ( rc == 0 && p2m_is_hostp2m(p2m) )
+    if ( entry_written && p2m_is_hostp2m(p2m) )

Quan

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