diff mbox

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

Message ID 58E78368.9020904@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Yu Zhang April 7, 2017, 12:17 p.m. UTC 
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:

              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

Comments

Jan Beulich April 7, 2017, 12:36 p.m. UTC | #1 
>>> On 07.04.17 at 14:17, <yu.c.zhang@linux.intel.com> wrote:
> On 4/7/2017 7:28 PM, Jan Beulich wrote:
>>>>> On 07.04.17 at 12:50, <yu.c.zhang@linux.intel.com> wrote:
>>> @@ -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.

Oh, wrong (i.e. I was wrong), you do change the type, and did
overlook that extra new line. So your first suggestion (still visible
above) seems right to me now.

Jan
diff mbox

Patch

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