| Message ID | 1463558911-98187-6-git-send-email-quan.xu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 18.05.16 at 10:08, <quan.xu@intel.com> wrote: > Propagate the IOMMU Device-TLB flush error up to IOMMU mapping. Btw - there's little reason to repeat the title here. > @@ -295,12 +297,23 @@ 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 ( unlikely(ret) ) > + rc = ret; > + } So you do the adjustment needed to add __must_check to amd_iommu_map_page(), but you don't actually add the annotation. Is there a reason for this? And of course the comment to an earlier patch applies regarding which error to return. Jan
On May 23, 2016 11:53 PM, Jan Beulich <JBeulich@suse.com> wrote: > >>> On 18.05.16 at 10:08, <quan.xu@intel.com> wrote: > > Propagate the IOMMU Device-TLB flush error up to IOMMU mapping. > > Btw - there's little reason to repeat the title here. > I'll drop it. Can I apply it to other patches? > > @@ -295,12 +297,23 @@ 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 ( unlikely(ret) ) > > + rc = ret; > > + } > > So you do the adjustment needed to add __must_check to > amd_iommu_map_page(), but you don't actually add the annotation. > Is there a reason for this? > Sorry, I missed it. I really need a __must_check to amd_iommu_map_page() in include/asm-x86/hvm/svm/amd-iommu-proto.h. > And of course the comment to an earlier patch applies regarding which error > to return. I'll apply it to all of my patch set. Quan
>>> On 24.05.16 at 11:01, <quan.xu@intel.com> wrote: > On May 23, 2016 11:53 PM, Jan Beulich <JBeulich@suse.com> wrote: >> >>> On 18.05.16 at 10:08, <quan.xu@intel.com> wrote: >> > Propagate the IOMMU Device-TLB flush error up to IOMMU mapping. >> >> Btw - there's little reason to repeat the title here. >> > > I'll drop it. > > Can I apply it to other patches? You mean dropping a description that simply repeats the title - yes, of course. Even better would of course be, where meaningful, if you added actual descriptions. Jan
On May 24, 2016 5:09 PM, Jan Beulich <JBeulich@suse.com> wrote: > >>> On 24.05.16 at 11:01, <quan.xu@intel.com> wrote: > > On May 23, 2016 11:53 PM, Jan Beulich <JBeulich@suse.com> wrote: > >> >>> On 18.05.16 at 10:08, <quan.xu@intel.com> wrote: > >> > Propagate the IOMMU Device-TLB flush error up to IOMMU mapping. > >> > >> Btw - there's little reason to repeat the title here. > >> > > > > I'll drop it. > > > > Can I apply it to other patches? > > You mean dropping a description that simply repeats the title - yes, of course. Yes, I think so too. > Even better would of course be, where meaningful, if you added actual > descriptions. > I am afraid I would make things worse, so I will just simply drop the description that simply repeats the title. Quan
diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c index 70b7475..86d6fb3 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,23 @@ 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 ( unlikely(ret) ) + rc = ret; + } if ( !(i & 0xfffff) ) process_pending_softirqs(); } + + if ( rc ) + AMD_IOMMU_DEBUG("d%d: IOMMU mapping failed %d.", + 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 29fb7fd..461688c 100644 --- a/xen/drivers/passthrough/vtd/iommu.c +++ b/xen/drivers/passthrough/vtd/iommu.c @@ -1724,13 +1724,15 @@ 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 }; u64 pg_maddr; + int rc = 0; /* Do nothing if VT-d shares EPT page table */ if ( iommu_use_hap_pt(d) ) @@ -1773,9 +1775,9 @@ static int intel_iommu_map_page( unmap_vtd_domain_page(page); if ( !this_cpu(iommu_dont_flush_iotlb) ) - __intel_iommu_iotlb_flush(d, gfn, dma_pte_present(old), 1); + rc = __intel_iommu_iotlb_flush(d, gfn, dma_pte_present(old), 1); - return 0; + return rc; } static int __must_check intel_iommu_unmap_page(struct domain *d, 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