Re: [PATCH v12 5/6] x86/ioreq server: Asynchronously reset outstanding p2m_ioreq_server entries.

[Yu Zhang <yu.c.zhang@linux.intel.com>]

On 4/7/2017 6:28 PM, George Dunlap wrote:
> On 07/04/17 11:14, Yu Zhang wrote:
>>
>> On 4/7/2017 5:40 PM, Jan Beulich wrote:
>>>>>> On 06.04.17 at 17:53, <yu.c.zhang@linux.intel.com> wrote:
>>>> --- a/xen/arch/x86/mm/p2m-ept.c
>>>> +++ b/xen/arch/x86/mm/p2m-ept.c
>>>> @@ -544,6 +544,12 @@ static int resolve_misconfig(struct p2m_domain
>>>> *p2m, unsigned long gfn)
>>>>                        e.ipat = ipat;
>>>>                        if ( e.recalc && p2m_is_changeable(e.sa_p2mt) )
>>>>                        {
>>>> +                         if ( e.sa_p2mt == p2m_ioreq_server )
>>>> +                         {
>>>> +                             ASSERT(p2m->ioreq.entry_count > 0);
>>>> +                             p2m->ioreq.entry_count--;
>>>> +                         }
>>>> +
>>>>                             e.sa_p2mt = p2m_is_logdirty_range(p2m, gfn
>>>> + i, gfn + i)
>>>>                                         ? p2m_ram_logdirty : p2m_ram_rw;
>>> I don't think this can be right: Why would it be valid to change the
>>> type from p2m_ioreq_server to p2m_ram_rw (or p2m_ram_logdirty)
>>> here, without taking into account further information? This code
>>> can run at any time, not just when you want to reset things. So at
>>> the very least there is a check missing whether a suitable ioreq
>>> server still exists (and only if it doesn't you want to do the type
>>> reset).
>> Also I do not think we need to check if a suitable ioreq server still
>> exists. We have guaranteed
>> in our patch that no new ioreq server will be mapped as long as the p2m
>> table is not clean. :)
> Jan is saying that you should only change ioreq_server -> ram if there
> is *not* an ioreq server; and that if this is called with an ioreq
> server still active, then it must be some other change you're looking at.
>
> The problem, though, is that misconfiguration happens in many
> circumstances.  Grep for "memory_type_changed()" -- each of those
> results in a recalculation of the entire p2m, which will (in the current
> code) wipe out any ioreq_server entries.

Well, I'm not aware that other actions besides the logdirty will cause 
the reset.
But if that would happen, will below change solve this?

@@ -546,12 +546,16 @@ static int resolve_misconfig(struct p2m_domain 
*p2m, unsigned long gfn)
                      {
                           if ( e.sa_p2mt == p2m_ioreq_server )
                           {
-                             ASSERT(p2m->ioreq.entry_count > 0);
-                             p2m->ioreq.entry_count--;
+                             if ( p2m->ioreq.server == NULL )
+                             {
+                                 ASSERT(p2m->ioreq.entry_count > 0);
+                                 p2m->ioreq.entry_count--;
+                                 e.sa_p2mt = p2m_ram_rw;
+                             }
                           }
-
-                         e.sa_p2mt = p2m_is_logdirty_range(p2m, gfn + 
i, gfn + i)
-                                     ? p2m_ram_logdirty : p2m_ram_rw;
+                         else
+                             e.sa_p2mt = p2m_is_logdirty_range(p2m, gfn 
+ i, gfn + i)
+                                         ? p2m_ram_logdirty : p2m_ram_rw;
                           ept_p2m_type_to_flags(p2m, &e, e.sa_p2mt, 
e.access);
                      }
                      e.recalc = 0;

Yu
>
>   -George
>


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