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 01, 2016 6:24 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
> >>> On 31.05.16 at 15:57, <quan.xu@xxxxxxxxx> wrote:
> > --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> > +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> > @@ -295,12 +297,22 @@ static void __hwdom_init
> amd_iommu_hwdom_init(struct domain *d)
> >               * a pfn_valid() check would seem desirable here.
> >               */
> >              if ( mfn_valid(pfn) )
> > -                amd_iommu_map_page(d, pfn, pfn,
> > -                                   IOMMUF_readable|IOMMUF_writable);
> > +            {
> > +                int ret;
> > +
> > +                ret = amd_iommu_map_page(d, pfn, pfn,
> > +
> > + IOMMUF_readable|IOMMUF_writable);
> 
> Same here as for the earlier patch regarding assignment vs initializer.
> 

I'll fix it in next v7.

> But overall the entire change to this function seems to rather belong into
> patch 2.

Indeed.

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

these modifications should belong to patch 2.

> > --- 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.

> 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..
So I think I can remain R-b as is for patch 3. 

btw, your R-b is a very expensive tag to me, and I really don't want to drop 
it. :):)..

Quan



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