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

From: "Xu, Quan" <quan.xu@intel.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: "Tian, Kevin" <kevin.tian@intel.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	"Wu, Feng" <feng.wu@intel.com>,
	"dario.faggioli@citrix.com" <dario.faggioli@citrix.com>,
	"xen-devel@lists.xen.org" <xen-devel@lists.xen.org>,
	Julien Grall <julien.grall@arm.com>,
	Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Subject: Re: [Patch v6 04/11] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU mapping (top level ones)
Date: Thu, 2 Jun 2016 07:25:11 +0000	[thread overview]
Message-ID: <945CA011AD5F084CBEA3E851C0AB28894B8C4A4D@SHSMSX101.ccr.corp.intel.com> (raw)
In-Reply-To: <574ED3E202000078000F0532@prv-mh.provo.novell.com>

On June 01, 2016 6:24 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 31.05.16 at 15:57, <quan.xu@intel.com> 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@lists.xen.org
http://lists.xen.org/xen-devel