Re: [Xen-devel] [PATCH v8 02/11] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping

[Xu, Quan]

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

[Jan Beulich]

>>> 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

[Xu, Quan]

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

[Tian, Kevin]

> 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

[Xu, Quan]

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

[Jan Beulich]

>>> 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

[Xu, Quan]

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

[George Dunlap]

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

[Jan Beulich]

>>> "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

[Xu, Quan]

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) )
+{
+