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

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

On June 02, 2016 5:21 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
> >>> On 02.06.16 at 09:25, <quan.xu@xxxxxxxxx> wrote:
> > On June 01, 2016 6:24 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
> >> >>> On 31.05.16 at 15:57, <quan.xu@xxxxxxxxx> wrote:
> >  As would a respective change to vtd_set_hwdom_mapping(), which
> >> I'm not sure which patch you've put that in.
> >>
> >
> > Sorry,  I missed it. I indeed it need to fix it as similar as above.
> > I wonder whether I could add a __must_check annotation to
> > iommu_map_page() or not, as which may be inconsistent with
> iommu_unmap_page().
> 
> Urgh - are you saying that by the end of the series they aren't _both_
> __must_check? Then I must have overlooked something while reviewing: They
> definitely both ought to be. Or wait - I've pointed this out in the context 
> of this
> patch, still seen below.
> 
> >> > --- a/xen/include/xen/iommu.h
> >> > +++ b/xen/include/xen/iommu.h
> >> > @@ -166,8 +166,8 @@ struct iommu_ops {  #endif /* HAS_PCI */
> >> >
> >> >      void (*teardown)(struct domain *d);
> >> > -    int (*map_page)(struct domain *d, unsigned long gfn, unsigned long
> mfn,
> >> > -                    unsigned int flags);
> >> > +    int __must_check (*map_page)(struct domain *d, unsigned long gfn,
> >> > +                                 unsigned long mfn, unsigned int
> >> > + flags);
> >>
> >> With this and with the rule set forth in the context of the
> >> discussion of
> > v5,
> >> iommu_map_page() (as well as any other caller of this hook that do
> >> not themselves _consume_ the error [e.g. hwdom init ones]) should
> >> become or already be __must_check, which afaict isn't the case.
> >
> > But does this rule also apply to these 'void' annotation functions?
> > .e.g, in the call tree of hwdom init ones / domain crash ones, we are
> > no need to bubble up error code, leaving these void annotation as is.
> 
> Note that my previous reply already answered that question (as I expected
> you would otherwise ask): I specifically excluded those functions that
> _consume_ the error, and I did give an example. I really don't know what else 
> I
> could have done to make clear what exceptions are to be expected.
> 
> >> The same then, btw.,
> >> applies to patch 3, and hence I have to withdraw the R-b that you've
> >> got there.
> >
> > I find these callers are grant_table/mm, and we limit __must_check
> > annotation to IOMMU functions for this patch set..
> 
> Talk isn't of those ones. The subject of patch 3 is unmapping, and hence the
> parallel to the one here is that iommu_unmap_page() needs to become
> __must_check there, along with the iommu_ops
> unmap_page() hook.
> 


Jan,
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().

Quan

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