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 7:28 PM, Jan Beulich wrote:
>>>> On 07.04.17 at 12:50, <yu.c.zhang@linux.intel.com> wrote:
>> 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;
> Now you _never_ change away from ioreq-server, you only adjust
> the counter.

Oh right.

Then sth. like:

diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index ecd5ceb..b960a1d 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -542,7 +542,10 @@ static int resolve_misconfig(struct p2m_domain 
*p2m, unsigned long gfn)
                                                 _mfn(e.mfn), 0, &ipat,
                                                 e.sa_p2mt == 
p2m_mmio_direct);
                      e.ipat = ipat;
-                    if ( e.recalc && p2m_is_changeable(e.sa_p2mt) )
+                    if ( e.recalc &&
+                         (p2m_check_changeable(e.sa_p2mt) ||
+                          (e.sa_p2mt == p2m_ioreq_server &&
+                           p2m->ioreq.server == NULL)) )
                      {
                           if ( e.sa_p2mt == p2m_ioreq_server )
                           {
diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
index 4b2ff9e..1c600b1 100644
--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -442,7 +442,8 @@ static int do_recalc(struct p2m_domain *p2m, 
unsigned long gfn)
              P2M_DEBUG("bogus recalc leaf at d%d:%lx:%u\n",
                        p2m->domain->domain_id, gfn, level);
          p2mt_old = p2m_flags_to_type(l1e_get_flags(e));
-        if ( p2m_is_changeable(p2mt_old) )
+        if ( p2m_check_changeable(p2mt_old) ||
+             (p2mt_old == p2m_ioreq_server && p2m->ioreq.server == NULL) )
          {
              unsigned long mask = ~0UL << (level * PAGETABLE_ORDER);
              p2m_type_t p2mt = p2m_is_logdirty_range(p2m, gfn & mask, 
gfn | ~mask)

But as you can see. I used p2m_check_changeable() here.

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


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