Re: [PATCH V2] x86/ioreq server: Fix XenGT couldn't reboot when XenGT use p2m_ioreq_server p2m_type

Re: [PATCH V2] x86/ioreq server: Fix XenGT couldn't reboot when XenGT use p2m_ioreq_server p2m_type

[George Dunlap]

On 09/05/17 22:22, Xiong Zhang wrote:
> 'commit 1679e0df3df6 ("x86/ioreq server: asynchronously reset
> outstanding p2m_ioreq_server entries")' will call
> p2m_change_entry_type_global() which set entry.recalc=1. Then
> the following get_entry(p2m_ioreq_server) will return
> p2m_ram_rw type.
> But 'commit 6d774a951696 ("x86/ioreq server: synchronously reset
> outstanding p2m_ioreq_server entries when an ioreq server unmaps")'
> assume get_entry(p2m_ioreq_server) will return p2m_ioreq_server
> type, then reset p2m_ioreq_server entries. The fact is the assumption
> isn't true, and sysnchronously reset function couldn't work. Then
> ioreq.entry_count is larger than zero after an ioreq server unmaps.
> During XenGT domU reboot, it will unmap, map and unmap ioreq
> server with old domid, the map will fail as ioreq.entry_count > 0 and
> reboot process is terminated.
> 
> This patch add p2m->recalc() hook which use the existing implementation
> specific function as ept resolve_misconfig and pt do_recalc, so
> p2m_finish_type_change() could call p2m->recalc() directly to
> change gfn p2m_type which need recalc.

This looks a lot nicer!  Two things: I think I'd rewrite the changelog
this way:

---
x86/ioreq_server: Make p2m_finish_type_change actually work

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

Also...

> 
> 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)
> 
> Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com>
> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
> ---
>  xen/arch/x86/hvm/dm.c     |  3 +--
>  xen/arch/x86/mm/p2m-ept.c |  7 ++++---
>  xen/arch/x86/mm/p2m-pt.c  |  7 ++++---
>  xen/arch/x86/mm/p2m.c     | 12 ++----------
>  xen/include/asm-x86/p2m.h |  5 +++--
>  5 files changed, 14 insertions(+), 20 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..f96bd3b 100644
> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -502,7 +502,7 @@ static int ept_invalidate_emt_range(struct p2m_domain *p2m,
>   * - zero if no adjustment was done,
>   * - a positive value if at least one adjustment was done.
>   */
> -static int resolve_misconfig(struct p2m_domain *p2m, unsigned long gfn)
> +static int ept_resolve_misconfig(struct p2m_domain *p2m, unsigned long gfn)

I think while we're renaming this I'd rename this to ept_do_recalc().

Thanks,
 -George


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

Re: [PATCH V2] x86/ioreq server: Fix XenGT couldn't reboot when XenGT use p2m_ioreq_server p2m_type

[Jan Beulich]

>>> On 09.05.17 at 11:44, <george.dunlap@citrix.com> wrote:
> On 09/05/17 22:22, Xiong Zhang wrote:
>> --- a/xen/arch/x86/mm/p2m-ept.c
>> +++ b/xen/arch/x86/mm/p2m-ept.c
>> @@ -502,7 +502,7 @@ static int ept_invalidate_emt_range(struct p2m_domain *p2m,
>>   * - zero if no adjustment was done,
>>   * - a positive value if at least one adjustment was done.
>>   */
>> -static int resolve_misconfig(struct p2m_domain *p2m, unsigned long gfn)
>> +static int ept_resolve_misconfig(struct p2m_domain *p2m, unsigned long gfn)
> 
> I think while we're renaming this I'd rename this to ept_do_recalc().

Which gets me to ask (once again) what purpose the ept_ prefix
has for a static function. I'd rather see this called do_recalc(), and
the p2m-pt variant could be left unchanged altogether.

Jan


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

Re: [PATCH V2] x86/ioreq server: Fix XenGT couldn't reboot when XenGT use p2m_ioreq_server p2m_type

[George Dunlap]

On 09/05/17 11:08, Jan Beulich wrote:
>>>> On 09.05.17 at 11:44, <george.dunlap@citrix.com> wrote:
>> On 09/05/17 22:22, Xiong Zhang wrote:
>>> --- a/xen/arch/x86/mm/p2m-ept.c
>>> +++ b/xen/arch/x86/mm/p2m-ept.c
>>> @@ -502,7 +502,7 @@ static int ept_invalidate_emt_range(struct p2m_domain *p2m,
>>>   * - zero if no adjustment was done,
>>>   * - a positive value if at least one adjustment was done.
>>>   */
>>> -static int resolve_misconfig(struct p2m_domain *p2m, unsigned long gfn)
>>> +static int ept_resolve_misconfig(struct p2m_domain *p2m, unsigned long gfn)
>>
>> I think while we're renaming this I'd rename this to ept_do_recalc().
> 
> Which gets me to ask (once again) what purpose the ept_ prefix
> has for a static function. I'd rather see this called do_recalc(), and
> the p2m-pt variant could be left unchanged altogether.

Well we should have them both named do_recalc() (no prefix), or have
them both tagged to specify which version they're for.  ISTR people
complaining about duplicate static symbols making things harder to debug
(i.e., is this do_recalc() in the stack trace the p2m-pt version or the
p2m-ept version?), so the latter is probably preferable.

"p2m_pt_" does seem like kind of a long prefix, but that seems to be
what the rest of the p2m_pt.c functions are called, so at this point
it's probably best to follow suit.

 -George

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

Re: [PATCH V2] x86/ioreq server: Fix XenGT couldn't reboot when XenGT use p2m_ioreq_server p2m_type

[Zhang, Xiong Y]

> >>> On 09.05.17 at 11:44, <george.dunlap@citrix.com> wrote:
> > On 09/05/17 22:22, Xiong Zhang wrote:
> >> --- a/xen/arch/x86/mm/p2m-ept.c
> >> +++ b/xen/arch/x86/mm/p2m-ept.c
> >> @@ -502,7 +502,7 @@ static int ept_invalidate_emt_range(struct
> p2m_domain *p2m,
> >>   * - zero if no adjustment was done,
> >>   * - a positive value if at least one adjustment was done.
> >>   */
> >> -static int resolve_misconfig(struct p2m_domain *p2m, unsigned long gfn)
> >> +static int ept_resolve_misconfig(struct p2m_domain *p2m, unsigned long
> gfn)
> >
> > I think while we're renaming this I'd rename this to ept_do_recalc().
> 
> Which gets me to ask (once again) what purpose the ept_ prefix
> has for a static function. I'd rather see this called do_recalc(), and
> the p2m-pt variant could be left unchanged altogether.
> 
[Zhang, Xiong Y] As all the functions with p2m have ept_ prefix in
p2m-ept.c and have p2m_pt_ prefix in p2m-pt.c, then I guess there
may be a potential rule to name these functions. If there isn't such 
rule, I will keep their name unchanged. Thanks.
> Jan


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

Re: [PATCH V2] x86/ioreq server: Fix XenGT couldn't reboot when XenGT use p2m_ioreq_server p2m_type

[Jan Beulich]

>>> On 09.05.17 at 12:21, <george.dunlap@citrix.com> wrote:
> On 09/05/17 11:08, Jan Beulich wrote:
>>>>> On 09.05.17 at 11:44, <george.dunlap@citrix.com> wrote:
>>> On 09/05/17 22:22, Xiong Zhang wrote:
>>>> --- a/xen/arch/x86/mm/p2m-ept.c
>>>> +++ b/xen/arch/x86/mm/p2m-ept.c
>>>> @@ -502,7 +502,7 @@ static int ept_invalidate_emt_range(struct p2m_domain 
> *p2m,
>>>>   * - zero if no adjustment was done,
>>>>   * - a positive value if at least one adjustment was done.
>>>>   */
>>>> -static int resolve_misconfig(struct p2m_domain *p2m, unsigned long gfn)
>>>> +static int ept_resolve_misconfig(struct p2m_domain *p2m, unsigned long gfn)
>>>
>>> I think while we're renaming this I'd rename this to ept_do_recalc().
>> 
>> Which gets me to ask (once again) what purpose the ept_ prefix
>> has for a static function. I'd rather see this called do_recalc(), and
>> the p2m-pt variant could be left unchanged altogether.
> 
> Well we should have them both named do_recalc() (no prefix), or have
> them both tagged to specify which version they're for.  ISTR people
> complaining about duplicate static symbols making things harder to debug
> (i.e., is this do_recalc() in the stack trace the p2m-pt version or the
> p2m-ept version?), so the latter is probably preferable.

But that's the reason I had done d37d63d4b5 ("symbols: prefix static
symbols with their source file names") - they are distinguishable in
stack traces nowadays.

Jan


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

Re: [PATCH V2] x86/ioreq server: Fix XenGT couldn't reboot when XenGT use p2m_ioreq_server p2m_type

[George Dunlap]

On 09/05/17 11:51, Jan Beulich wrote:
>>>> On 09.05.17 at 12:21, <george.dunlap@citrix.com> wrote:
>> On 09/05/17 11:08, Jan Beulich wrote:
>>>>>> On 09.05.17 at 11:44, <george.dunlap@citrix.com> wrote:
>>>> On 09/05/17 22:22, Xiong Zhang wrote:
>>>>> --- a/xen/arch/x86/mm/p2m-ept.c
>>>>> +++ b/xen/arch/x86/mm/p2m-ept.c
>>>>> @@ -502,7 +502,7 @@ static int ept_invalidate_emt_range(struct p2m_domain 
>> *p2m,
>>>>>   * - zero if no adjustment was done,
>>>>>   * - a positive value if at least one adjustment was done.
>>>>>   */
>>>>> -static int resolve_misconfig(struct p2m_domain *p2m, unsigned long gfn)
>>>>> +static int ept_resolve_misconfig(struct p2m_domain *p2m, unsigned long gfn)
>>>>
>>>> I think while we're renaming this I'd rename this to ept_do_recalc().
>>>
>>> Which gets me to ask (once again) what purpose the ept_ prefix
>>> has for a static function. I'd rather see this called do_recalc(), and
>>> the p2m-pt variant could be left unchanged altogether.
>>
>> Well we should have them both named do_recalc() (no prefix), or have
>> them both tagged to specify which version they're for.  ISTR people
>> complaining about duplicate static symbols making things harder to debug
>> (i.e., is this do_recalc() in the stack trace the p2m-pt version or the
>> p2m-ept version?), so the latter is probably preferable.
> 
> But that's the reason I had done d37d63d4b5 ("symbols: prefix static
> symbols with their source file names") - they are distinguishable in
> stack traces nowadays.

Ah, right.  In that case, Xiong, go ahead and leave the two functions
named as the are.  I plan at some point in the future to do a wider
renaming if "resolve_misconfig" in p2m-ept.c, so I can change the name
of all the related functions at the same time.

 -George


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

[PATCH V2] x86/ioreq server: Fix XenGT couldn't reboot when XenGT use p2m_ioreq_server p2m_type

[Xiong Zhang]

'commit 1679e0df3df6 ("x86/ioreq server: asynchronously reset
outstanding p2m_ioreq_server entries")' will call
p2m_change_entry_type_global() which set entry.recalc=1. Then
the following get_entry(p2m_ioreq_server) will return
p2m_ram_rw type.
But 'commit 6d774a951696 ("x86/ioreq server: synchronously reset
outstanding p2m_ioreq_server entries when an ioreq server unmaps")'
assume get_entry(p2m_ioreq_server) will return p2m_ioreq_server
type, then reset p2m_ioreq_server entries. The fact is the assumption
isn't true, and sysnchronously reset function couldn't work. Then
ioreq.entry_count is larger than zero after an ioreq server unmaps.
During XenGT domU reboot, it will unmap, map and unmap ioreq
server with old domid, the map will fail as ioreq.entry_count > 0 and
reboot process is terminated.

This patch add p2m->recalc() hook which use the existing implementation
specific function as ept resolve_misconfig and pt do_recalc, so
p2m_finish_type_change() could call p2m->recalc() directly to
change gfn p2m_type which need recalc.

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)

Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com>
Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
---
 xen/arch/x86/hvm/dm.c     |  3 +--
 xen/arch/x86/mm/p2m-ept.c |  7 ++++---
 xen/arch/x86/mm/p2m-pt.c  |  7 ++++---
 xen/arch/x86/mm/p2m.c     | 12 ++----------
 xen/include/asm-x86/p2m.h |  5 +++--
 5 files changed, 14 insertions(+), 20 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..f96bd3b 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -502,7 +502,7 @@ static int ept_invalidate_emt_range(struct p2m_domain *p2m,
  * - zero if no adjustment was done,
  * - a positive value if at least one adjustment was done.
  */
-static int resolve_misconfig(struct p2m_domain *p2m, unsigned long gfn)
+static int ept_resolve_misconfig(struct p2m_domain *p2m, unsigned long gfn)
 {
     struct ept_data *ept = &p2m->ept;
     unsigned int level = ept->wl;
@@ -659,7 +659,7 @@ bool_t ept_handle_misconfig(uint64_t gpa)
     p2m_lock(p2m);
 
     spurious = curr->arch.hvm_vmx.ept_spurious_misconfig;
-    rc = resolve_misconfig(p2m, PFN_DOWN(gpa));
+    rc = ept_resolve_misconfig(p2m, PFN_DOWN(gpa));
     curr->arch.hvm_vmx.ept_spurious_misconfig = 0;
 
     p2m_unlock(p2m);
@@ -707,7 +707,7 @@ ept_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
         return -EINVAL;
 
     /* Carry out any eventually pending earlier changes first. */
-    ret = resolve_misconfig(p2m, gfn);
+    ret = ept_resolve_misconfig(p2m, gfn);
     if ( ret < 0 )
         return ret;
 
@@ -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 = ept_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..b0f6aa0 100644
--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -367,7 +367,7 @@ static int p2m_pt_set_recalc_range(struct p2m_domain *p2m,
  * GFN. Propagate the re-calculation flag down to the next page table level
  * for entries not involved in the translation of the given GFN.
  */
-static int do_recalc(struct p2m_domain *p2m, unsigned long gfn)
+static int p2m_pt_do_recalc(struct p2m_domain *p2m, unsigned long gfn)
 {
     void *table;
     unsigned long gfn_remainder = gfn;
@@ -493,7 +493,7 @@ int p2m_pt_handle_deferred_changes(uint64_t gpa)
     int rc;
 
     p2m_lock(p2m);
-    rc = do_recalc(p2m, PFN_DOWN(gpa));
+    rc = p2m_pt_do_recalc(p2m, PFN_DOWN(gpa));
     p2m_unlock(p2m);
 
     return rc;
@@ -555,7 +555,7 @@ p2m_pt_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
     }
 
     /* Carry out any eventually pending earlier changes first. */
-    rc = do_recalc(p2m, gfn);
+    rc = p2m_pt_do_recalc(p2m, gfn);
     if ( rc < 0 )
         return rc;
 
@@ -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 = p2m_pt_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, &t);
-
-        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, unsigned long gfn,
 /* Synchronously change the p2m type for a range of gfns */
 void p2m_finish_type_change(struct domain *d,
                             gfn_t first_gfn,
-                            unsigned long max_nr,
-                            p2m_type_t ot, p2m_type_t nt);
+                            unsigned long max_nr);
 
 /* Report a change affecting memory types. */
 void p2m_memory_type_changed(struct domain *d);
-- 
2.7.4


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

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

[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 <xiong.y.zhang@intel.com>
> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>

---
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 <george.dunlap@citrix.com>


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

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

[Jan Beulich]

>>> On 11.05.17 at 00:57, <xiong.y.zhang@intel.com> 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 <xiong.y.zhang@intel.com>
> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
> ---

... 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, &t);
> -
> -        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

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

[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 <xiong.y.zhang@intel.com>
Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
---
 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, &t);
-
-        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, unsigned long gfn,
 /* Synchronously change the p2m type for a range of gfns */
 void p2m_finish_type_change(struct domain *d,
                             gfn_t first_gfn,
-                            unsigned long max_nr,
-                            p2m_type_t ot, p2m_type_t nt);
+                            unsigned long max_nr);
 
 /* Report a change affecting memory types. */
 void p2m_memory_type_changed(struct domain *d);
-- 
2.7.4


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

Re: [PATCH V4] x86/ioreq_server: Make p2m_finish_type_change actually work

[Jan Beulich]

>>> On 12.05.17 at 04:42, <xiong.y.zhang@intel.com> 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")'
> 
> Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com>
> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
> Reviewed-by: George Dunlap <george.dunlap@ctrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
albeit I'm not overly happy with ...

> +int p2m_finish_type_change(struct domain *d,
> +                           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));
> +    int rc = 0;
>  
>      p2m_lock(p2m);
>  
>      last_gfn = min(last_gfn, p2m->max_mapped_pfn);
>      while ( gfn <= last_gfn )
>      {
> -        get_gfn_query_unlocked(d, gfn, &t);
> -
> -        if ( t == ot )
> -            p2m_change_type_one(d, gfn, t, nt);
> +        rc = p2m->recalc(p2m, gfn);
> +        /* 
> +         * ept->recalc could return 0/1/-ENOMEM. pt->recalc could return
> +         * 0/-ENOMEM/-ENOENT, -ENOENT isn't an error as we are looping
> +         * gfn here.
> +         */
> +        if ( rc == -ENOENT)
> +            rc = 0;
> +        else if ( rc < 0 )
> +        {
> +            gdprintk(XENLOG_ERR, "p2m->recalc failed! Dom%d gfn=%lx\n",
> +                     d->domain_id, gfn);

... a message being logged here.

Also I'm sure it was pointed out before that if this is meant for
4.9 (which I assume it is) you should have Cc-ed Julien (now
added).

Jan


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

Re: [PATCH V4] x86/ioreq_server: Make p2m_finish_type_change actually work

[Julien Grall]

Hi,

On 11/05/17 12:12, Jan Beulich wrote:
>>>> On 12.05.17 at 04:42, <xiong.y.zhang@intel.com> 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")'
>>
>> Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com>
>> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
>> Reviewed-by: George Dunlap <george.dunlap@ctrix.com>
>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> albeit I'm not overly happy with ...
>
>> +int p2m_finish_type_change(struct domain *d,
>> +                           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));
>> +    int rc = 0;
>>
>>      p2m_lock(p2m);
>>
>>      last_gfn = min(last_gfn, p2m->max_mapped_pfn);
>>      while ( gfn <= last_gfn )
>>      {
>> -        get_gfn_query_unlocked(d, gfn, &t);
>> -
>> -        if ( t == ot )
>> -            p2m_change_type_one(d, gfn, t, nt);
>> +        rc = p2m->recalc(p2m, gfn);
>> +        /*
>> +         * ept->recalc could return 0/1/-ENOMEM. pt->recalc could return
>> +         * 0/-ENOMEM/-ENOENT, -ENOENT isn't an error as we are looping
>> +         * gfn here.
>> +         */
>> +        if ( rc == -ENOENT)

NIT: space missing before )

>> +            rc = 0;
>> +        else if ( rc < 0 )
>> +        {
>> +            gdprintk(XENLOG_ERR, "p2m->recalc failed! Dom%d gfn=%lx\n",
>> +                     d->domain_id, gfn);
>
> ... a message being logged here.
>
> Also I'm sure it was pointed out before that if this is meant for
> 4.9 (which I assume it is) you should have Cc-ed Julien (now
> added).

Xiong Zhang, can you confirm this is meant for Xen 4.9?

Cheers,

-- 
Julien Grall

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

[PATCH V4] x86/ioreq_server: Make p2m_finish_type_change actually work

[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")'

Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com>
Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
Reviewed-by: George Dunlap <george.dunlap@ctrix.com>
---
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)
    Keep the name of p2m-specific recal function unchanged. (Jan)
v4: Move version info below S-o-B and handle return value of
    p2m->recalc. (Jan)
---
 xen/arch/x86/hvm/dm.c     |  5 +++--
 xen/arch/x86/mm/p2m-ept.c |  1 +
 xen/arch/x86/mm/p2m-pt.c  |  1 +
 xen/arch/x86/mm/p2m.c     | 35 +++++++++++++++++++++++------------
 xen/include/asm-x86/p2m.h |  7 ++++---
 5 files changed, 32 insertions(+), 17 deletions(-)

diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
index d72b7bd..99bf66a 100644
--- a/xen/arch/x86/hvm/dm.c
+++ b/xen/arch/x86/hvm/dm.c
@@ -412,8 +412,9 @@ 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);
+                rc = p2m_finish_type_change(d, _gfn(first_gfn), 256);
+                if ( rc < 0 )
+                    break;
 
                 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..668c5a6 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1011,33 +1011,44 @@ void p2m_change_type_range(struct domain *d,
     p2m_unlock(p2m);
 }
 
-/* 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)
+/*
+ * Finish p2m type change for gfns which are marked as need_recalc in a range.
+ * Returns: 0/1 for success, negative for failure
+ */
+int p2m_finish_type_change(struct domain *d,
+                           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));
+    int rc = 0;
 
     p2m_lock(p2m);
 
     last_gfn = min(last_gfn, p2m->max_mapped_pfn);
     while ( gfn <= last_gfn )
     {
-        get_gfn_query_unlocked(d, gfn, &t);
-
-        if ( t == ot )
-            p2m_change_type_one(d, gfn, t, nt);
+        rc = p2m->recalc(p2m, gfn);
+        /* 
+         * ept->recalc could return 0/1/-ENOMEM. pt->recalc could return
+         * 0/-ENOMEM/-ENOENT, -ENOENT isn't an error as we are looping
+         * gfn here.
+         */
+        if ( rc == -ENOENT)
+            rc = 0;
+        else if ( rc < 0 )
+        {
+            gdprintk(XENLOG_ERR, "p2m->recalc failed! Dom%d gfn=%lx\n",
+                     d->domain_id, gfn);
+            break;
+        }
 
         gfn++;
     }
 
     p2m_unlock(p2m);
+
+    return rc;
 }
 
 /*
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 7574a9b..d7d47fe 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);
@@ -607,10 +609,9 @@ int p2m_change_type_one(struct domain *d, unsigned long gfn,
                         p2m_type_t ot, p2m_type_t nt);
 
 /* Synchronously change the p2m type for a range of gfns */
-void p2m_finish_type_change(struct domain *d,
+int p2m_finish_type_change(struct domain *d,
                             gfn_t first_gfn,
-                            unsigned long max_nr,
-                            p2m_type_t ot, p2m_type_t nt);
+                            unsigned long max_nr);
 
 /* Report a change affecting memory types. */
 void p2m_memory_type_changed(struct domain *d);
-- 
2.7.4


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

[PATCH V5] x86/ioreq_server: Make p2m_finish_type_change actually work

[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")'

Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com>
Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
Reviewed-by: George Dunlap <george.dunlap@ctrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
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)
    Keep the name of p2m-specific recal function unchanged. (Jan)
v4: Move version info below S-o-B and handle return value of
    p2m->recalc. (Jan)
v5: Fix coding style. (Julien)

The target of this patch is Xen 4.9.
---
 xen/arch/x86/hvm/dm.c     |  5 +++--
 xen/arch/x86/mm/p2m-ept.c |  1 +
 xen/arch/x86/mm/p2m-pt.c  |  1 +
 xen/arch/x86/mm/p2m.c     | 35 +++++++++++++++++++++++------------
 xen/include/asm-x86/p2m.h |  9 +++++----
 5 files changed, 33 insertions(+), 18 deletions(-)

diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
index d72b7bd..99bf66a 100644
--- a/xen/arch/x86/hvm/dm.c
+++ b/xen/arch/x86/hvm/dm.c
@@ -412,8 +412,9 @@ 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);
+                rc = p2m_finish_type_change(d, _gfn(first_gfn), 256);
+                if ( rc < 0 )
+                    break;
 
                 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..1600422 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1011,33 +1011,44 @@ void p2m_change_type_range(struct domain *d,
     p2m_unlock(p2m);
 }
 
-/* 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)
+/*
+ * Finish p2m type change for gfns which are marked as need_recalc in a range.
+ * Returns: 0/1 for success, negative for failure
+ */
+int p2m_finish_type_change(struct domain *d,
+                           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));
+    int rc = 0;
 
     p2m_lock(p2m);
 
     last_gfn = min(last_gfn, p2m->max_mapped_pfn);
     while ( gfn <= last_gfn )
     {
-        get_gfn_query_unlocked(d, gfn, &t);
-
-        if ( t == ot )
-            p2m_change_type_one(d, gfn, t, nt);
+        rc = p2m->recalc(p2m, gfn);
+        /*
+         * ept->recalc could return 0/1/-ENOMEM. pt->recalc could return
+         * 0/-ENOMEM/-ENOENT, -ENOENT isn't an error as we are looping
+         * gfn here.
+         */
+        if ( rc == -ENOENT )
+            rc = 0;
+        else if ( rc < 0 )
+        {
+            gdprintk(XENLOG_ERR, "p2m->recalc failed! Dom%d gfn=%lx\n",
+                     d->domain_id, gfn);
+            break;
+        }
 
         gfn++;
     }
 
     p2m_unlock(p2m);
+
+    return rc;
 }
 
 /*
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 7574a9b..408f7da 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);
@@ -607,10 +609,9 @@ int p2m_change_type_one(struct domain *d, unsigned long gfn,
                         p2m_type_t ot, p2m_type_t nt);
 
 /* Synchronously change the p2m type for a range of gfns */
-void p2m_finish_type_change(struct domain *d,
-                            gfn_t first_gfn,
-                            unsigned long max_nr,
-                            p2m_type_t ot, p2m_type_t nt);
+int p2m_finish_type_change(struct domain *d,
+                           gfn_t first_gfn,
+                           unsigned long max_nr);
 
 /* Report a change affecting memory types. */
 void p2m_memory_type_changed(struct domain *d);
-- 
2.7.4


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

Re: [PATCH V4] x86/ioreq_server: Make p2m_finish_type_change actually work

[George Dunlap]

On Fri, May 12, 2017 at 3:42 AM, Xiong Zhang <xiong.y.zhang@intel.com> 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")'
>
> Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com>
> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
> Reviewed-by: George Dunlap <george.dunlap@ctrix.com>
> ---
> 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)
>     Keep the name of p2m-specific recal function unchanged. (Jan)
> v4: Move version info below S-o-B and handle return value of
>     p2m->recalc. (Jan)

Sorry to be picky, but the handling of the return value introduces
another way the patch could be incorrect, so you should have dropped
my R-b.

I'll respond to v5.

 -George

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

Re: [PATCH V5] x86/ioreq_server: Make p2m_finish_type_change actually work

[George Dunlap]

On Sat, May 13, 2017 at 1:34 AM, Xiong Zhang <xiong.y.zhang@intel.com> 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")'
>
> Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com>
> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
> Reviewed-by: George Dunlap <george.dunlap@ctrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> ---
> 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)
>     Keep the name of p2m-specific recal function unchanged. (Jan)
> v4: Move version info below S-o-B and handle return value of
>     p2m->recalc. (Jan)
> v5: Fix coding style. (Julien)
>
> The target of this patch is Xen 4.9.
> ---
>  xen/arch/x86/hvm/dm.c     |  5 +++--
>  xen/arch/x86/mm/p2m-ept.c |  1 +
>  xen/arch/x86/mm/p2m-pt.c  |  1 +
>  xen/arch/x86/mm/p2m.c     | 35 +++++++++++++++++++++++------------
>  xen/include/asm-x86/p2m.h |  9 +++++----
>  5 files changed, 33 insertions(+), 18 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
> index d72b7bd..99bf66a 100644
> --- a/xen/arch/x86/hvm/dm.c
> +++ b/xen/arch/x86/hvm/dm.c
> @@ -412,8 +412,9 @@ 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);
> +                rc = p2m_finish_type_change(d, _gfn(first_gfn), 256);
> +                if ( rc < 0 )
> +                    break;
>
>                  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..1600422 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1011,33 +1011,44 @@ void p2m_change_type_range(struct domain *d,
>      p2m_unlock(p2m);
>  }
>
> -/* 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)
> +/*
> + * Finish p2m type change for gfns which are marked as need_recalc in a range.
> + * Returns: 0/1 for success, negative for failure
> + */
> +int p2m_finish_type_change(struct domain *d,
> +                           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));
> +    int rc = 0;
>
>      p2m_lock(p2m);
>
>      last_gfn = min(last_gfn, p2m->max_mapped_pfn);
>      while ( gfn <= last_gfn )
>      {
> -        get_gfn_query_unlocked(d, gfn, &t);
> -
> -        if ( t == ot )
> -            p2m_change_type_one(d, gfn, t, nt);
> +        rc = p2m->recalc(p2m, gfn);
> +        /*
> +         * ept->recalc could return 0/1/-ENOMEM. pt->recalc could return
> +         * 0/-ENOMEM/-ENOENT, -ENOENT isn't an error as we are looping
> +         * gfn here.
> +         */

Hmm, now that these functions are being called externally it would be
good if their semantics was the same.  But since we're a bit late in
the cycle to do that kind of rework:

Acked-by: George Dunlap <george.dunlap@citrix.com>

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

Re: [PATCH V5] x86/ioreq_server: Make p2m_finish_type_change actually work

[Julien Grall]

Hi,

On 15/05/17 14:47, George Dunlap wrote:
> On Sat, May 13, 2017 at 1:34 AM, Xiong Zhang <xiong.y.zhang@intel.com> 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")'
>>
>> Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com>
>> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
>> Reviewed-by: George Dunlap <george.dunlap@ctrix.com>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> 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)
>>     Keep the name of p2m-specific recal function unchanged. (Jan)
>> v4: Move version info below S-o-B and handle return value of
>>     p2m->recalc. (Jan)
>> v5: Fix coding style. (Julien)
>>
>> The target of this patch is Xen 4.9.
>> ---
>>  xen/arch/x86/hvm/dm.c     |  5 +++--
>>  xen/arch/x86/mm/p2m-ept.c |  1 +
>>  xen/arch/x86/mm/p2m-pt.c  |  1 +
>>  xen/arch/x86/mm/p2m.c     | 35 +++++++++++++++++++++++------------
>>  xen/include/asm-x86/p2m.h |  9 +++++----
>>  5 files changed, 33 insertions(+), 18 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
>> index d72b7bd..99bf66a 100644
>> --- a/xen/arch/x86/hvm/dm.c
>> +++ b/xen/arch/x86/hvm/dm.c
>> @@ -412,8 +412,9 @@ 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);
>> +                rc = p2m_finish_type_change(d, _gfn(first_gfn), 256);
>> +                if ( rc < 0 )
>> +                    break;
>>
>>                  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..1600422 100644
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -1011,33 +1011,44 @@ void p2m_change_type_range(struct domain *d,
>>      p2m_unlock(p2m);
>>  }
>>
>> -/* 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)
>> +/*
>> + * Finish p2m type change for gfns which are marked as need_recalc in a range.
>> + * Returns: 0/1 for success, negative for failure
>> + */
>> +int p2m_finish_type_change(struct domain *d,
>> +                           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));
>> +    int rc = 0;
>>
>>      p2m_lock(p2m);
>>
>>      last_gfn = min(last_gfn, p2m->max_mapped_pfn);
>>      while ( gfn <= last_gfn )
>>      {
>> -        get_gfn_query_unlocked(d, gfn, &t);
>> -
>> -        if ( t == ot )
>> -            p2m_change_type_one(d, gfn, t, nt);
>> +        rc = p2m->recalc(p2m, gfn);
>> +        /*
>> +         * ept->recalc could return 0/1/-ENOMEM. pt->recalc could return
>> +         * 0/-ENOMEM/-ENOENT, -ENOENT isn't an error as we are looping
>> +         * gfn here.
>> +         */
>
> Hmm, now that these functions are being called externally it would be
> good if their semantics was the same.  But since we're a bit late in
> the cycle to do that kind of rework:

I guess this is a call for a rework after the tree has opened?

>
> Acked-by: George Dunlap <george.dunlap@citrix.com>
>

Release-acked-by: Julien Grall <julien.grall@arm.com>

Cheers,

-- 
Julien Grall

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