Re: [Xen-devel] [PATCH v8 02/11] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping
On June 15, 2016 4:42 PM, Jan Beulich wrote: > >>> On 15.06.16 at 10:35, wrote: > > On June 15, 2016 4:22 PM, Tian, Kevin wrote: > >> > From: Xu, Quan > >> > Sent: Wednesday, June 15, 2016 4:16 PM > >> > > >> > On June 15, 2016 3:45 PM, Jan Beulich wrote: > >> > > >>> On 15.06.16 at 03:54, wrote: > >> > > > Jan, > >> > > > Could you help me review patch 7? Thanks. > >> > > > Then, I can send out next v9 soon and get started to focus on > >> > > > next patch set. > >> > > > >> > > That patch is fine now from my pov, feel free to stick my R-b on it. > >> > > I actually have it queued for committing already, pending an ARM > >> > > ack for patch 4 (which is why yesterday I committed only the > >> > > first three patches of that series); I didn't check yet which > >> > > other acks may still be missing on later patches, everything up > >> > > to patch 8 is ready to go in afaic, pending all necessary acks are in > >> > > place. > >> > > > >> > > >> > Jan, > >> > thanks very much!! > >> > For patch 4, I will ping arm maintainer in that email. > >> > > >> > > >> > > >> > Kevin, > >> > For patch 9, could you help me review it? With your reply of > >> > http://lists.xenproject.org/archives/html/xen-devel/2016- > >> 06/msg01842.html , > >> >there is no issue doing so based on spec. I think what I do is > >> > changing ' iommu_rc / iommu_ret ' to ' context_rc / iotlb_rc '. > >> >in this case, I wonder whether this can be fixed upon commit, > >> > then I am no need to send out v9. > >> > > >> > >> Yes, no more comments except the naming: > >> > >> Acked-by: Kevin Tian > > > > Jan, > > If there are no other problem, could you help me fix these naming > > issue upon commit? > > I wouldn't want to do that. As said in an earlier reply, I have things up to > patch > 8 queued for commit (which by implication means I'd expect at least 9...11 to > see another revision). > Got it. I will send out v9 when patch 4 is acked. > > Also for the comments, you can change them as you like. > > I'm not clear which comments you refer to here. > For all of comments, especially the repetitive ones of patch 9. Quan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8 02/11] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping
>>> On 15.06.16 at 10:35, wrote: > On June 15, 2016 4:22 PM, Tian, Kevin wrote: >> > From: Xu, Quan >> > Sent: Wednesday, June 15, 2016 4:16 PM >> > >> > On June 15, 2016 3:45 PM, Jan Beulich wrote: >> > > >>> On 15.06.16 at 03:54, wrote: >> > > > Jan, >> > > > Could you help me review patch 7? Thanks. >> > > > Then, I can send out next v9 soon and get started to focus on next >> > > > patch set. >> > > >> > > That patch is fine now from my pov, feel free to stick my R-b on it. >> > > I actually have it queued for committing already, pending an ARM ack >> > > for patch 4 (which is why yesterday I committed only the first three >> > > patches of that series); I didn't check yet which other acks may >> > > still be missing on later patches, everything up to patch 8 is ready >> > > to go in afaic, pending all necessary acks are in place. >> > > >> > >> > Jan, >> > thanks very much!! >> > For patch 4, I will ping arm maintainer in that email. >> > >> > >> > >> > Kevin, >> > For patch 9, could you help me review it? With your reply of >> > http://lists.xenproject.org/archives/html/xen-devel/2016- >> 06/msg01842.html , >> >there is no issue doing so based on spec. I think what I do is >> > changing ' iommu_rc / iommu_ret ' to ' context_rc / iotlb_rc '. >> >in this case, I wonder whether this can be fixed upon commit, then >> > I am no need to send out v9. >> > >> >> Yes, no more comments except the naming: >> >> Acked-by: Kevin Tian > > Jan, > If there are no other problem, could you help me fix these naming issue upon > commit? I wouldn't want to do that. As said in an earlier reply, I have things up to patch 8 queued for commit (which by implication means I'd expect at least 9...11 to see another revision). > Also for the comments, you can change them as you like. I'm not clear which comments you refer to here. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8 02/11] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping
On June 15, 2016 4:22 PM, Tian, Kevin wrote: > > From: Xu, Quan > > Sent: Wednesday, June 15, 2016 4:16 PM > > > > On June 15, 2016 3:45 PM, Jan Beulich wrote: > > > >>> On 15.06.16 at 03:54, wrote: > > > > Jan, > > > > Could you help me review patch 7? Thanks. > > > > Then, I can send out next v9 soon and get started to focus on next > > > > patch set. > > > > > > That patch is fine now from my pov, feel free to stick my R-b on it. > > > I actually have it queued for committing already, pending an ARM ack > > > for patch 4 (which is why yesterday I committed only the first three > > > patches of that series); I didn't check yet which other acks may > > > still be missing on later patches, everything up to patch 8 is ready > > > to go in afaic, pending all necessary acks are in place. > > > > > > > Jan, > > thanks very much!! > > For patch 4, I will ping arm maintainer in that email. > > > > > > > > Kevin, > > For patch 9, could you help me review it? With your reply of > > http://lists.xenproject.org/archives/html/xen-devel/2016- > 06/msg01842.html , > >there is no issue doing so based on spec. I think what I do is > > changing ' iommu_rc / iommu_ret ' to ' context_rc / iotlb_rc '. > >in this case, I wonder whether this can be fixed upon commit, then > > I am no need to send out v9. > > > > Yes, no more comments except the naming: > > Acked-by: Kevin Tian > Jan, If there are no other problem, could you help me fix these naming issue upon commit? Also for the comments, you can change them as you like. Quan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8 02/11] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping
> From: Xu, Quan > Sent: Wednesday, June 15, 2016 4:16 PM > > On June 15, 2016 3:45 PM, Jan Beulich wrote: > > >>> On 15.06.16 at 03:54, wrote: > > > Jan, > > > Could you help me review patch 7? Thanks. > > > Then, I can send out next v9 soon and get started to focus on next > > > patch set. > > > > That patch is fine now from my pov, feel free to stick my R-b on it. > > I actually have it queued for committing already, pending an ARM ack for > > patch 4 (which is why yesterday I committed only the first three patches of > > that series); I didn't check yet which other acks may still be missing on > > later > > patches, everything up to patch 8 is ready to go in afaic, pending all > > necessary > > acks are in place. > > > > Jan, > thanks very much!! > For patch 4, I will ping arm maintainer in that email. > > > > Kevin, > For patch 9, could you help me review it? With your reply of > http://lists.xenproject.org/archives/html/xen-devel/2016-06/msg01842.html , >there is no issue doing so based on spec. I think what I do is changing ' > iommu_rc / > iommu_ret ' to ' context_rc / iotlb_rc '. >in this case, I wonder whether this can be fixed upon commit, then I am no > need to send > out v9. > Yes, no more comments except the naming: Acked-by: Kevin Tian Thanks Kevin ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8 02/11] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping
On June 15, 2016 3:45 PM, Jan Beulich wrote: > >>> On 15.06.16 at 03:54, wrote: > > Jan, > > Could you help me review patch 7? Thanks. > > Then, I can send out next v9 soon and get started to focus on next > > patch set. > > That patch is fine now from my pov, feel free to stick my R-b on it. > I actually have it queued for committing already, pending an ARM ack for > patch 4 (which is why yesterday I committed only the first three patches of > that series); I didn't check yet which other acks may still be missing on > later > patches, everything up to patch 8 is ready to go in afaic, pending all > necessary > acks are in place. > Jan, thanks very much!! For patch 4, I will ping arm maintainer in that email. Kevin, For patch 9, could you help me review it? With your reply of http://lists.xenproject.org/archives/html/xen-devel/2016-06/msg01842.html , there is no issue doing so based on spec. I think what I do is changing ' iommu_rc / iommu_ret ' to ' context_rc / iotlb_rc '. in this case, I wonder whether this can be fixed upon commit, then I am no need to send out v9. Quan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8 02/11] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping
>>> On 15.06.16 at 03:54, wrote: > Jan, > Could you help me review patch 7? Thanks. > Then, I can send out next v9 soon and get started to focus on next patch > set. That patch is fine now from my pov, feel free to stick my R-b on it. I actually have it queued for committing already, pending an ARM ack for patch 4 (which is why yesterday I committed only the first three patches of that series); I didn't check yet which other acks may still be missing on later patches, everything up to patch 8 is ready to go in afaic, pending all necessary acks are in place. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8 02/11] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping
On June 14, 2016 12:37 AM, George Dunlap wrote: > On Mon, Jun 13, 2016 at 4:17 PM, Xu, Quan wrote: > > From: Quan Xu > > > > 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 > > Reviewed-by: Jan Beulich > > Reviewed-by: Suravee Suthikulpanit > > Acked-by: Kevin Tian > > Acked-by: George Dunlap > > Phew! > > One comment... > > > +while ( i-- ) > > +/* > > + * IOMMU unmapping should perhaps continue > > despite an > > + * error in an attempt to do best effort > > cleanup, and > > + * consume the error as __must_check > > annotation. > > + */ > > +if ( iommu_unmap_page(p2m->domain, gfn + i) ) > > +continue; > > I'd take out the "perhaps", (since there's no 'perhaps' about it) but other > than > that I think this comment is fine. > > It sounds like Jan had something more along the following in mind: > > /* If statement to satisfy __must_check */ > > Either one works. The shorter one is sufficient, but the longer one isn't too > much I don't think. > George, Thanks for your comment.. I think your shorter one is better. Jan, Could you help me review patch 7? Thanks. Then, I can send out next v9 soon and get started to focus on next patch set. Quan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8 02/11] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping
On Mon, Jun 13, 2016 at 4:17 PM, Xu, Quan wrote: > From: Quan Xu > > 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 > Reviewed-by: Jan Beulich > Reviewed-by: Suravee Suthikulpanit > Acked-by: Kevin Tian Acked-by: George Dunlap Phew! One comment... > +while ( i-- ) > +/* > + * IOMMU unmapping should perhaps continue > despite an > + * error in an attempt to do best effort > cleanup, and > + * consume the error as __must_check annotation. > + */ > +if ( iommu_unmap_page(p2m->domain, gfn + i) ) > +continue; I'd take out the "perhaps", (since there's no 'perhaps' about it) but other than that I think this comment is fine. It sounds like Jan had something more along the following in mind: /* If statement to satisfy __must_check */ Either one works. The shorter one is sufficient, but the longer one isn't too much I don't think. -George ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8 02/11] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping
>>> "Xu, Quan" 06/13/16 5:22 PM >>> >From: Quan Xu > >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 >Reviewed-by: Jan Beulich >Reviewed-by: Suravee Suthikulpanit >Acked-by: Kevin Tian > >CC: Jan Beulich >CC: Andrew Cooper >CC: Jun Nakajima >CC: Kevin Tian >CC: George Dunlap >CC: Suravee Suthikulpanit >CC: Feng Wu > >v8: >1. add missing blank >2. add a brief comment (Jan, if you have a better one, could you help me >enhance it upon commit?) This _still_ sits above the first --- separator, despite you having been asked more than once to move it down. As to question 2, I'll see what I can do (I'm specifically not very happy that this comment which you added in more than one place isn't really "brief"). And before the patch can be committed, it'll need George's ack anyway. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [PATCH v8 02/11] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping
From: Quan Xu 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 Reviewed-by: Jan Beulich Reviewed-by: Suravee Suthikulpanit Acked-by: Kevin Tian CC: Jan Beulich CC: Andrew Cooper CC: Jun Nakajima CC: Kevin Tian CC: George Dunlap CC: Suravee Suthikulpanit CC: Feng Wu v8: 1. add missing blank 2. add a brief comment (Jan, if you have a better one, could you help me enhance it upon commit?) --- xen/arch/x86/mm.c | 13 ++ xen/arch/x86/mm/p2m-ept.c | 39 +++-- xen/arch/x86/mm/p2m-pt.c| 28 ++--- xen/arch/x86/mm/p2m.c | 23 ++--- xen/arch/x86/x86_64/mm.c| 9 ++- xen/drivers/passthrough/amd/pci_amd_iommu.c | 15 +-- xen/drivers/passthrough/iommu.c | 13 +- xen/drivers/passthrough/vtd/x86/vtd.c | 15 +-- xen/include/xen/iommu.h | 6 ++--- 9 files changed, 134 insertions(+), 27 deletions(-) diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index 8d10a3e..ae7c8ab 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, iommu_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))); +iommu_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); +iommu_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 = iommu_ret; + return rc; } diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c index 1ed5b47..a233194 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,10 +813,15 @@ 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 ) @@ -831,10 +837,29 @@ out: { if ( iommu_flags ) for ( i = 0; i < (1 << order); i++ ) -iommu_map_page(d, gfn + i, mfn_x(mfn) + i, iommu_flags); +{ +rc = iommu_map_page(d, gfn + i, mfn_x(mfn) + i, iommu_flags); +if ( unlikely(rc) ) +{ +while ( i-- ) +/* + * IOMMU unmapping should perhaps continue despite an + * error in an attempt to do best effort cleanup, and + * consume the error as __must_ch