| Message ID | 1462524880-67205-4-git-send-email-quan.xu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote: > When IOMMU mapping is failed, we issue a best effort rollback, stopping > IOMMU mapping, unmapping the previous IOMMU maps and then reporting the > error up to the call trees. When rollback is not feasible (in early > initialization phase or trade-off of complexity) for the hardware domain, > we do things on a best effort basis, only throwing out an error message. > > IOMMU unmapping should perhaps continue despite an error, in an attempt > to do best effort cleanup. > > Signed-off-by: Quan Xu <quan.xu@intel.com> > > CC: Keir Fraser <keir@xen.org> > CC: Jan Beulich <jbeulich@suse.com> > CC: Andrew Cooper <andrew.cooper3@citrix.com> > CC: Jun Nakajima <jun.nakajima@intel.com> > CC: Kevin Tian <kevin.tian@intel.com> > CC: George Dunlap <george.dunlap@eu.citrix.com> > --- Somewhere here I continue to miss a summary on what has changed compared to the previous version. For review especially of larger patches (where preferably one wouldn't want to re-review the entire thing) this is more than just a nice-to-have. > @@ -812,17 +813,22 @@ ept_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn, > rc = atomic_write_ept_entry(ept_entry, new_entry, target); > if ( unlikely(rc) ) > old_entry.epte = 0; > - else if ( p2mt != p2m_invalid && > - (gfn + (1UL << order) - 1 > p2m->max_mapped_pfn) ) > - /* Track the highest gfn for which we have ever had a valid mapping */ > - p2m->max_mapped_pfn = gfn + (1UL << order) - 1; > + else > + { > + entry_written = 1; > + > + if ( p2mt != p2m_invalid && > + (gfn + (1UL << order) - 1 > p2m->max_mapped_pfn) ) > + /* Track the highest gfn for which we have ever had a valid mapping */ > + p2m->max_mapped_pfn = gfn + (1UL << order) - 1; > + } > > out: > if ( needs_sync ) > ept_sync_domain(p2m); > > /* For host p2m, may need to change VT-d page table.*/ > - if ( rc == 0 && p2m_is_hostp2m(p2m) && need_iommu(d) && > + if ( entry_written && p2m_is_hostp2m(p2m) && need_iommu(d) && > need_modify_vtd_table ) > { I'd prefer this conditional to remain untouched, but I'll leave the decision to the maintainers of the file. > @@ -831,10 +837,28 @@ out: > { > if ( iommu_flags ) > for ( i = 0; i < (1 << order); i++ ) > - iommu_map_page(d, gfn + i, mfn_x(mfn) + i, iommu_flags); > + { > + ret = iommu_map_page(d, gfn + i, mfn_x(mfn) + i, iommu_flags); > + > + if ( unlikely(ret) ) > + { > + while ( i-- ) > + iommu_unmap_page(p2m->domain, gfn + i); Loops like this are, btw., good examples of the log spam issue I've been pointing out on an earlier patch of this series. > --- a/xen/arch/x86/mm/p2m.c > +++ b/xen/arch/x86/mm/p2m.c > @@ -638,13 +638,20 @@ p2m_remove_page(struct p2m_domain *p2m, unsigned long gfn, unsigned long mfn, > mfn_t mfn_return; > p2m_type_t t; > p2m_access_t a; > + int rc = 0, ret; > > if ( !paging_mode_translate(p2m->domain) ) > { > if ( need_iommu(p2m->domain) ) > for ( i = 0; i < (1 << page_order); i++ ) > - iommu_unmap_page(p2m->domain, mfn + i); > - return 0; > + { > + ret = iommu_unmap_page(p2m->domain, mfn + i); > + > + if ( !rc ) > + rc = ret; > + } > + > + return rc; > } In code like this, btw., restricting the scope of "ret" to the innermost block would help future readers see immediately that the value of "ret" is of no further interest outside of that block. Having reached the end of the patch, I'm missing the __must_check additions that you said you would do in this new iteration. Is there any reason for their absence? Did I overlook something? Jan
On Tue, May 10, 2016 at 9:44 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote: >> When IOMMU mapping is failed, we issue a best effort rollback, stopping >> IOMMU mapping, unmapping the previous IOMMU maps and then reporting the >> error up to the call trees. When rollback is not feasible (in early >> initialization phase or trade-off of complexity) for the hardware domain, >> we do things on a best effort basis, only throwing out an error message. >> >> IOMMU unmapping should perhaps continue despite an error, in an attempt >> to do best effort cleanup. >> >> Signed-off-by: Quan Xu <quan.xu@intel.com> >> >> CC: Keir Fraser <keir@xen.org> >> CC: Jan Beulich <jbeulich@suse.com> >> CC: Andrew Cooper <andrew.cooper3@citrix.com> >> CC: Jun Nakajima <jun.nakajima@intel.com> >> CC: Kevin Tian <kevin.tian@intel.com> >> CC: George Dunlap <george.dunlap@eu.citrix.com> >> --- > > Somewhere here I continue to miss a summary on what has changed > compared to the previous version. For review especially of larger > patches (where preferably one wouldn't want to re-review the entire > thing) this is more than just a nice-to-have. > >> @@ -812,17 +813,22 @@ ept_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn, >> rc = atomic_write_ept_entry(ept_entry, new_entry, target); >> if ( unlikely(rc) ) >> old_entry.epte = 0; >> - else if ( p2mt != p2m_invalid && >> - (gfn + (1UL << order) - 1 > p2m->max_mapped_pfn) ) >> - /* Track the highest gfn for which we have ever had a valid mapping */ >> - p2m->max_mapped_pfn = gfn + (1UL << order) - 1; >> + else >> + { >> + entry_written = 1; >> + >> + if ( p2mt != p2m_invalid && >> + (gfn + (1UL << order) - 1 > p2m->max_mapped_pfn) ) >> + /* Track the highest gfn for which we have ever had a valid mapping */ >> + p2m->max_mapped_pfn = gfn + (1UL << order) - 1; >> + } >> >> out: >> if ( needs_sync ) >> ept_sync_domain(p2m); >> >> /* For host p2m, may need to change VT-d page table.*/ >> - if ( rc == 0 && p2m_is_hostp2m(p2m) && need_iommu(d) && >> + if ( entry_written && p2m_is_hostp2m(p2m) && need_iommu(d) && >> need_modify_vtd_table ) >> { > > I'd prefer this conditional to remain untouched, but I'll leave the > decision to the maintainers of the file. Any particular reason you think it would be better untouched? I asked for it to be changed to "entry_written", because it seemed to me that's what was actually wanted (i.e., you're checking whether rc == 0 to determine whether the entry was written or not). At the moment the checks will be identical, but if someone changed something later, rc might be non-zero even though the entry had been written, in which case (I think) you'd want the iommu update to happen. It's not that big a deal to me, but I do prefer it this way (unless I've misunderstood something). >> --- a/xen/arch/x86/mm/p2m.c >> +++ b/xen/arch/x86/mm/p2m.c >> @@ -638,13 +638,20 @@ p2m_remove_page(struct p2m_domain *p2m, unsigned long gfn, unsigned long mfn, >> mfn_t mfn_return; >> p2m_type_t t; >> p2m_access_t a; >> + int rc = 0, ret; >> >> if ( !paging_mode_translate(p2m->domain) ) >> { >> if ( need_iommu(p2m->domain) ) >> for ( i = 0; i < (1 << page_order); i++ ) >> - iommu_unmap_page(p2m->domain, mfn + i); >> - return 0; >> + { >> + ret = iommu_unmap_page(p2m->domain, mfn + i); >> + >> + if ( !rc ) >> + rc = ret; >> + } >> + >> + return rc; >> } > > In code like this, btw., restricting the scope of "ret" to the innermost > block would help future readers see immediately that the value of > "ret" is of no further interest outside of that block. I wouldn't ask for re-send just for this, but... > Having reached the end of the patch, I'm missing the __must_check > additions that you said you would do in this new iteration. Is there > any reason for their absence? Did I overlook something? If it's going to be re-sent anyway, moving the ret declaration inside the loop might as well be done. Other than that, it looks good to me, thanks. -George
On Tue, May 10, 2016 at 3:45 PM, George Dunlap <George.Dunlap@eu.citrix.com> wrote: > On Tue, May 10, 2016 at 9:44 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote: >>> When IOMMU mapping is failed, we issue a best effort rollback, stopping >>> IOMMU mapping, unmapping the previous IOMMU maps and then reporting the >>> error up to the call trees. When rollback is not feasible (in early >>> initialization phase or trade-off of complexity) for the hardware domain, >>> we do things on a best effort basis, only throwing out an error message. >>> >>> IOMMU unmapping should perhaps continue despite an error, in an attempt >>> to do best effort cleanup. >>> >>> Signed-off-by: Quan Xu <quan.xu@intel.com> >>> >>> CC: Keir Fraser <keir@xen.org> >>> CC: Jan Beulich <jbeulich@suse.com> >>> CC: Andrew Cooper <andrew.cooper3@citrix.com> >>> CC: Jun Nakajima <jun.nakajima@intel.com> >>> CC: Kevin Tian <kevin.tian@intel.com> >>> CC: George Dunlap <george.dunlap@eu.citrix.com> >>> --- >> >> Somewhere here I continue to miss a summary on what has changed >> compared to the previous version. For review especially of larger >> patches (where preferably one wouldn't want to re-review the entire >> thing) this is more than just a nice-to-have. >> >>> @@ -812,17 +813,22 @@ ept_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn, >>> rc = atomic_write_ept_entry(ept_entry, new_entry, target); >>> if ( unlikely(rc) ) >>> old_entry.epte = 0; >>> - else if ( p2mt != p2m_invalid && >>> - (gfn + (1UL << order) - 1 > p2m->max_mapped_pfn) ) >>> - /* Track the highest gfn for which we have ever had a valid mapping */ >>> - p2m->max_mapped_pfn = gfn + (1UL << order) - 1; >>> + else >>> + { >>> + entry_written = 1; >>> + >>> + if ( p2mt != p2m_invalid && >>> + (gfn + (1UL << order) - 1 > p2m->max_mapped_pfn) ) >>> + /* Track the highest gfn for which we have ever had a valid mapping */ >>> + p2m->max_mapped_pfn = gfn + (1UL << order) - 1; >>> + } >>> >>> out: >>> if ( needs_sync ) >>> ept_sync_domain(p2m); >>> >>> /* For host p2m, may need to change VT-d page table.*/ >>> - if ( rc == 0 && p2m_is_hostp2m(p2m) && need_iommu(d) && >>> + if ( entry_written && p2m_is_hostp2m(p2m) && need_iommu(d) && >>> need_modify_vtd_table ) >>> { >> >> I'd prefer this conditional to remain untouched, but I'll leave the >> decision to the maintainers of the file. > > Any particular reason you think it would be better untouched? > > I asked for it to be changed to "entry_written", because it seemed to > me that's what was actually wanted (i.e., you're checking whether rc > == 0 to determine whether the entry was written or not). At the > moment the checks will be identical, but if someone changed something > later, rc might be non-zero even though the entry had been written, in > which case (I think) you'd want the iommu update to happen. > > It's not that big a deal to me, but I do prefer it this way (unless > I've misunderstood something). See the discussion on patch 8 regarding why I now think Jan is probably right. -George
>>> On 10.05.16 at 16:45, <George.Dunlap@eu.citrix.com> wrote: > On Tue, May 10, 2016 at 9:44 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote: >>> @@ -812,17 +813,22 @@ ept_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn, >>> rc = atomic_write_ept_entry(ept_entry, new_entry, target); >>> if ( unlikely(rc) ) >>> old_entry.epte = 0; >>> - else if ( p2mt != p2m_invalid && >>> - (gfn + (1UL << order) - 1 > p2m->max_mapped_pfn) ) >>> - /* Track the highest gfn for which we have ever had a valid mapping */ >>> - p2m->max_mapped_pfn = gfn + (1UL << order) - 1; >>> + else >>> + { >>> + entry_written = 1; >>> + >>> + if ( p2mt != p2m_invalid && >>> + (gfn + (1UL << order) - 1 > p2m->max_mapped_pfn) ) >>> + /* Track the highest gfn for which we have ever had a valid mapping */ >>> + p2m->max_mapped_pfn = gfn + (1UL << order) - 1; >>> + } >>> >>> out: >>> if ( needs_sync ) >>> ept_sync_domain(p2m); >>> >>> /* For host p2m, may need to change VT-d page table.*/ >>> - if ( rc == 0 && p2m_is_hostp2m(p2m) && need_iommu(d) && >>> + if ( entry_written && p2m_is_hostp2m(p2m) && need_iommu(d) && >>> need_modify_vtd_table ) >>> { >> >> I'd prefer this conditional to remain untouched, but I'll leave the >> decision to the maintainers of the file. > > Any particular reason you think it would be better untouched? IMO it's misleading here, and appropriate only in the other place (where the altp2m-s get updated). > I asked for it to be changed to "entry_written", because it seemed to > me that's what was actually wanted (i.e., you're checking whether rc > == 0 to determine whether the entry was written or not). At the > moment the checks will be identical, but if someone changed something > later, rc might be non-zero even though the entry had been written, in > which case (I think) you'd want the iommu update to happen. > > It's not that big a deal to me, but I do prefer it this way (unless > I've misunderstood something). > >>> --- a/xen/arch/x86/mm/p2m.c >>> +++ b/xen/arch/x86/mm/p2m.c >>> @@ -638,13 +638,20 @@ p2m_remove_page(struct p2m_domain *p2m, unsigned long gfn, unsigned long mfn, >>> mfn_t mfn_return; >>> p2m_type_t t; >>> p2m_access_t a; >>> + int rc = 0, ret; >>> >>> if ( !paging_mode_translate(p2m->domain) ) >>> { >>> if ( need_iommu(p2m->domain) ) >>> for ( i = 0; i < (1 << page_order); i++ ) >>> - iommu_unmap_page(p2m->domain, mfn + i); >>> - return 0; >>> + { >>> + ret = iommu_unmap_page(p2m->domain, mfn + i); >>> + >>> + if ( !rc ) >>> + rc = ret; >>> + } >>> + >>> + return rc; >>> } >> >> In code like this, btw., restricting the scope of "ret" to the innermost >> block would help future readers see immediately that the value of >> "ret" is of no further interest outside of that block. > > I wouldn't ask for re-send just for this, but... I agree, and I meant to express this with the "btw". Jan >> Having reached the end of the patch, I'm missing the __must_check >> additions that you said you would do in this new iteration. Is there >> any reason for their absence? Did I overlook something? > > If it's going to be re-sent anyway, moving the ret declaration inside > the loop might as well be done. > > Other than that, it looks good to me, thanks. > > -George
On May 10, 2016 11:00 PM, George Dunlap <George.Dunlap@eu.citrix.com> wrote: > On Tue, May 10, 2016 at 3:45 PM, George Dunlap > <George.Dunlap@eu.citrix.com> wrote: > > On Tue, May 10, 2016 at 9:44 AM, Jan Beulich <JBeulich@suse.com> wrote: > >>>>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote: > >>> When IOMMU mapping is failed, we issue a best effort rollback, > >>> stopping IOMMU mapping, unmapping the previous IOMMU maps and > then > >>> reporting the error up to the call trees. When rollback is not > >>> feasible (in early initialization phase or trade-off of complexity) > >>> for the hardware domain, we do things on a best effort basis, only > throwing out an error message. > >>> > >>> IOMMU unmapping should perhaps continue despite an error, in an > >>> attempt to do best effort cleanup. > >>> > >>> Signed-off-by: Quan Xu <quan.xu@intel.com> > >>> > >>> CC: Keir Fraser <keir@xen.org> > >>> CC: Jan Beulich <jbeulich@suse.com> > >>> CC: Andrew Cooper <andrew.cooper3@citrix.com> > >>> CC: Jun Nakajima <jun.nakajima@intel.com> > >>> CC: Kevin Tian <kevin.tian@intel.com> > >>> CC: George Dunlap <george.dunlap@eu.citrix.com> > >>> --- > >> > >> Somewhere here I continue to miss a summary on what has changed > >> compared to the previous version. For review especially of larger > >> patches (where preferably one wouldn't want to re-review the entire > >> thing) this is more than just a nice-to-have. > >> > >>> @@ -812,17 +813,22 @@ ept_set_entry(struct p2m_domain *p2m, > unsigned long gfn, mfn_t mfn, > >>> rc = atomic_write_ept_entry(ept_entry, new_entry, target); > >>> if ( unlikely(rc) ) > >>> old_entry.epte = 0; > >>> - else if ( p2mt != p2m_invalid && > >>> - (gfn + (1UL << order) - 1 > p2m->max_mapped_pfn) ) > >>> - /* Track the highest gfn for which we have ever had a valid mapping > */ > >>> - p2m->max_mapped_pfn = gfn + (1UL << order) - 1; > >>> + else > >>> + { > >>> + entry_written = 1; > >>> + > >>> + if ( p2mt != p2m_invalid && > >>> + (gfn + (1UL << order) - 1 > p2m->max_mapped_pfn) ) > >>> + /* Track the highest gfn for which we have ever had a valid > mapping */ > >>> + p2m->max_mapped_pfn = gfn + (1UL << order) - 1; > >>> + } > >>> > >>> out: > >>> if ( needs_sync ) > >>> ept_sync_domain(p2m); > >>> > >>> /* For host p2m, may need to change VT-d page table.*/ > >>> - if ( rc == 0 && p2m_is_hostp2m(p2m) && need_iommu(d) && > >>> + if ( entry_written && p2m_is_hostp2m(p2m) && need_iommu(d) && > >>> need_modify_vtd_table ) > >>> { > >> > >> I'd prefer this conditional to remain untouched, but I'll leave the > >> decision to the maintainers of the file. > > > > Any particular reason you think it would be better untouched? > > > > I asked for it to be changed to "entry_written", because it seemed to > > me that's what was actually wanted (i.e., you're checking whether rc > > == 0 to determine whether the entry was written or not). At the > > moment the checks will be identical, but if someone changed something > > later, rc might be non-zero even though the entry had been written, in > > which case (I think) you'd want the iommu update to happen. > > > > It's not that big a deal to me, but I do prefer it this way (unless > > I've misunderstood something). > > See the discussion on patch 8 regarding why I now think Jan is probably right. > Agreed. Thanks for your careful checking. Check it again -- 1. then I am no need to check 'rc' as below: if ( rc == 0 && p2m_is_hostp2m(p2m) && need_iommu(d) && need_modify_vtd_table ) { + if ( !rc ) + rc = ret; ... + if ( !rc && unlikely(ret) ) + rc = ret; } 2. leave the below as is: - if ( rc == 0 && p2m_is_hostp2m(p2m) ) + if ( entry_written && p2m_is_hostp2m(p2m) ) Quan
On May 10, 2016 10:45 PM, George Dunlap <George.Dunlap@eu.citrix.com> wrote: > On Tue, May 10, 2016 at 9:44 AM, Jan Beulich <JBeulich@suse.com> wrote: > >>>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote: > >> When IOMMU mapping is failed, we issue a best effort rollback, > >> stopping IOMMU mapping, unmapping the previous IOMMU maps and > then > >> reporting the error up to the call trees. When rollback is not > >> feasible (in early initialization phase or trade-off of complexity) > >> for the hardware domain, we do things on a best effort basis, only > throwing out an error message. > >> > >> IOMMU unmapping should perhaps continue despite an error, in an > >> attempt to do best effort cleanup. > >> > >> --- a/xen/arch/x86/mm/p2m.c > >> +++ b/xen/arch/x86/mm/p2m.c > >> @@ -638,13 +638,20 @@ p2m_remove_page(struct p2m_domain *p2m, > unsigned long gfn, unsigned long mfn, > >> mfn_t mfn_return; > >> p2m_type_t t; > >> p2m_access_t a; > >> + int rc = 0, ret; > >> > >> if ( !paging_mode_translate(p2m->domain) ) > >> { > >> if ( need_iommu(p2m->domain) ) > >> for ( i = 0; i < (1 << page_order); i++ ) > >> - iommu_unmap_page(p2m->domain, mfn + i); > >> - return 0; > >> + { > >> + ret = iommu_unmap_page(p2m->domain, mfn + i); > >> + > >> + if ( !rc ) > >> + rc = ret; > >> + } > >> + > >> + return rc; > >> } > > > > In code like this, btw., restricting the scope of "ret" to the > > innermost block would help future readers see immediately that the > > value of "ret" is of no further interest outside of that block. > > I wouldn't ask for re-send just for this, but... > > > Having reached the end of the patch, I'm missing the __must_check > > additions that you said you would do in this new iteration. Is there > > any reason for their absence? Did I overlook something? > > If it's going to be re-sent anyway, moving the ret declaration inside the loop > might as well be done. > > Other than that, it looks good to me, thanks. > I'll fix and re-send it in next v5. Quan
On May 10, 2016 4:44 PM, Jan Beulich <JBeulich@suse.com> wrote: > >>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote: > > --- a/xen/arch/x86/mm/p2m.c > > +++ b/xen/arch/x86/mm/p2m.c > > @@ -638,13 +638,20 @@ p2m_remove_page(struct p2m_domain *p2m, > unsigned long gfn, unsigned long mfn, > > mfn_t mfn_return; > > p2m_type_t t; > > p2m_access_t a; > > + int rc = 0, ret; > > > > if ( !paging_mode_translate(p2m->domain) ) > > { > > if ( need_iommu(p2m->domain) ) > > for ( i = 0; i < (1 << page_order); i++ ) > > - iommu_unmap_page(p2m->domain, mfn + i); > > - return 0; > > + { > > + ret = iommu_unmap_page(p2m->domain, mfn + i); > > + > > + if ( !rc ) > > + rc = ret; > > + } > > + > > + return rc; > > } > > In code like this, btw., restricting the scope of "ret" to the innermost block > would help future readers see immediately that the value of "ret" is of no > further interest outside of that block. > > Having reached the end of the patch, I'm missing the __must_check additions > that you said you would do in this new iteration. Is there any reason for their > absence? Did I overlook something? > Sorry, I did overlook something. Checked the v2/v3 replies again, I still can't find it. I only add the __must_check annotation for these functions you point out. Do I need to add the __must_check annotation for these functions (but not void function) in this patch? Quan
>>> On 11.05.16 at 05:39, <quan.xu@intel.com> wrote: > On May 10, 2016 4:44 PM, Jan Beulich <JBeulich@suse.com> wrote: >> >>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote: >> > --- a/xen/arch/x86/mm/p2m.c >> > +++ b/xen/arch/x86/mm/p2m.c >> > @@ -638,13 +638,20 @@ p2m_remove_page(struct p2m_domain *p2m, >> unsigned long gfn, unsigned long mfn, >> > mfn_t mfn_return; >> > p2m_type_t t; >> > p2m_access_t a; >> > + int rc = 0, ret; >> > >> > if ( !paging_mode_translate(p2m->domain) ) >> > { >> > if ( need_iommu(p2m->domain) ) >> > for ( i = 0; i < (1 << page_order); i++ ) >> > - iommu_unmap_page(p2m->domain, mfn + i); >> > - return 0; >> > + { >> > + ret = iommu_unmap_page(p2m->domain, mfn + i); >> > + >> > + if ( !rc ) >> > + rc = ret; >> > + } >> > + >> > + return rc; >> > } >> >> In code like this, btw., restricting the scope of "ret" to the innermost > block >> would help future readers see immediately that the value of "ret" is of no >> further interest outside of that block. >> >> Having reached the end of the patch, I'm missing the __must_check additions >> that you said you would do in this new iteration. Is there any reason for > their >> absence? Did I overlook something? >> > > Sorry, I did overlook something. > Checked the v2/v3 replies again, I still can't find it. > I only add the __must_check annotation for these functions you point out. Okay, that's the problem then: When we discussed this originally (in abstract terms) I had clearly said that all involved functions should become __must_check ones to make sure all cases get caught where so far error returns got ignored. And on that basis (as well as on the common grounds that I try to avoid repeating the same comment over and over when reviewing a patch or a series of patches) you should have determined yourself the full set of functions needing the annotation. The rule of thumb is: If a function calls a __must_check one and doesn't itself consume the return value, it should also obtain __must_check. > Do I need to add the __must_check annotation for these functions (but not > void function) in this patch? IOW - yes. And you'll need to apply the same consideration to most (all?) other patches in this series. Jan
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index 2bb920b..14b54a9 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -2467,7 +2467,7 @@ static int __get_page_type(struct page_info *page, unsigned long type, int preemptible) { unsigned long nx, x, y = page->u.inuse.type_info; - int rc = 0; + int rc = 0, ret = 0; ASSERT(!(type & ~(PGT_type_mask | PGT_pae_xen_l2))); @@ -2578,11 +2578,11 @@ static int __get_page_type(struct page_info *page, unsigned long type, if ( d && is_pv_domain(d) && unlikely(need_iommu(d)) ) { if ( (x & PGT_type_mask) == PGT_writable_page ) - iommu_unmap_page(d, mfn_to_gmfn(d, page_to_mfn(page))); + ret = iommu_unmap_page(d, mfn_to_gmfn(d, page_to_mfn(page))); else if ( type == PGT_writable_page ) - iommu_map_page(d, mfn_to_gmfn(d, page_to_mfn(page)), - page_to_mfn(page), - IOMMUF_readable|IOMMUF_writable); + ret = iommu_map_page(d, mfn_to_gmfn(d, page_to_mfn(page)), + page_to_mfn(page), + IOMMUF_readable|IOMMUF_writable); } } @@ -2599,6 +2599,9 @@ static int __get_page_type(struct page_info *page, unsigned long type, if ( (x & PGT_partial) && !(nx & PGT_partial) ) put_page(page); + if ( !rc ) + rc = ret; + return rc; } diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c index 1ed5b47..814cb72 100644 --- a/xen/arch/x86/mm/p2m-ept.c +++ b/xen/arch/x86/mm/p2m-ept.c @@ -667,6 +667,7 @@ ept_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn, unsigned long gfn_remainder = gfn; unsigned int i, target = order / EPT_TABLE_ORDER; int ret, rc = 0; + bool_t entry_written = 0; bool_t direct_mmio = (p2mt == p2m_mmio_direct); uint8_t ipat = 0; bool_t need_modify_vtd_table = 1; @@ -812,17 +813,22 @@ ept_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn, rc = atomic_write_ept_entry(ept_entry, new_entry, target); if ( unlikely(rc) ) old_entry.epte = 0; - else if ( p2mt != p2m_invalid && - (gfn + (1UL << order) - 1 > p2m->max_mapped_pfn) ) - /* Track the highest gfn for which we have ever had a valid mapping */ - p2m->max_mapped_pfn = gfn + (1UL << order) - 1; + else + { + entry_written = 1; + + if ( p2mt != p2m_invalid && + (gfn + (1UL << order) - 1 > p2m->max_mapped_pfn) ) + /* Track the highest gfn for which we have ever had a valid mapping */ + p2m->max_mapped_pfn = gfn + (1UL << order) - 1; + } out: if ( needs_sync ) ept_sync_domain(p2m); /* For host p2m, may need to change VT-d page table.*/ - if ( rc == 0 && p2m_is_hostp2m(p2m) && need_iommu(d) && + if ( entry_written && p2m_is_hostp2m(p2m) && need_iommu(d) && need_modify_vtd_table ) { if ( iommu_hap_pt_share ) @@ -831,10 +837,28 @@ out: { if ( iommu_flags ) for ( i = 0; i < (1 << order); i++ ) - iommu_map_page(d, gfn + i, mfn_x(mfn) + i, iommu_flags); + { + ret = iommu_map_page(d, gfn + i, mfn_x(mfn) + i, iommu_flags); + + if ( unlikely(ret) ) + { + while ( i-- ) + iommu_unmap_page(p2m->domain, gfn + i); + + if ( !rc ) + rc = ret; + + break; + } + } else for ( i = 0; i < (1 << order); i++ ) - iommu_unmap_page(d, gfn + i); + { + ret = iommu_unmap_page(d, gfn + i); + + if ( !rc ) + rc = ret; + } } } @@ -847,7 +871,7 @@ out: if ( is_epte_present(&old_entry) ) ept_free_entry(p2m, &old_entry, target); - if ( rc == 0 && p2m_is_hostp2m(p2m) ) + if ( entry_written && p2m_is_hostp2m(p2m) ) p2m_altp2m_propagate_change(d, _gfn(gfn), mfn, order, p2mt, p2ma); return rc; diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c index 3d80612..5426f92 100644 --- a/xen/arch/x86/mm/p2m-pt.c +++ b/xen/arch/x86/mm/p2m-pt.c @@ -498,7 +498,7 @@ p2m_pt_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn, l1_pgentry_t intermediate_entry = l1e_empty(); l2_pgentry_t l2e_content; l3_pgentry_t l3e_content; - int rc; + int rc, ret; unsigned int iommu_pte_flags = p2m_get_iommu_flags(p2mt); /* * old_mfn and iommu_old_flags control possible flush/update needs on the @@ -680,11 +680,29 @@ p2m_pt_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn, } else if ( iommu_pte_flags ) for ( i = 0; i < (1UL << page_order); i++ ) - iommu_map_page(p2m->domain, gfn + i, mfn_x(mfn) + i, - iommu_pte_flags); + { + ret = iommu_map_page(p2m->domain, gfn + i, mfn_x(mfn) + i, + iommu_pte_flags); + + if ( unlikely(ret) ) + { + while ( i-- ) + iommu_unmap_page(p2m->domain, gfn + i); + + if ( !rc ) + rc = ret; + + break; + } + } else for ( i = 0; i < (1UL << page_order); i++ ) - iommu_unmap_page(p2m->domain, gfn + i); + { + ret = iommu_unmap_page(p2m->domain, gfn + i); + + if ( !rc ) + rc = ret; + } } /* diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c index 94eabf6..cb77ef2 100644 --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -638,13 +638,20 @@ p2m_remove_page(struct p2m_domain *p2m, unsigned long gfn, unsigned long mfn, mfn_t mfn_return; p2m_type_t t; p2m_access_t a; + int rc = 0, ret; if ( !paging_mode_translate(p2m->domain) ) { if ( need_iommu(p2m->domain) ) for ( i = 0; i < (1 << page_order); i++ ) - iommu_unmap_page(p2m->domain, mfn + i); - return 0; + { + ret = iommu_unmap_page(p2m->domain, mfn + i); + + if ( !rc ) + rc = ret; + } + + return rc; } ASSERT(gfn_locked_by_me(p2m, gfn)); diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c index 09560c0..cca4cf3 100644 --- a/xen/drivers/passthrough/iommu.c +++ b/xen/drivers/passthrough/iommu.c @@ -171,6 +171,8 @@ void __hwdom_init iommu_hwdom_init(struct domain *d) { struct page_info *page; unsigned int i = 0; + int ret, rc = 0; + page_list_for_each ( page, &d->page_list ) { unsigned long mfn = page_to_mfn(page); @@ -181,10 +183,20 @@ 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); + + ret = hd->platform_ops->map_page(d, gfn, mfn, mapping); + + if ( unlikely(ret) ) + rc = ret; + if ( !(i++ & 0xfffff) ) process_pending_softirqs(); } + + if ( rc ) + printk(XENLOG_WARNING + "iommu_hwdom_init: IOMMU mapping failed for dom%d.", + d->domain_id); } return hd->platform_ops->hwdom_init(d);
When IOMMU mapping is failed, we issue a best effort rollback, stopping IOMMU mapping, unmapping the previous IOMMU maps and then reporting the error up to the call trees. When rollback is not feasible (in early initialization phase or trade-off of complexity) for the hardware domain, we do things on a best effort basis, only throwing out an error message. IOMMU unmapping should perhaps continue despite an error, in an attempt to do best effort cleanup. Signed-off-by: Quan Xu <quan.xu@intel.com> CC: Keir Fraser <keir@xen.org> CC: Jan Beulich <jbeulich@suse.com> CC: Andrew Cooper <andrew.cooper3@citrix.com> CC: Jun Nakajima <jun.nakajima@intel.com> CC: Kevin Tian <kevin.tian@intel.com> CC: George Dunlap <george.dunlap@eu.citrix.com> --- xen/arch/x86/mm.c | 13 ++++++++----- xen/arch/x86/mm/p2m-ept.c | 40 ++++++++++++++++++++++++++++++++-------- xen/arch/x86/mm/p2m-pt.c | 26 ++++++++++++++++++++++---- xen/arch/x86/mm/p2m.c | 11 +++++++++-- xen/drivers/passthrough/iommu.c | 14 +++++++++++++- 5 files changed, 84 insertions(+), 20 deletions(-)