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

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Xu, Quan
> Sent: Wednesday, April 20, 2016 1:27 PM
> 
> On  April 19, 2016 2:44pm, Tian, Kevin <kevin.tian@xxxxxxxxx> 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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel