| Message ID | 1464703056-4741-5-git-send-email-quan.xu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> 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. But overall the entire change to this function seems to rather belong into patch 2. As would a respective change to vtd_set_hwdom_mapping(), which I'm not sure which patch you've put that in. > --- 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. The same then, btw., applies to patch 3, and hence I have to withdraw the R-b that you've got there. Jan
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
>>> On 02.06.16 at 09:25, <quan.xu@intel.com> wrote: > 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: > 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. > So I think I can remain R-b as is for patch 3. No. Jan
On June 02, 2016 5:21 PM, Jan Beulich <JBeulich@suse.com> wrote: > I really don't know what else I > could have done to make clear what exceptions are to be expected. Sorry, it may make you frustrated, but what you said is very clear. I really need to slow down and read your email along with code context carefully. Maybe I want to save time for that sriov PT bug. Sorry again. Quan
On June 02, 2016 5:21 PM, Jan Beulich <JBeulich@suse.com> wrote: > >>> On 02.06.16 at 09:25, <quan.xu@intel.com> wrote: > > 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: > > 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
>>> On 07.06.16 at 09:51, <quan.xu@intel.com> wrote: > 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(). Well, with there already being a message logged down the call tree in case of error, I think the return value should simply be latched into a local variable, and nothing really be done with it (which should satisfy the compiler afaict, but it may be that Coverity would grumble about such). We really don't want to omit the __must_check just because there are a few cases where the error is of no further relevance; the annotation gets put there to make sure we catch all (current and future) cases where errors must not be ignored. Jan
On June 07, 2016 4:19 PM, Jan Beulich <JBeulich@suse.com> wrote: > >>> On 07.06.16 at 09:51, <quan.xu@intel.com> wrote: > > 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(). > > Well, with there already being a message logged down the call tree in case of > error, I think the return value should simply be latched into a local variable, > and nothing really be done with it (which should satisfy the compiler afaict, > but it may be that Coverity would grumble about such). We really don't want > to omit the __must_check just because there are a few cases where the error > is of no further relevance; the annotation gets put there to make sure we > catch all (current and future) cases where errors must not be ignored. > Agreed. We really need to add __must_check annotation to iommu_map_page() / iommu_unmap_page(). Along with discussion of a local variable for _for()_ loop, I need to define this local variable into _while()_ loop: e.g., as above case: +while ( i-- ) +{ + int iommu_ret = iommu_unmap_page(p2m->domain, gfn + i); + + break; +} , right? But I really like this way: + int iommu_ret; + +while ( i-- ) +{ + iommu_ret = iommu_unmap_page(p2m->domain, gfn + i); + + break; +} Quan
>>> On 07.06.16 at 10:40, <quan.xu@intel.com> wrote: > Along with discussion of a local variable for _for()_ loop, I need to > define this local variable into _while()_ loop: > e.g., as above case: > > +while ( i-- ) > +{ > + int iommu_ret = iommu_unmap_page(p2m->domain, gfn + i); > + > + break; > +} > > , right? Something like this. As said, tricking Coverity into not reporting the unused value as a possible problem would additionally be needed. Andrew - do you know of any pre-existing pattern usable for this purpose? > But I really like this way: > > + int iommu_ret; > + > +while ( i-- ) > +{ > + iommu_ret = iommu_unmap_page(p2m->domain, gfn + i); > + > + break; > +} Rather not - iommu_ret isn't used outside of the while() scope. (Unless, of course, the Coverity related adjustment makes this desirable / necessary, but I would guess the tool would then still be able to figure that the assignment is pointless on all but the last iteration.) Jan
diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c index 70b7475..17c1f6d 100644 --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c @@ -285,6 +285,8 @@ static void __hwdom_init amd_iommu_hwdom_init(struct domain *d) if ( !iommu_passthrough && !need_iommu(d) ) { + int rc = 0; + /* Set up 1:1 page table for dom0 */ for ( i = 0; i < max_pdx; i++ ) { @@ -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); + if ( !rc ) + rc = ret; + } if ( !(i & 0xfffff) ) process_pending_softirqs(); } + + if ( rc ) + AMD_IOMMU_DEBUG("d%d: IOMMU mapping failed: %d\n", + d->domain_id, rc); } for_each_amd_iommu ( iommu ) diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c index 1ce4ddf..ee5c89d 100644 --- a/xen/drivers/passthrough/arm/smmu.c +++ b/xen/drivers/passthrough/arm/smmu.c @@ -2745,8 +2745,8 @@ static void arm_smmu_iommu_domain_teardown(struct domain *d) xfree(xen_domain); } -static int arm_smmu_map_page(struct domain *d, unsigned long gfn, - unsigned long mfn, unsigned int flags) +static int __must_check arm_smmu_map_page(struct domain *d, unsigned long gfn, + unsigned long mfn, unsigned int flags) { p2m_type_t t; diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c index 4844193..e900019 100644 --- a/xen/drivers/passthrough/vtd/iommu.c +++ b/xen/drivers/passthrough/vtd/iommu.c @@ -1687,9 +1687,10 @@ static void iommu_domain_teardown(struct domain *d) spin_unlock(&hd->arch.mapping_lock); } -static int intel_iommu_map_page( - struct domain *d, unsigned long gfn, unsigned long mfn, - unsigned int flags) +static int __must_check intel_iommu_map_page(struct domain *d, + unsigned long gfn, + unsigned long mfn, + unsigned int flags) { struct domain_iommu *hd = dom_iommu(d); struct dma_pte *page = NULL, *pte = NULL, old, new = { 0 }; diff --git a/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h b/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h index 57b6cc1..ac9f036 100644 --- a/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h @@ -51,8 +51,8 @@ int amd_iommu_init(void); int amd_iommu_update_ivrs_mapping_acpi(void); /* mapping functions */ -int amd_iommu_map_page(struct domain *d, unsigned long gfn, unsigned long mfn, - unsigned int flags); +int __must_check amd_iommu_map_page(struct domain *d, unsigned long gfn, + unsigned long mfn, unsigned int flags); int __must_check amd_iommu_unmap_page(struct domain *d, unsigned long gfn); u64 amd_iommu_get_next_table_from_pte(u32 *entry); int amd_iommu_reserve_domain_unity_map(struct domain *domain, diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h index 73a7f1e..14041a1 100644 --- 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); int __must_check (*unmap_page)(struct domain *d, unsigned long gfn); void (*free_page_table)(struct page_info *); #ifdef CONFIG_X86