| Message ID | 1454667507-79751-2-git-send-email-quan.xu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 05.02.16 at 11:18, <quan.xu@intel.com> wrote: > This patch checks all kinds of error and all the way up > the call trees of VT-d Device-TLB flush(IOMMU part). > > Signed-off-by: Quan Xu <quan.xu@intel.com> > --- > xen/drivers/passthrough/amd/iommu_init.c | 4 +- > xen/drivers/passthrough/amd/pci_amd_iommu.c | 4 +- > xen/drivers/passthrough/arm/smmu.c | 13 +-- > xen/drivers/passthrough/iommu.c | 37 +++++--- > xen/drivers/passthrough/vtd/extern.h | 4 +- > xen/drivers/passthrough/vtd/iommu.c | 125 ++++++++++++++++---------- > xen/drivers/passthrough/vtd/qinval.c | 2 +- > xen/drivers/passthrough/vtd/quirks.c | 26 +++--- > xen/drivers/passthrough/vtd/x86/vtd.c | 17 +++- > xen/drivers/passthrough/x86/iommu.c | 6 +- > xen/include/asm-x86/hvm/svm/amd-iommu-proto.h | 4 +- > xen/include/asm-x86/iommu.h | 2 +- > xen/include/xen/iommu.h | 20 ++--- > 13 files changed, 166 insertions(+), 98 deletions(-) Please be sure to Cc all maintainers of code you modify. > @@ -2737,8 +2739,9 @@ static int arm_smmu_iommu_domain_init(struct domain *d) > return 0; > } > > -static void __hwdom_init arm_smmu_iommu_hwdom_init(struct domain *d) > +static int __hwdom_init arm_smmu_iommu_hwdom_init(struct domain *d) > { > + return 0; > } Bad indentation. > --- a/xen/drivers/passthrough/iommu.c > +++ b/xen/drivers/passthrough/iommu.c > @@ -146,14 +146,15 @@ static void __hwdom_init check_hwdom_reqs(struct domain *d) > iommu_dom0_strict = 1; > } > > -void __hwdom_init iommu_hwdom_init(struct domain *d) > +int __hwdom_init iommu_hwdom_init(struct domain *d) > { > struct hvm_iommu *hd = domain_hvm_iommu(d); > + int rc = 0; Pointless initializer (more elsewhere). > @@ -171,7 +172,10 @@ void __hwdom_init iommu_hwdom_init(struct domain *d) > ((page->u.inuse.type_info & PGT_type_mask) > == PGT_writable_page) ) > mapping |= IOMMUF_writable; > - hd->platform_ops->map_page(d, gfn, mfn, mapping); > + rc = hd->platform_ops->map_page(d, gfn, mfn, mapping); > + if ( rc ) > + return rc; So in this patch I can't find error checking getting introduced for the caller of this function. And if you were to add it - what would your intentions be? Panic? IOW I'm not sure bailing here is a good idea. Logging an error message (but only once in this loop) would be a possibility. (This pattern repeats further down.) > @@ -369,12 +376,16 @@ void iommu_share_p2m_table(struct domain* d) > ops->share_p2m(d); > } > > -void iommu_crash_shutdown(void) > +int iommu_crash_shutdown(void) > { > const struct iommu_ops *ops = iommu_get_ops(); > + > if ( iommu_enabled ) > - ops->crash_shutdown(); > + return ops->crash_shutdown(); > + > iommu_enabled = iommu_intremap = 0; > + > + return 0; > } Here again the question is - what is the error value going to be used for? We're trying to shut down a crashed system when coming here. > @@ -584,37 +587,37 @@ static void __intel_iommu_iotlb_flush(struct domain *d, unsigned long gfn, > continue; > > if ( page_count > 1 || gfn == -1 ) > - { > - if ( iommu_flush_iotlb_dsi(iommu, iommu_domid, > - 0, flush_dev_iotlb) ) > - iommu_flush_write_buffer(iommu); > - } > + rc = iommu_flush_iotlb_dsi(iommu, iommu_domid, > + 0, flush_dev_iotlb); > else > - { > - if ( iommu_flush_iotlb_psi(iommu, iommu_domid, > + rc = iommu_flush_iotlb_psi(iommu, iommu_domid, > (paddr_t)gfn << PAGE_SHIFT_4K, 0, > - !dma_old_pte_present, flush_dev_iotlb) ) > - iommu_flush_write_buffer(iommu); > - } > + !dma_old_pte_present, flush_dev_iotlb); > + > + if ( rc ) > + iommu_flush_write_buffer(iommu); > } > + > + return rc; > } rc may be positive here (and afaict that's the indication to do the flush, not one of failure). Quite likely this code didn't mean to flush iommu_flush_iotlb_{d,p}si() returning a negative value... > @@ -1742,7 +1757,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); > + { > + return __intel_iommu_iotlb_flush(d, gfn, dma_pte_present(old), 1); > + } Pointless introduction of braces. > --- a/xen/drivers/passthrough/x86/iommu.c > +++ b/xen/drivers/passthrough/x86/iommu.c > @@ -104,7 +104,11 @@ int arch_iommu_populate_page_table(struct domain *d) > this_cpu(iommu_dont_flush_iotlb) = 0; > > if ( !rc ) > - iommu_iotlb_flush_all(d); > + { > + rc = iommu_iotlb_flush_all(d); > + if ( rc ) > + return rc; > + } But you leave all page tables set up? > else if ( rc != -ERESTART ) > iommu_teardown(d); I think you should let things reach this tear down in that case. Jan
> On February 11, 2016 at 1:01am, <JBeulich@suse.com> wrote: > >>> On 05.02.16 at 11:18, <quan.xu@intel.com> wrote: > > This patch checks all kinds of error and all the way up the call trees > > of VT-d Device-TLB flush(IOMMU part). > Please be sure to Cc all maintainers of code you modify. > OK, also CCed Dario Faggioli. Jan, could I re-send out the V5 and cc all maintainers of code I modify? > > @@ -2737,8 +2739,9 @@ static int arm_smmu_iommu_domain_init(struct > domain *d) > > return 0; > > } > > > > -static void __hwdom_init arm_smmu_iommu_hwdom_init(struct domain *d) > > +static int __hwdom_init arm_smmu_iommu_hwdom_init(struct domain *d) > > { > > + return 0; > > } > > Bad indentation. > I will modify it in next version. I tried to align with the coding style, as There was similar indentation nearby arm_smmu_iommu_hwdom_init() in that file. > > --- a/xen/drivers/passthrough/iommu.c > > +++ b/xen/drivers/passthrough/iommu.c > > @@ -146,14 +146,15 @@ static void __hwdom_init > check_hwdom_reqs(struct domain *d) > > iommu_dom0_strict = 1; > > } > > > > -void __hwdom_init iommu_hwdom_init(struct domain *d) > > +int __hwdom_init iommu_hwdom_init(struct domain *d) > > { > > struct hvm_iommu *hd = domain_hvm_iommu(d); > > + int rc = 0; > > Pointless initializer (more elsewhere). > I will modify them in next version. > > @@ -171,7 +172,10 @@ void __hwdom_init iommu_hwdom_init(struct > domain *d) > > ((page->u.inuse.type_info & PGT_type_mask) > > == PGT_writable_page) ) > > mapping |= IOMMUF_writable; > > - hd->platform_ops->map_page(d, gfn, mfn, mapping); > > + rc = hd->platform_ops->map_page(d, gfn, mfn, mapping); > > + if ( rc ) > > + return rc; > > So in this patch I can't find error checking getting introduced for the caller of > this function. And if you were to add it - what would your intentions be? Panic? > IOW I'm not sure bailing here is a good idea. Logging an error message (but only > once in this loop) would be a possibility. (This pattern repeats further down.) > Could I log an error message and break in this loop? > > @@ -369,12 +376,16 @@ void iommu_share_p2m_table(struct domain* d) > > ops->share_p2m(d); > > } > > > > -void iommu_crash_shutdown(void) > > +int iommu_crash_shutdown(void) > > { > > const struct iommu_ops *ops = iommu_get_ops(); > > + > > if ( iommu_enabled ) > > - ops->crash_shutdown(); > > + return ops->crash_shutdown(); > > + > > iommu_enabled = iommu_intremap = 0; > > + > > + return 0; > > } > > Here again the question is - what is the error value going to be used for? We're > trying to shut down a crashed system when coming here. > I tried to clean up in error handling path chained up. It logs an error message, When it calls iommu_crash_shutdown() and returns a non-zero value [in patch 2/7]. > > @@ -584,37 +587,37 @@ static void __intel_iommu_iotlb_flush(struct > domain *d, unsigned long gfn, > > continue; > > > > if ( page_count > 1 || gfn == -1 ) > > - { > > - if ( iommu_flush_iotlb_dsi(iommu, iommu_domid, > > - 0, flush_dev_iotlb) ) > > - iommu_flush_write_buffer(iommu); > > - } > > + rc = iommu_flush_iotlb_dsi(iommu, iommu_domid, > > + 0, flush_dev_iotlb); > > else > > - { > > - if ( iommu_flush_iotlb_psi(iommu, iommu_domid, > > + rc = iommu_flush_iotlb_psi(iommu, iommu_domid, > > (paddr_t)gfn << PAGE_SHIFT_4K, 0, > > - !dma_old_pte_present, flush_dev_iotlb) ) > > - iommu_flush_write_buffer(iommu); > > - } > > + !dma_old_pte_present, flush_dev_iotlb); > > + > > + if ( rc ) > > + iommu_flush_write_buffer(iommu); > > } > > + > > + return rc; > > } > > rc may be positive here (and afaict that's the indication to do the flush, not one > of failure). Quite likely this code didn't mean to flush > iommu_flush_iotlb_{d,p}si() returning a negative value... > > IIUC, you refer to the follow: flush_iotlb_qi( { ... if ( !cap_caching_mode(iommu->cap) ) return 1; ... } As Kevin mentioned, originally 0/1 is used to indicate whether caller needs to flush cache. But I try to return ZERO in [PATCH v5 1/7]. Because: 1) if It returns _1_, device passthrough doesn't work when I tried to clean up in error handling path chained up. 2) as VT-d spec said, Caching Mode is 0: invalidations are not required for modifications to individual not present or invalid entries. That is tricky for this point. Correct me if I am wrong. > > @@ -1742,7 +1757,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); > > + { > > + return __intel_iommu_iotlb_flush(d, gfn, dma_pte_present(old), > 1); > > + } > > Pointless introduction of braces. > I will modify it in next version. > > --- a/xen/drivers/passthrough/x86/iommu.c > > +++ b/xen/drivers/passthrough/x86/iommu.c > > @@ -104,7 +104,11 @@ int arch_iommu_populate_page_table(struct > domain *d) > > this_cpu(iommu_dont_flush_iotlb) = 0; > > > > if ( !rc ) > > - iommu_iotlb_flush_all(d); > > + { > > + rc = iommu_iotlb_flush_all(d); > > + if ( rc ) > > + return rc; > > + } > > But you leave all page tables set up? > > > else if ( rc != -ERESTART ) > > iommu_teardown(d); > > I think you should let things reach this tear down in that case. > Agreed. Jan, thanks for your comments. BTW, just for a check, did you only review patch v5 1/7 ? Hope I didn't miss your other comments. Quan
>>> On 16.02.16 at 11:50, <quan.xu@intel.com> wrote: >> On February 11, 2016 at 1:01am, <JBeulich@suse.com> wrote: >> >>> On 05.02.16 at 11:18, <quan.xu@intel.com> wrote: >> > This patch checks all kinds of error and all the way up the call trees >> > of VT-d Device-TLB flush(IOMMU part). > >> Please be sure to Cc all maintainers of code you modify. >> > OK, also CCed Dario Faggioli. > Jan, could I re-send out the V5 and cc all maintainers of code I modify? I'd say rather make sure you Cc the right people on v6, to avoid spamming everyone's mailboxes. >> > @@ -2737,8 +2739,9 @@ static int arm_smmu_iommu_domain_init(struct >> domain *d) >> > return 0; >> > } >> > >> > -static void __hwdom_init arm_smmu_iommu_hwdom_init(struct domain *d) >> > +static int __hwdom_init arm_smmu_iommu_hwdom_init(struct domain *d) >> > { >> > + return 0; >> > } >> >> Bad indentation. >> > > I will modify it in next version. I tried to align with the coding style, as > > There was similar indentation nearby arm_smmu_iommu_hwdom_init() in that > file. Then I'm afraid you didn't check enough of the properties of the file: It appears to use Linux style, and hence indentation is by tabs instead of spaces. >> > @@ -171,7 +172,10 @@ void __hwdom_init iommu_hwdom_init(struct >> domain *d) >> > ((page->u.inuse.type_info & PGT_type_mask) >> > == PGT_writable_page) ) >> > mapping |= IOMMUF_writable; >> > - hd->platform_ops->map_page(d, gfn, mfn, mapping); >> > + rc = hd->platform_ops->map_page(d, gfn, mfn, mapping); >> > + if ( rc ) >> > + return rc; >> >> So in this patch I can't find error checking getting introduced for the > caller of >> this function. And if you were to add it - what would your intentions be? > Panic? >> IOW I'm not sure bailing here is a good idea. Logging an error message (but > only >> once in this loop) would be a possibility. (This pattern repeats further > down.) >> > > Could I log an error message and break in this loop? Logging an error message - yes. Breaking the loop - no; this should continue to be a best effort attempt at getting things set up for Dom0. >> > @@ -369,12 +376,16 @@ void iommu_share_p2m_table(struct domain* d) >> > ops->share_p2m(d); >> > } >> > >> > -void iommu_crash_shutdown(void) >> > +int iommu_crash_shutdown(void) >> > { >> > const struct iommu_ops *ops = iommu_get_ops(); >> > + >> > if ( iommu_enabled ) >> > - ops->crash_shutdown(); >> > + return ops->crash_shutdown(); >> > + >> > iommu_enabled = iommu_intremap = 0; >> > + >> > + return 0; >> > } >> >> Here again the question is - what is the error value going to be used for? We're >> trying to shut down a crashed system when coming here. >> > I tried to clean up in error handling path chained up. It logs an error > message, > When it calls iommu_crash_shutdown() and returns a non-zero value [in patch > 2/7]. That sounds okay than (I didn't get around to look at patches 2-7 yet), but is somewhat contrary to me request of adding __must_check as far as possible, which - if done here - would break the build without also adjusting the caller(s). >> > @@ -584,37 +587,37 @@ static void __intel_iommu_iotlb_flush(struct >> domain *d, unsigned long gfn, >> > continue; >> > >> > if ( page_count > 1 || gfn == -1 ) >> > - { >> > - if ( iommu_flush_iotlb_dsi(iommu, iommu_domid, >> > - 0, flush_dev_iotlb) ) >> > - iommu_flush_write_buffer(iommu); >> > - } >> > + rc = iommu_flush_iotlb_dsi(iommu, iommu_domid, >> > + 0, flush_dev_iotlb); >> > else >> > - { >> > - if ( iommu_flush_iotlb_psi(iommu, iommu_domid, >> > + rc = iommu_flush_iotlb_psi(iommu, iommu_domid, >> > (paddr_t)gfn << PAGE_SHIFT_4K, 0, >> > - !dma_old_pte_present, flush_dev_iotlb) ) >> > - iommu_flush_write_buffer(iommu); >> > - } >> > + !dma_old_pte_present, flush_dev_iotlb); >> > + >> > + if ( rc ) >> > + iommu_flush_write_buffer(iommu); >> > } >> > + >> > + return rc; >> > } >> >> rc may be positive here (and afaict that's the indication to do the flush, > not one >> of failure). Quite likely this code didn't mean to flush >> iommu_flush_iotlb_{d,p}si() returning a negative value... >> >> > IIUC, you refer to the follow: > flush_iotlb_qi( > { > ... > if ( !cap_caching_mode(iommu->cap) ) > return 1; > ... > } > > > As Kevin mentioned, originally 0/1 is used to indicate whether caller needs > to flush cache. > But I try to return ZERO in [PATCH v5 1/7]. Because: > 1) if It returns _1_, device passthrough doesn't work when I tried to clean > up in error handling path chained up. > 2) as VT-d spec said, Caching Mode is 0: invalidations are not required for > modifications to individual not present > or invalid entries. > > That is tricky for this point. Correct me if I am wrong. I'm not sure I see the trickiness: All you need is - call iommu_flush_write_buffer() only when rc > 0 (if I remember the polarity right, else it might be rc == 0) - return zero from this function when rc is positive. Jan
> On February 16, 2016 7:06pm, <JBeulich@suse.com> wrote: > >>> On 16.02.16 at 11:50, <quan.xu@intel.com> wrote: > >> On February 11, 2016 at 1:01am, <JBeulich@suse.com> wrote: > >> >>> On 05.02.16 at 11:18, <quan.xu@intel.com> wrote: > >> > @@ -369,12 +376,16 @@ void iommu_share_p2m_table(struct domain* > d) > >> > ops->share_p2m(d); > >> > } > >> > > >> > -void iommu_crash_shutdown(void) > >> > +int iommu_crash_shutdown(void) > >> > { > >> > const struct iommu_ops *ops = iommu_get_ops(); > >> > + > >> > if ( iommu_enabled ) > >> > - ops->crash_shutdown(); > >> > + return ops->crash_shutdown(); > >> > + > >> > iommu_enabled = iommu_intremap = 0; > >> > + > >> > + return 0; > >> > } > >> > >> Here again the question is - what is the error value going to be used > >> for? We're trying to shut down a crashed system when coming here. > >> > > I tried to clean up in error handling path chained up. It logs an > > error message, When it calls iommu_crash_shutdown() and returns a > > non-zero value [in patch 2/7]. > > That sounds okay than (I didn't get around to look at patches 2-7 yet), but is > somewhat contrary to me request of adding __must_check as far as possible, > which - if done here - would break the build without also adjusting the caller(s). > If they are in the same patch set, I think it is acceptable. BTW, with patch 1/7, I can build Xen successfully( make xen ). To align this rule, I'd better merge patch1/7 and patch 2/7 into a large patch. Quan
>>> On 17.02.16 at 13:49, <quan.xu@intel.com> wrote: >> On February 16, 2016 7:06pm, <JBeulich@suse.com> wrote: >> >>> On 16.02.16 at 11:50, <quan.xu@intel.com> wrote: >> >> On February 11, 2016 at 1:01am, <JBeulich@suse.com> wrote: >> >> >>> On 05.02.16 at 11:18, <quan.xu@intel.com> wrote: > >> >> > @@ -369,12 +376,16 @@ void iommu_share_p2m_table(struct domain* >> d) >> >> > ops->share_p2m(d); >> >> > } >> >> > >> >> > -void iommu_crash_shutdown(void) >> >> > +int iommu_crash_shutdown(void) >> >> > { >> >> > const struct iommu_ops *ops = iommu_get_ops(); >> >> > + >> >> > if ( iommu_enabled ) >> >> > - ops->crash_shutdown(); >> >> > + return ops->crash_shutdown(); >> >> > + >> >> > iommu_enabled = iommu_intremap = 0; >> >> > + >> >> > + return 0; >> >> > } >> >> >> >> Here again the question is - what is the error value going to be used >> >> for? We're trying to shut down a crashed system when coming here. >> >> >> > I tried to clean up in error handling path chained up. It logs an >> > error message, When it calls iommu_crash_shutdown() and returns a >> > non-zero value [in patch 2/7]. >> >> That sounds okay than (I didn't get around to look at patches 2-7 yet), but > is >> somewhat contrary to me request of adding __must_check as far as possible, >> which - if done here - would break the build without also adjusting the > caller(s). >> > > > If they are in the same patch set, I think it is acceptable. If unavoidable, yes. But please remember that patch series don't necessarily get committed in one go. > BTW, with patch 1/7, I can build Xen successfully( make xen ). > To align this rule, I'd better merge patch1/7 and patch 2/7 into a large > patch. Not having looked at patch 2 yet (I'm about to), I can't tell whether this makes sense. I'd rather not see large patches become even larger though - please make a reasonable attempt at splitting things (in the case here for example by adjusting top level functions first, working your way down to leaf ones, thus guaranteeing that newly added __must_check annotations will be properly honored at each patch boundary). Jan
> On February 17, 2016 9:05pm, <JBeulich@suse.com> wrote: > >>> On 17.02.16 at 13:49, <quan.xu@intel.com> wrote: > >> On February 16, 2016 7:06pm, <JBeulich@suse.com> wrote: > >> >>> On 16.02.16 at 11:50, <quan.xu@intel.com> wrote: > >> >> On February 11, 2016 at 1:01am, <JBeulich@suse.com> wrote: > >> >> >>> On 05.02.16 at 11:18, <quan.xu@intel.com> wrote: > > > >> >> > @@ -369,12 +376,16 @@ void iommu_share_p2m_table(struct > domain* > >> d) > >> >> > ops->share_p2m(d); > >> >> > } > >> >> > > >> >> > -void iommu_crash_shutdown(void) > >> >> > +int iommu_crash_shutdown(void) > >> >> > { > >> >> > const struct iommu_ops *ops = iommu_get_ops(); > >> >> > + > >> >> > if ( iommu_enabled ) > >> >> > - ops->crash_shutdown(); > >> >> > + return ops->crash_shutdown(); > >> >> > + > >> >> > iommu_enabled = iommu_intremap = 0; > >> >> > + > >> >> > + return 0; > >> >> > } > >> >> > >> >> Here again the question is - what is the error value going to be > >> >> used for? We're trying to shut down a crashed system when coming here. > >> >> > >> > I tried to clean up in error handling path chained up. It logs an > >> > error message, When it calls iommu_crash_shutdown() and returns a > >> > non-zero value [in patch 2/7]. > >> > >> That sounds okay than (I didn't get around to look at patches 2-7 > >> yet), but > > is > >> somewhat contrary to me request of adding __must_check as far as > >> possible, which - if done here - would break the build without also > >> adjusting the > > caller(s). > >> > > > > > > If they are in the same patch set, I think it is acceptable. > > If unavoidable, yes. But please remember that patch series don't necessarily get > committed in one go. > > > BTW, with patch 1/7, I can build Xen successfully( make xen ). > > To align this rule, I'd better merge patch1/7 and patch 2/7 into a > > large patch. > > Not having looked at patch 2 yet > (I'm about to), Good news. :):) I think there are maybe some minor issues for this patch set ( _my_estimate_), and I will address these issues immediately. > I can't tell whether this makes > sense. I'd rather not see large patches become even larger though - please > make a reasonable attempt at splitting things (in the case here for example by > adjusting top level functions first, working your way down to leaf ones, thus > guaranteeing that newly added __must_check annotations will be properly > honored at each patch boundary). > Okay, I will try to do it in v6. Thanks. Quan
> From: Xu, Quan > Sent: Wednesday, February 17, 2016 9:38 PM > > > > > BTW, with patch 1/7, I can build Xen successfully( make xen ). > > > To align this rule, I'd better merge patch1/7 and patch 2/7 into a > > > large patch. > > > > Not having looked at patch 2 yet > > (I'm about to), > > Good news. :):) > I think there are maybe some minor issues for this patch set ( _my_estimate_), and I will > address these issues immediately. > Hi, Quan, are you expecting people to continue looking at your v5, or better to wait for v7? :-) Thanks Kevin
> On February 18, 2016 2:16pm, <Tian, Kevin> wrote: > > From: Xu, Quan > > Sent: Wednesday, February 17, 2016 9:38 PM > > > > > > > BTW, with patch 1/7, I can build Xen successfully( make xen ). > > > > To align this rule, I'd better merge patch1/7 and patch 2/7 into a > > > > large patch. > > > > > > Not having looked at patch 2 yet > > > (I'm about to), > > > > Good news. :):) > > I think there are maybe some minor issues for this patch set ( > > _my_estimate_), and I will address these issues immediately. > > > > Hi, Quan, are you expecting people to continue looking at your v5, or better to > wait for v7? :-) > V7? I think the other people would better wait for v6. I plan to send out v6 in next week. Jan gave some valuable comments for v5. I will fix it ASAP. I apologize to everyone who has to go through v5 patch set. I missed some v4 comments and didn't fully test the v5 patch set, as I tried my best to send out v5 on the last working day before Chinese Spring Festival and it was a long periods for v4 discussion. Quan
> From: Xu, Quan > Sent: Thursday, February 18, 2016 2:34 PM > > > On February 18, 2016 2:16pm, <Tian, Kevin> wrote: > > > From: Xu, Quan > > > Sent: Wednesday, February 17, 2016 9:38 PM > > > > > > > > > BTW, with patch 1/7, I can build Xen successfully( make xen ). > > > > > To align this rule, I'd better merge patch1/7 and patch 2/7 into a > > > > > large patch. > > > > > > > > Not having looked at patch 2 yet > > > > (I'm about to), > > > > > > Good news. :):) > > > I think there are maybe some minor issues for this patch set ( > > > _my_estimate_), and I will address these issues immediately. > > > > > > > Hi, Quan, are you expecting people to continue looking at your v5, or better to > > wait for v7? :-) > > > > V7? sorry. I meant v6. > I think the other people would better wait for v6. I plan to send out v6 in next week. > Jan gave some valuable comments for v5. I will fix it ASAP. > I apologize to everyone who has to go through v5 patch set. I missed some v4 > comments and didn't fully test the v5 patch set, as I tried my best to send out v5 on the > last > working day before Chinese Spring Festival and it was a long periods for v4 discussion. > Thanks for your work. Kevin
> On February 18, 2016 2:37pm, <Tian, Kevin> wrote: > > From: Xu, Quan > > Sent: Thursday, February 18, 2016 2:34 PM > Thanks for your work. > Kevin I appreciate your help!:) Quan
> On February 11, 2016 1:01am, Jan Beulich <JBeulich@suse.com> wrote: > >>> On 05.02.16 at 11:18, <quan.xu@intel.com> wrote: > > This patch checks all kinds of error and all the way up the call trees > > of VT-d Device-TLB flush(IOMMU part). > > @@ -171,7 +172,10 @@ void __hwdom_init iommu_hwdom_init(struct > domain *d) > > ((page->u.inuse.type_info & PGT_type_mask) > > == PGT_writable_page) ) > > mapping |= IOMMUF_writable; > > - hd->platform_ops->map_page(d, gfn, mfn, mapping); > > + rc = hd->platform_ops->map_page(d, gfn, mfn, mapping); > > + if ( rc ) > > + return rc; > > So in this patch I can't find error checking getting introduced for the caller of > this function. And if you were to add it - what would your intentions be? Panic? > IOW I'm not sure bailing here is a good idea. Logging an error message (but only > once in this loop) would be a possibility. (This pattern repeats further down.) > While I read the code/email again and again, I think I'd better _not_ return error value. Then the below modifications are pointless: 1. - void (*hwdom_init)(struct domain *d); + int (*hwdom_init)(struct domain *d); 2. -void iommu_hwdom_init(struct domain *d); +int iommu_hwdom_init(struct domain *d); > > @@ -369,12 +376,16 @@ void iommu_share_p2m_table(struct domain* d) > > ops->share_p2m(d); > > } > > > > -void iommu_crash_shutdown(void) > > +int iommu_crash_shutdown(void) > > { > > const struct iommu_ops *ops = iommu_get_ops(); > > + > > if ( iommu_enabled ) > > - ops->crash_shutdown(); > > + return ops->crash_shutdown(); > > + > > iommu_enabled = iommu_intremap = 0; > > + > > + return 0; > > } > > Here again the question is - what is the error value going to be used for? We're > trying to shut down a crashed system when coming here. > It is similar as the above case. I think I'd better _not_ return error value. Then the below modifications are pointless: 1. - void (*crash_shutdown)(void); + int (*crash_shutdown)(void); 2. -void amd_iommu_crash_shutdown(void); +int amd_iommu_crash_shutdown(void); 3. -void iommu_crash_shutdown(void); +int iommu_crash_shutdown(void); Right? Quan
>>> On 23.02.16 at 12:43, <quan.xu@intel.com> wrote: >> On February 11, 2016 1:01am, Jan Beulich <JBeulich@suse.com> wrote: >> >>> On 05.02.16 at 11:18, <quan.xu@intel.com> wrote: >> > This patch checks all kinds of error and all the way up the call trees >> > of VT-d Device-TLB flush(IOMMU part). >> > @@ -171,7 +172,10 @@ void __hwdom_init iommu_hwdom_init(struct >> domain *d) >> > ((page->u.inuse.type_info & PGT_type_mask) >> > == PGT_writable_page) ) >> > mapping |= IOMMUF_writable; >> > - hd->platform_ops->map_page(d, gfn, mfn, mapping); >> > + rc = hd->platform_ops->map_page(d, gfn, mfn, mapping); >> > + if ( rc ) >> > + return rc; >> >> So in this patch I can't find error checking getting introduced for the caller of >> this function. And if you were to add it - what would your intentions be? Panic? >> IOW I'm not sure bailing here is a good idea. Logging an error message (but only >> once in this loop) would be a possibility. (This pattern repeats further down.) >> > While I read the code/email again and again, > I think I'd better _not_ return error value. Then the below modifications > are pointless: > 1. > - void (*hwdom_init)(struct domain *d); > + int (*hwdom_init)(struct domain *d); > > 2. > -void iommu_hwdom_init(struct domain *d); > +int iommu_hwdom_init(struct domain *d); Agreed. Just log some message for failure in the case above, but make sure that is (a) useful for debugging and (b) not one message per page. >> > @@ -369,12 +376,16 @@ void iommu_share_p2m_table(struct domain* d) >> > ops->share_p2m(d); >> > } >> > >> > -void iommu_crash_shutdown(void) >> > +int iommu_crash_shutdown(void) >> > { >> > const struct iommu_ops *ops = iommu_get_ops(); >> > + >> > if ( iommu_enabled ) >> > - ops->crash_shutdown(); >> > + return ops->crash_shutdown(); >> > + >> > iommu_enabled = iommu_intremap = 0; >> > + >> > + return 0; >> > } >> >> Here again the question is - what is the error value going to be used for? We're >> trying to shut down a crashed system when coming here. >> > > It is similar as the above case. > I think I'd better _not_ return error value. Then the below modifications > are pointless: > 1. > - void (*crash_shutdown)(void); > + int (*crash_shutdown)(void); > > 2. > -void amd_iommu_crash_shutdown(void); > +int amd_iommu_crash_shutdown(void); > > 3. > -void iommu_crash_shutdown(void); > +int iommu_crash_shutdown(void); Indeed, same as above. Jan
diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c index d90a2d2..ec47e22 100644 --- a/xen/drivers/passthrough/amd/iommu_init.c +++ b/xen/drivers/passthrough/amd/iommu_init.c @@ -1340,12 +1340,14 @@ static void invalidate_all_devices(void) iterate_ivrs_mappings(_invalidate_all_devices); } -void amd_iommu_suspend(void) +int amd_iommu_suspend(void) { struct amd_iommu *iommu; for_each_amd_iommu ( iommu ) disable_iommu(iommu); + + return 0; } void amd_iommu_resume(void) diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c index c1c0b6b..449de13 100644 --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c @@ -280,7 +280,7 @@ static int amd_iommu_domain_init(struct domain *d) return 0; } -static void __hwdom_init amd_iommu_hwdom_init(struct domain *d) +static int __hwdom_init amd_iommu_hwdom_init(struct domain *d) { unsigned long i; const struct amd_iommu *iommu; @@ -312,6 +312,8 @@ static void __hwdom_init amd_iommu_hwdom_init(struct domain *d) BUG(); setup_hwdom_pci_devices(d, amd_iommu_setup_hwdom_device); + + return 0; } void amd_iommu_disable_domain_device(struct domain *domain, diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c index bb08827..155b7f3 100644 --- a/xen/drivers/passthrough/arm/smmu.c +++ b/xen/drivers/passthrough/arm/smmu.c @@ -2544,7 +2544,7 @@ static int force_stage = 2; */ static u32 platform_features = ARM_SMMU_FEAT_COHERENT_WALK; -static void arm_smmu_iotlb_flush_all(struct domain *d) +static int arm_smmu_iotlb_flush_all(struct domain *d) { struct arm_smmu_xen_domain *smmu_domain = domain_hvm_iommu(d)->arch.priv; struct iommu_domain *cfg; @@ -2561,13 +2561,15 @@ static void arm_smmu_iotlb_flush_all(struct domain *d) arm_smmu_tlb_inv_context(cfg->priv); } spin_unlock(&smmu_domain->lock); + + return 0; } -static void arm_smmu_iotlb_flush(struct domain *d, unsigned long gfn, - unsigned int page_count) +static int arm_smmu_iotlb_flush(struct domain *d, unsigned long gfn, + unsigned int page_count) { /* ARM SMMU v1 doesn't have flush by VMA and VMID */ - arm_smmu_iotlb_flush_all(d); + return arm_smmu_iotlb_flush_all(d); } static struct iommu_domain *arm_smmu_get_domain(struct domain *d, @@ -2737,8 +2739,9 @@ static int arm_smmu_iommu_domain_init(struct domain *d) return 0; } -static void __hwdom_init arm_smmu_iommu_hwdom_init(struct domain *d) +static int __hwdom_init arm_smmu_iommu_hwdom_init(struct domain *d) { + return 0; } static void arm_smmu_iommu_domain_teardown(struct domain *d) diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c index d5137733..cdf8e9a 100644 --- a/xen/drivers/passthrough/iommu.c +++ b/xen/drivers/passthrough/iommu.c @@ -146,14 +146,15 @@ static void __hwdom_init check_hwdom_reqs(struct domain *d) iommu_dom0_strict = 1; } -void __hwdom_init iommu_hwdom_init(struct domain *d) +int __hwdom_init iommu_hwdom_init(struct domain *d) { struct hvm_iommu *hd = domain_hvm_iommu(d); + int rc = 0; check_hwdom_reqs(d); if ( !iommu_enabled ) - return; + return 0; register_keyhandler('o', &iommu_p2m_table); d->need_iommu = !!iommu_dom0_strict; @@ -171,7 +172,10 @@ void __hwdom_init iommu_hwdom_init(struct domain *d) ((page->u.inuse.type_info & PGT_type_mask) == PGT_writable_page) ) mapping |= IOMMUF_writable; - hd->platform_ops->map_page(d, gfn, mfn, mapping); + rc = hd->platform_ops->map_page(d, gfn, mfn, mapping); + if ( rc ) + return rc; + if ( !(i++ & 0xfffff) ) process_pending_softirqs(); } @@ -266,24 +270,24 @@ static void iommu_free_pagetables(unsigned long unused) cpumask_cycle(smp_processor_id(), &cpu_online_map)); } -void iommu_iotlb_flush(struct domain *d, unsigned long gfn, unsigned int page_count) +int iommu_iotlb_flush(struct domain *d, unsigned long gfn, unsigned int page_count) { struct hvm_iommu *hd = domain_hvm_iommu(d); if ( !iommu_enabled || !hd->platform_ops || !hd->platform_ops->iotlb_flush ) - return; + return 0; - hd->platform_ops->iotlb_flush(d, gfn, page_count); + return hd->platform_ops->iotlb_flush(d, gfn, page_count); } -void iommu_iotlb_flush_all(struct domain *d) +int iommu_iotlb_flush_all(struct domain *d) { struct hvm_iommu *hd = domain_hvm_iommu(d); if ( !iommu_enabled || !hd->platform_ops || !hd->platform_ops->iotlb_flush_all ) - return; + return 0; - hd->platform_ops->iotlb_flush_all(d); + return hd->platform_ops->iotlb_flush_all(d); } int __init iommu_setup(void) @@ -354,11 +358,14 @@ int iommu_do_domctl( return ret; } -void iommu_suspend() +int iommu_suspend() { const struct iommu_ops *ops = iommu_get_ops(); + if ( iommu_enabled ) - ops->suspend(); + return ops->suspend(); + + return 0; } void iommu_share_p2m_table(struct domain* d) @@ -369,12 +376,16 @@ void iommu_share_p2m_table(struct domain* d) ops->share_p2m(d); } -void iommu_crash_shutdown(void) +int iommu_crash_shutdown(void) { const struct iommu_ops *ops = iommu_get_ops(); + if ( iommu_enabled ) - ops->crash_shutdown(); + return ops->crash_shutdown(); + iommu_enabled = iommu_intremap = 0; + + return 0; } int iommu_get_reserved_device_memory(iommu_grdm_t *func, void *ctxt) diff --git a/xen/drivers/passthrough/vtd/extern.h b/xen/drivers/passthrough/vtd/extern.h index 8acf889..ec9c513 100644 --- a/xen/drivers/passthrough/vtd/extern.h +++ b/xen/drivers/passthrough/vtd/extern.h @@ -91,11 +91,11 @@ int is_igd_vt_enabled_quirk(void); void platform_quirks_init(void); void vtd_ops_preamble_quirk(struct iommu* iommu); void vtd_ops_postamble_quirk(struct iommu* iommu); -void me_wifi_quirk(struct domain *domain, u8 bus, u8 devfn, int map); +int me_wifi_quirk(struct domain *domain, u8 bus, u8 devfn, int map); void pci_vtd_quirk(const struct pci_dev *); int platform_supports_intremap(void); int platform_supports_x2apic(void); -void vtd_set_hwdom_mapping(struct domain *d); +int vtd_set_hwdom_mapping(struct domain *d); #endif // _VTD_EXTERN_H_ diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c index dd13865..a780632 100644 --- a/xen/drivers/passthrough/vtd/iommu.c +++ b/xen/drivers/passthrough/vtd/iommu.c @@ -542,7 +542,7 @@ static int iommu_flush_iotlb_psi( return status; } -static void iommu_flush_all(void) +static int iommu_flush_all(void) { struct acpi_drhd_unit *drhd; struct iommu *iommu; @@ -554,11 +554,13 @@ static void iommu_flush_all(void) iommu = drhd->iommu; iommu_flush_context_global(iommu, 0); flush_dev_iotlb = find_ats_dev_drhd(iommu) ? 1 : 0; - iommu_flush_iotlb_global(iommu, 0, flush_dev_iotlb); + return iommu_flush_iotlb_global(iommu, 0, flush_dev_iotlb); } + + return 0; } -static void __intel_iommu_iotlb_flush(struct domain *d, unsigned long gfn, +static int __intel_iommu_iotlb_flush(struct domain *d, unsigned long gfn, int dma_old_pte_present, unsigned int page_count) { struct hvm_iommu *hd = domain_hvm_iommu(d); @@ -566,6 +568,7 @@ static void __intel_iommu_iotlb_flush(struct domain *d, unsigned long gfn, struct iommu *iommu; int flush_dev_iotlb; int iommu_domid; + int rc = 0; /* * No need pcideves_lock here because we have flush @@ -584,37 +587,37 @@ static void __intel_iommu_iotlb_flush(struct domain *d, unsigned long gfn, continue; if ( page_count > 1 || gfn == -1 ) - { - if ( iommu_flush_iotlb_dsi(iommu, iommu_domid, - 0, flush_dev_iotlb) ) - iommu_flush_write_buffer(iommu); - } + rc = iommu_flush_iotlb_dsi(iommu, iommu_domid, + 0, flush_dev_iotlb); else - { - if ( iommu_flush_iotlb_psi(iommu, iommu_domid, + rc = iommu_flush_iotlb_psi(iommu, iommu_domid, (paddr_t)gfn << PAGE_SHIFT_4K, 0, - !dma_old_pte_present, flush_dev_iotlb) ) - iommu_flush_write_buffer(iommu); - } + !dma_old_pte_present, flush_dev_iotlb); + + if ( rc ) + iommu_flush_write_buffer(iommu); } + + return rc; } -static void intel_iommu_iotlb_flush(struct domain *d, unsigned long gfn, unsigned int page_count) +static int intel_iommu_iotlb_flush(struct domain *d, unsigned long gfn, unsigned int page_count) { - __intel_iommu_iotlb_flush(d, gfn, 1, page_count); + return __intel_iommu_iotlb_flush(d, gfn, 1, page_count); } -static void intel_iommu_iotlb_flush_all(struct domain *d) +static int intel_iommu_iotlb_flush_all(struct domain *d) { - __intel_iommu_iotlb_flush(d, 0, 0, 0); + return __intel_iommu_iotlb_flush(d, 0, 0, 0); } /* clear one page's page table */ -static void dma_pte_clear_one(struct domain *domain, u64 addr) +static int dma_pte_clear_one(struct domain *domain, u64 addr) { struct hvm_iommu *hd = domain_hvm_iommu(domain); struct dma_pte *page = NULL, *pte = NULL; u64 pg_maddr; + int rc = 0; spin_lock(&hd->arch.mapping_lock); /* get last level pte */ @@ -622,7 +625,7 @@ static void dma_pte_clear_one(struct domain *domain, u64 addr) if ( pg_maddr == 0 ) { spin_unlock(&hd->arch.mapping_lock); - return; + return -ENOMEM; } page = (struct dma_pte *)map_vtd_domain_page(pg_maddr); @@ -632,7 +635,7 @@ static void dma_pte_clear_one(struct domain *domain, u64 addr) { spin_unlock(&hd->arch.mapping_lock); unmap_vtd_domain_page(page); - return; + return 0; } dma_clear_pte(*pte); @@ -640,9 +643,11 @@ static void dma_pte_clear_one(struct domain *domain, u64 addr) iommu_flush_cache_entry(pte, sizeof(struct dma_pte)); if ( !this_cpu(iommu_dont_flush_iotlb) ) - __intel_iommu_iotlb_flush(domain, addr >> PAGE_SHIFT_4K, 1, 1); + rc = __intel_iommu_iotlb_flush(domain, addr >> PAGE_SHIFT_4K, 1, 1); unmap_vtd_domain_page(page); + + return rc; } static void iommu_free_pagetable(u64 pt_maddr, int level) @@ -1251,20 +1256,25 @@ static int intel_iommu_domain_init(struct domain *d) return 0; } -static void __hwdom_init intel_iommu_hwdom_init(struct domain *d) +static int __hwdom_init intel_iommu_hwdom_init(struct domain *d) { struct acpi_drhd_unit *drhd; + int rc; if ( !iommu_passthrough && !need_iommu(d) ) { /* Set up 1:1 page table for hardware domain. */ - vtd_set_hwdom_mapping(d); + rc = vtd_set_hwdom_mapping(d); + if ( rc ) + return rc; } setup_hwdom_pci_devices(d, setup_hwdom_device); setup_hwdom_rmrr(d); - iommu_flush_all(); + rc = iommu_flush_all(); + if ( rc ) + return rc; for_each_drhd_unit ( drhd ) { @@ -1273,6 +1283,8 @@ static void __hwdom_init intel_iommu_hwdom_init(struct domain *d) BUG(); iommu_enable_translation(drhd); } + + return 0; } int domain_context_mapping_one( @@ -1285,6 +1297,7 @@ int domain_context_mapping_one( u64 maddr, pgd_maddr; u16 seg = iommu->intel->drhd->segment; int agaw; + int rc = 0; ASSERT(spin_is_locked(&pcidevs_lock)); spin_lock(&iommu->lock); @@ -1404,17 +1417,18 @@ int domain_context_mapping_one( else { int flush_dev_iotlb = find_ats_dev_drhd(iommu) ? 1 : 0; - iommu_flush_iotlb_dsi(iommu, 0, 1, flush_dev_iotlb); + + rc = iommu_flush_iotlb_dsi(iommu, 0, 1, flush_dev_iotlb); } set_bit(iommu->index, &hd->arch.iommu_bitmap); unmap_vtd_domain_page(context_entries); - if ( !seg ) - me_wifi_quirk(domain, bus, devfn, MAP_ME_PHANTOM_FUNC); + if ( !rc && !seg ) + rc = me_wifi_quirk(domain, bus, devfn, MAP_ME_PHANTOM_FUNC); - return 0; + return rc; } static int domain_context_mapping( @@ -1509,6 +1523,7 @@ int domain_context_unmap_one( struct context_entry *context, *context_entries; u64 maddr; int iommu_domid; + int rc = 0; ASSERT(spin_is_locked(&pcidevs_lock)); spin_lock(&iommu->lock); @@ -1543,16 +1558,16 @@ int domain_context_unmap_one( else { int flush_dev_iotlb = find_ats_dev_drhd(iommu) ? 1 : 0; - iommu_flush_iotlb_dsi(iommu, iommu_domid, 0, flush_dev_iotlb); + rc = iommu_flush_iotlb_dsi(iommu, iommu_domid, 0, flush_dev_iotlb); } spin_unlock(&iommu->lock); unmap_vtd_domain_page(context_entries); - if ( !iommu->intel->drhd->segment ) - me_wifi_quirk(domain, bus, devfn, UNMAP_ME_PHANTOM_FUNC); + if ( !rc && !iommu->intel->drhd->segment ) + rc = me_wifi_quirk(domain, bus, devfn, UNMAP_ME_PHANTOM_FUNC); - return 0; + return rc; } static int domain_context_unmap( @@ -1742,7 +1757,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); + { + return __intel_iommu_iotlb_flush(d, gfn, dma_pte_present(old), 1); + } return 0; } @@ -1753,19 +1770,18 @@ static int intel_iommu_unmap_page(struct domain *d, unsigned long gfn) if ( iommu_passthrough && is_hardware_domain(d) ) return 0; - dma_pte_clear_one(d, (paddr_t)gfn << PAGE_SHIFT_4K); - - return 0; + return dma_pte_clear_one(d, (paddr_t)gfn << PAGE_SHIFT_4K); } -void iommu_pte_flush(struct domain *d, u64 gfn, u64 *pte, - int order, int present) +int iommu_pte_flush(struct domain *d, u64 gfn, u64 *pte, + int order, int present) { struct acpi_drhd_unit *drhd; struct iommu *iommu = NULL; struct hvm_iommu *hd = domain_hvm_iommu(d); int flush_dev_iotlb; int iommu_domid; + int rc = 0; iommu_flush_cache_entry(pte, sizeof(struct dma_pte)); @@ -1779,11 +1795,14 @@ void iommu_pte_flush(struct domain *d, u64 gfn, u64 *pte, iommu_domid= domain_iommu_domid(d, iommu); if ( iommu_domid == -1 ) continue; - if ( iommu_flush_iotlb_psi(iommu, iommu_domid, + rc = iommu_flush_iotlb_psi(iommu, iommu_domid, (paddr_t)gfn << PAGE_SHIFT_4K, - order, !present, flush_dev_iotlb) ) + order, !present, flush_dev_iotlb); + if ( rc ) iommu_flush_write_buffer(iommu); } + + return rc; } static int __init vtd_ept_page_compatible(struct iommu *iommu) @@ -2103,8 +2122,8 @@ static int init_vtd_hw(void) return -EIO; } } - iommu_flush_all(); - return 0; + + return iommu_flush_all(); } static void __hwdom_init setup_hwdom_rmrr(struct domain *d) @@ -2372,16 +2391,19 @@ static int intel_iommu_group_id(u16 seg, u8 bus, u8 devfn) } static u32 iommu_state[MAX_IOMMUS][MAX_IOMMU_REGS]; -static void vtd_suspend(void) +static int vtd_suspend(void) { struct acpi_drhd_unit *drhd; struct iommu *iommu; + int rc; u32 i; if ( !iommu_enabled ) - return; + return 0; - iommu_flush_all(); + rc = iommu_flush_all(); + if ( rc ) + return rc; for_each_drhd_unit ( drhd ) { @@ -2410,17 +2432,22 @@ static void vtd_suspend(void) if ( !iommu_intremap && iommu_qinval ) disable_qinval(iommu); } + + return 0; } -static void vtd_crash_shutdown(void) +static int vtd_crash_shutdown(void) { struct acpi_drhd_unit *drhd; struct iommu *iommu; + int rc; if ( !iommu_enabled ) - return; + return 0; - iommu_flush_all(); + rc = iommu_flush_all(); + if ( rc ) + return rc; for_each_drhd_unit ( drhd ) { @@ -2429,6 +2456,8 @@ static void vtd_crash_shutdown(void) disable_intremap(drhd->iommu); disable_qinval(drhd->iommu); } + + return 0; } static void vtd_resume(void) diff --git a/xen/drivers/passthrough/vtd/qinval.c b/xen/drivers/passthrough/vtd/qinval.c index b81b0bd..946e812 100644 --- a/xen/drivers/passthrough/vtd/qinval.c +++ b/xen/drivers/passthrough/vtd/qinval.c @@ -324,7 +324,7 @@ static int flush_iotlb_qi( if ( flush_non_present_entry ) { if ( !cap_caching_mode(iommu->cap) ) - return 1; + return 0; else did = 0; } diff --git a/xen/drivers/passthrough/vtd/quirks.c b/xen/drivers/passthrough/vtd/quirks.c index 1888843..d9aea7e 100644 --- a/xen/drivers/passthrough/vtd/quirks.c +++ b/xen/drivers/passthrough/vtd/quirks.c @@ -332,10 +332,11 @@ void __init platform_quirks_init(void) * assigning Intel integrated wifi device to a guest. */ -static void map_me_phantom_function(struct domain *domain, u32 dev, int map) +static int map_me_phantom_function(struct domain *domain, u32 dev, int map) { struct acpi_drhd_unit *drhd; struct pci_dev *pdev; + int rc = 0; /* find ME VT-d engine base on a real ME device */ pdev = pci_get_pdev(0, 0, PCI_DEVFN(dev, 0)); @@ -343,23 +344,26 @@ static void map_me_phantom_function(struct domain *domain, u32 dev, int map) /* map or unmap ME phantom function */ if ( map ) - domain_context_mapping_one(domain, drhd->iommu, 0, - PCI_DEVFN(dev, 7), NULL); + rc = domain_context_mapping_one(domain, drhd->iommu, 0, + PCI_DEVFN(dev, 7), NULL); else - domain_context_unmap_one(domain, drhd->iommu, 0, - PCI_DEVFN(dev, 7)); + rc = domain_context_unmap_one(domain, drhd->iommu, 0, + PCI_DEVFN(dev, 7)); + + return rc; } -void me_wifi_quirk(struct domain *domain, u8 bus, u8 devfn, int map) +int me_wifi_quirk(struct domain *domain, u8 bus, u8 devfn, int map) { u32 id; + int rc = 0; id = pci_conf_read32(0, 0, 0, 0, 0); if ( IS_CTG(id) ) { /* quit if ME does not exist */ if ( pci_conf_read32(0, 0, 3, 0, 0) == 0xffffffff ) - return; + return -ENOENT; /* if device is WLAN device, map ME phantom device 0:3.7 */ id = pci_conf_read32(0, bus, PCI_SLOT(devfn), PCI_FUNC(devfn), 0); @@ -373,7 +377,7 @@ void me_wifi_quirk(struct domain *domain, u8 bus, u8 devfn, int map) case 0x423b8086: case 0x423c8086: case 0x423d8086: - map_me_phantom_function(domain, 3, map); + rc = map_me_phantom_function(domain, 3, map); break; default: break; @@ -383,7 +387,7 @@ void me_wifi_quirk(struct domain *domain, u8 bus, u8 devfn, int map) { /* quit if ME does not exist */ if ( pci_conf_read32(0, 0, 22, 0, 0) == 0xffffffff ) - return; + return -ENOENT; /* if device is WLAN device, map ME phantom device 0:22.7 */ id = pci_conf_read32(0, bus, PCI_SLOT(devfn), PCI_FUNC(devfn), 0); @@ -399,12 +403,14 @@ void me_wifi_quirk(struct domain *domain, u8 bus, u8 devfn, int map) case 0x42388086: /* Puma Peak */ case 0x422b8086: case 0x422c8086: - map_me_phantom_function(domain, 22, map); + rc = map_me_phantom_function(domain, 22, map); break; default: break; } } + + return rc; } void pci_vtd_quirk(const struct pci_dev *pdev) diff --git a/xen/drivers/passthrough/vtd/x86/vtd.c b/xen/drivers/passthrough/vtd/x86/vtd.c index c0d6aab..a19177c 100644 --- a/xen/drivers/passthrough/vtd/x86/vtd.c +++ b/xen/drivers/passthrough/vtd/x86/vtd.c @@ -108,9 +108,10 @@ void hvm_dpci_isairq_eoi(struct domain *d, unsigned int isairq) spin_unlock(&d->event_lock); } -void __hwdom_init vtd_set_hwdom_mapping(struct domain *d) +int __hwdom_init vtd_set_hwdom_mapping(struct domain *d) { unsigned long i, j, tmp, top; + int rc = 0; BUG_ON(!is_hardware_domain(d)); @@ -140,11 +141,21 @@ void __hwdom_init vtd_set_hwdom_mapping(struct domain *d) tmp = 1 << (PAGE_SHIFT - PAGE_SHIFT_4K); for ( j = 0; j < tmp; j++ ) - iommu_map_page(d, pfn * tmp + j, pfn * tmp + j, - IOMMUF_readable|IOMMUF_writable); + { + rc = iommu_map_page(d, pfn * tmp + j, pfn * tmp + j, + IOMMUF_readable|IOMMUF_writable); + if ( rc ) + { + while ( j-- > 0 ) + iommu_unmap_page(d, pfn * tmp + j); + break; + } + } if (!(i & (0xfffff >> (PAGE_SHIFT - PAGE_SHIFT_4K)))) process_pending_softirqs(); } + + return rc; } diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c index 8cbb655..6674fb0 100644 --- a/xen/drivers/passthrough/x86/iommu.c +++ b/xen/drivers/passthrough/x86/iommu.c @@ -104,7 +104,11 @@ int arch_iommu_populate_page_table(struct domain *d) this_cpu(iommu_dont_flush_iotlb) = 0; if ( !rc ) - iommu_iotlb_flush_all(d); + { + rc = iommu_iotlb_flush_all(d); + if ( rc ) + return rc; + } else if ( rc != -ERESTART ) iommu_teardown(d); 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 9c51172..4691f9b 100644 --- a/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h @@ -119,8 +119,8 @@ extern unsigned long *shared_intremap_inuse; /* power management support */ void amd_iommu_resume(void); -void amd_iommu_suspend(void); -void amd_iommu_crash_shutdown(void); +int amd_iommu_suspend(void); +int amd_iommu_crash_shutdown(void); /* guest iommu support */ void amd_iommu_send_guest_cmd(struct amd_iommu *iommu, u32 cmd[]); diff --git a/xen/include/asm-x86/iommu.h b/xen/include/asm-x86/iommu.h index 29203d7..cf2a269 100644 --- a/xen/include/asm-x86/iommu.h +++ b/xen/include/asm-x86/iommu.h @@ -26,7 +26,7 @@ int iommu_setup_hpet_msi(struct msi_desc *); /* While VT-d specific, this must get declared in a generic header. */ int adjust_vtd_irq_affinities(void); -void iommu_pte_flush(struct domain *d, u64 gfn, u64 *pte, int order, int present); +int iommu_pte_flush(struct domain *d, u64 gfn, u64 *pte, int order, int present); int iommu_supports_eim(void); int iommu_enable_x2apic_IR(void); void iommu_disable_x2apic_IR(void); diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h index 8f3a20e..f5b6f7e 100644 --- a/xen/include/xen/iommu.h +++ b/xen/include/xen/iommu.h @@ -55,7 +55,7 @@ int iommu_add_device(struct pci_dev *pdev); int iommu_enable_device(struct pci_dev *pdev); int iommu_remove_device(struct pci_dev *pdev); int iommu_domain_init(struct domain *d); -void iommu_hwdom_init(struct domain *d); +int iommu_hwdom_init(struct domain *d); void iommu_domain_destroy(struct domain *d); int deassign_device(struct domain *d, u16 seg, u8 bus, u8 devfn); @@ -134,7 +134,7 @@ typedef int iommu_grdm_t(xen_pfn_t start, xen_ulong_t nr, u32 id, void *ctxt); struct iommu_ops { int (*init)(struct domain *d); - void (*hwdom_init)(struct domain *d); + int (*hwdom_init)(struct domain *d); int (*add_device)(u8 devfn, device_t *dev); int (*enable_device)(device_t *dev); int (*remove_device)(u8 devfn, device_t *dev); @@ -157,19 +157,19 @@ struct iommu_ops { unsigned int (*read_apic_from_ire)(unsigned int apic, unsigned int reg); int (*setup_hpet_msi)(struct msi_desc *); #endif /* CONFIG_X86 */ - void (*suspend)(void); + int (*suspend)(void); void (*resume)(void); void (*share_p2m)(struct domain *d); - void (*crash_shutdown)(void); - void (*iotlb_flush)(struct domain *d, unsigned long gfn, unsigned int page_count); - void (*iotlb_flush_all)(struct domain *d); + int (*crash_shutdown)(void); + int (*iotlb_flush)(struct domain *d, unsigned long gfn, unsigned int page_count); + int (*iotlb_flush_all)(struct domain *d); int (*get_reserved_device_memory)(iommu_grdm_t *, void *); void (*dump_p2m_table)(struct domain *d); }; -void iommu_suspend(void); +int iommu_suspend(void); void iommu_resume(void); -void iommu_crash_shutdown(void); +int iommu_crash_shutdown(void); int iommu_get_reserved_device_memory(iommu_grdm_t *, void *); void iommu_share_p2m_table(struct domain *d); @@ -182,8 +182,8 @@ int iommu_do_pci_domctl(struct xen_domctl *, struct domain *d, int iommu_do_domctl(struct xen_domctl *, struct domain *d, XEN_GUEST_HANDLE_PARAM(xen_domctl_t)); -void iommu_iotlb_flush(struct domain *d, unsigned long gfn, unsigned int page_count); -void iommu_iotlb_flush_all(struct domain *d); +int iommu_iotlb_flush(struct domain *d, unsigned long gfn, unsigned int page_count); +int iommu_iotlb_flush_all(struct domain *d); /* * The purpose of the iommu_dont_flush_iotlb optional cpu flag is to
This patch checks all kinds of error and all the way up the call trees of VT-d Device-TLB flush(IOMMU part). Signed-off-by: Quan Xu <quan.xu@intel.com> --- xen/drivers/passthrough/amd/iommu_init.c | 4 +- xen/drivers/passthrough/amd/pci_amd_iommu.c | 4 +- xen/drivers/passthrough/arm/smmu.c | 13 +-- xen/drivers/passthrough/iommu.c | 37 +++++--- xen/drivers/passthrough/vtd/extern.h | 4 +- xen/drivers/passthrough/vtd/iommu.c | 125 ++++++++++++++++---------- xen/drivers/passthrough/vtd/qinval.c | 2 +- xen/drivers/passthrough/vtd/quirks.c | 26 +++--- xen/drivers/passthrough/vtd/x86/vtd.c | 17 +++- xen/drivers/passthrough/x86/iommu.c | 6 +- xen/include/asm-x86/hvm/svm/amd-iommu-proto.h | 4 +- xen/include/asm-x86/iommu.h | 2 +- xen/include/xen/iommu.h | 20 ++--- 13 files changed, 166 insertions(+), 98 deletions(-)