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

From: "Tian, Kevin" <kevin.tian@intel.com>
To: "Xu, Quan" <quan.xu@intel.com>,
	"xen-devel@lists.xen.org" <xen-devel@lists.xen.org>
Cc: Keir Fraser <keir@xen.org>, Jan Beulich <jbeulich@suse.com>,
	George Dunlap <george.dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	"dario.faggioli@citrix.com" <dario.faggioli@citrix.com>,
	"Nakajima, Jun" <jun.nakajima@intel.com>
Subject: Re: [PATCH v2 03/11] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping
Date: Wed, 20 Apr 2016 05:44:07 +0000	[thread overview]
Message-ID: <AADFC41AFE54684AB9EE6CBC0274A5D15F82385C@SHSMSX101.ccr.corp.intel.com> (raw)
In-Reply-To: <945CA011AD5F084CBEA3E851C0AB28894B88E02A@SHSMSX101.ccr.corp.intel.com>

> From: Xu, Quan
> Sent: Wednesday, April 20, 2016 1:27 PM
> 
> On  April 19, 2016 2:44pm, Tian, Kevin <kevin.tian@intel.com> wrote:
> > > From: Xu, Quan
> > > Sent: Monday, April 18, 2016 10:00 PM
> > >
> > > If IOMMU mapping and unmapping failed, the domain (with the exception
> > > of the hardware domain) is crashed, treated as a fatal error. Rollback
> > > can be lighter weight.
> >
> > What do you mean by 'lighter weight"? Please clarify it.
> >
> > >
> > > For the hardware domain, we do things on a best effort basis. When
> > > rollback is not feasible (in early initialization phase or trade-off
> > > of complexity), at least, unmap upon maps error or then throw out error
> > message.
> >
> > remove 'or'. Based on next sentence, is above specific for IOMMU mapping?
> >
> > >
> > > IOMMU unmapping should perhaps continue despite an error, in an
> > > attempt to do best effort cleanup.
> > >
> 
> 
> 
> Could I enhance the commit log as below:
> """
> If IOMMU mapping and unmapping failed, the domain (with the exception of the hardware
> domain) is crashed,
> treated as a fatal error. Rollback can be lighter weight (at least errors need to be
> propagated).

Still not clear about 'lighter weight'. Then what is the 'heavier weight' side then?
Isn't "at least errors need to be propagated" exactly what 'rollback' means?

> 
> IOMMU mapping breaks for an error, unmapping upon maps, throwing out error message
> and then reporting
> the error up 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 error
> message.

If above elaborates what earlier 'lighter weight" means, then please just
say a best-effort rollack.

> 
> IOMMU unmapping should perhaps continue despite an error, in an attempt to do best
> effort cleanup.
> """
> 
> 
> I am still not sure whether we really need throw out error message for each IOMMU
> mapping or not.
> If yes, I will throw out error message for each IOMMU mapping in next v3.

I'm not sure about your question here. Have to judge based on specific case.

> > >          {
> > >              if ( iommu_flags )
> > >                  for ( i = 0; i < (1 << order); i++ )
> > > -                    iommu_map_page(d, gfn + i, mfn_x(mfn) + i,
> > iommu_flags);
> > > +                {
> > > +                    ret = iommu_map_page(d, gfn + i, mfn_x(mfn) + i,
> > > + iommu_flags);
> > > +
> > > +                    if ( unlikely(ret) )
> > > +                    {
> > > +                        while (i)
> > > +                            iommu_unmap_page(d, gfn + --i);
> >
> > How about below?
> >
> > while (i-- >= 0)
> > 	iommu_unmap_page(d, gfn + i);
> >
> 
> this modification is based on discussion rooted at
> http://lists.xenproject.org/archives/html/xen-devel/2016-03/msg01779.html
> wait for Jan's decision.

Either way is OK. 'gfn + --I' just looks not very readable to me.

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