Re: [Xen-devel] [PATCH V3] x86/ioreq_server: Make p2m_finish_type_change actually work

2017-05-10 Thread Jan Beulich 
>>> On 11.05.17 at 00:57,  wrote:

As a general note: Please send patches _to_ the list, _cc_-ing
maintainers and other relevant people.

> v1: Add ioreq_pre_recalc query flag to get the old p2m_type.(Jan)
> v2: Add p2m->recalc() hook to change gfn p2m_type. (George)
> v3: Make commit message clearer. (George)

This doesn't belong into the commit message, so ought to go ...

> Signed-off-by: Xiong Zhang 
> Signed-off-by: Yu Zhang 
> ---

... below the first separation marker. (As a side note, v3 had more
changes than just an adjustment to the commit message.)

> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1013,26 +1013,18 @@ void p2m_change_type_range(struct domain *d,
>  
>  /* Synchronously modify the p2m type for a range of gfns from ot to nt. */
>  void p2m_finish_type_change(struct domain *d,
> -gfn_t first_gfn, unsigned long max_nr,
> -p2m_type_t ot, p2m_type_t nt)
> +gfn_t first_gfn, unsigned long max_nr)
>  {
>  struct p2m_domain *p2m = p2m_get_hostp2m(d);
> -p2m_type_t t;
>  unsigned long gfn = gfn_x(first_gfn);
>  unsigned long last_gfn = gfn + max_nr - 1;
>  
> -ASSERT(ot != nt);
> -ASSERT(p2m_is_changeable(ot) && p2m_is_changeable(nt));
> -
>  p2m_lock(p2m);
>  
>  last_gfn = min(last_gfn, p2m->max_mapped_pfn);
>  while ( gfn <= last_gfn )
>  {
> -get_gfn_query_unlocked(d, gfn, );
> -
> -if ( t == ot )
> -p2m_change_type_one(d, gfn, t, nt);
> +p2m->recalc(p2m, gfn);

You shouldn't ignore the return value here. It being positive may
be of no interest for the moment, but it being negative certainly
is.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH V3] x86/ioreq_server: Make p2m_finish_type_change actually work

2017-05-10 Thread George Dunlap 
On 10/05/17 23:57, Xiong Zhang wrote:
> Commit 6d774a951696 ("x86/ioreq server: synchronously reset outstanding
> p2m_ioreq_server entries when an ioreq server unmaps") introduced
> p2m_finish_type_change(), which was meant to synchronously finish a
> previously initiated type change over a gpfn range.  It did this by
> calling get_entry(), checking if it was the appropriate type, and then
> calling set_entry().
> 
> Unfortunately, a previous commit (1679e0df3df6 "x86/ioreq server:
> asynchronously reset outstanding p2m_ioreq_server entries") modified
> get_entry() to always return the new type after the type change, meaning
> that p2m_finish_type_change() never changed any entries.  Which means
> when an ioreq server was detached and then re-attached (as happens in
> XenGT on reboot) the re-attach failed.
> 
> Fix this by using the existing p2m-specific recalculation logic instead
> of doing a read-check-write loop.
> 
> Fix: 'commit 6d774a951696 ("x86/ioreq server: synchronously reset
>   outstanding p2m_ioreq_server entries when an ioreq server unmaps")'
> 
> v1: Add ioreq_pre_recalc query flag to get the old p2m_type.(Jan)
> v2: Add p2m->recalc() hook to change gfn p2m_type. (George)
> v3: Make commit message clearer. (George)

These changes should be put like this

> 
> Signed-off-by: Xiong Zhang 
> Signed-off-by: Yu Zhang 

---
v1: Add ioreq_pre_recalc query flag to get the old p2m_type.(Jan)
v2: Add p2m->recalc() hook to change gfn p2m_type. (George)
v3: Make commit message clearer. (George)

That is, below the S-o-B, you should add a line with '---', and then
after that add any comments that you want to aim at the reviewers (such
as changes between the versions) but not to end up checked into the tree.

This can be fixed up on check-in.

Other than that:

Reviewed-by: George Dunlap 


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [PATCH V3] x86/ioreq_server: Make p2m_finish_type_change actually work

2017-05-10 Thread Xiong Zhang 
Commit 6d774a951696 ("x86/ioreq server: synchronously reset outstanding
p2m_ioreq_server entries when an ioreq server unmaps") introduced
p2m_finish_type_change(), which was meant to synchronously finish a
previously initiated type change over a gpfn range.  It did this by
calling get_entry(), checking if it was the appropriate type, and then
calling set_entry().

Unfortunately, a previous commit (1679e0df3df6 "x86/ioreq server:
asynchronously reset outstanding p2m_ioreq_server entries") modified
get_entry() to always return the new type after the type change, meaning
that p2m_finish_type_change() never changed any entries.  Which means
when an ioreq server was detached and then re-attached (as happens in
XenGT on reboot) the re-attach failed.

Fix this by using the existing p2m-specific recalculation logic instead
of doing a read-check-write loop.

Fix: 'commit 6d774a951696 ("x86/ioreq server: synchronously reset
  outstanding p2m_ioreq_server entries when an ioreq server unmaps")'

v1: Add ioreq_pre_recalc query flag to get the old p2m_type.(Jan)
v2: Add p2m->recalc() hook to change gfn p2m_type. (George)
v3: Make commit message clearer. (George)

Signed-off-by: Xiong Zhang 
Signed-off-by: Yu Zhang 
---
 xen/arch/x86/hvm/dm.c |  3 +--
 xen/arch/x86/mm/p2m-ept.c |  1 +
 xen/arch/x86/mm/p2m-pt.c  |  1 +
 xen/arch/x86/mm/p2m.c | 12 ++--
 xen/include/asm-x86/p2m.h |  5 +++--
 5 files changed, 8 insertions(+), 14 deletions(-)

diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
index d72b7bd..c1627ec 100644
--- a/xen/arch/x86/hvm/dm.c
+++ b/xen/arch/x86/hvm/dm.c
@@ -412,8 +412,7 @@ static int dm_op(domid_t domid,
 first_gfn <= p2m->max_mapped_pfn )
 {
 /* Iterate p2m table for 256 gfns each time. */
-p2m_finish_type_change(d, _gfn(first_gfn), 256,
-   p2m_ioreq_server, p2m_ram_rw);
+p2m_finish_type_change(d, _gfn(first_gfn), 256);
 
 first_gfn += 256;
 
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index f37a1f2..09efba7 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -1238,6 +1238,7 @@ int ept_p2m_init(struct p2m_domain *p2m)
 
 p2m->set_entry = ept_set_entry;
 p2m->get_entry = ept_get_entry;
+p2m->recalc = resolve_misconfig;
 p2m->change_entry_type_global = ept_change_entry_type_global;
 p2m->change_entry_type_range = ept_change_entry_type_range;
 p2m->memory_type_changed = ept_memory_type_changed;
diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
index 5079b59..2eddeee 100644
--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -1153,6 +1153,7 @@ void p2m_pt_init(struct p2m_domain *p2m)
 {
 p2m->set_entry = p2m_pt_set_entry;
 p2m->get_entry = p2m_pt_get_entry;
+p2m->recalc = do_recalc;
 p2m->change_entry_type_global = p2m_pt_change_entry_type_global;
 p2m->change_entry_type_range = p2m_pt_change_entry_type_range;
 p2m->write_p2m_entry = paging_write_p2m_entry;
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 1d57e5c..2bad2e1 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1013,26 +1013,18 @@ void p2m_change_type_range(struct domain *d,
 
 /* Synchronously modify the p2m type for a range of gfns from ot to nt. */
 void p2m_finish_type_change(struct domain *d,
-gfn_t first_gfn, unsigned long max_nr,
-p2m_type_t ot, p2m_type_t nt)
+gfn_t first_gfn, unsigned long max_nr)
 {
 struct p2m_domain *p2m = p2m_get_hostp2m(d);
-p2m_type_t t;
 unsigned long gfn = gfn_x(first_gfn);
 unsigned long last_gfn = gfn + max_nr - 1;
 
-ASSERT(ot != nt);
-ASSERT(p2m_is_changeable(ot) && p2m_is_changeable(nt));
-
 p2m_lock(p2m);
 
 last_gfn = min(last_gfn, p2m->max_mapped_pfn);
 while ( gfn <= last_gfn )
 {
-get_gfn_query_unlocked(d, gfn, );
-
-if ( t == ot )
-p2m_change_type_one(d, gfn, t, nt);
+p2m->recalc(p2m, gfn);
 
 gfn++;
 }
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 7574a9b..081639c 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -246,6 +246,8 @@ struct p2m_domain {
 p2m_query_t q,
 unsigned int *page_order,
 bool_t *sve);
+int(*recalc)(struct p2m_domain *p2m,
+ unsigned long gfn);
 void   (*enable_hardware_log_dirty)(struct p2m_domain *p2m);
 void   (*disable_hardware_log_dirty)(struct p2m_domain *p2m);
 void   (*flush_hardware_cached_dirty)(struct p2m_domain *p2m);
@@ -609,8 +611,7 @@ int p2m_change_type_one(struct domain *d,