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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.04.17 at 12:14, <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:
> On 4/7/2017 5:40 PM, Jan Beulich wrote:
>>>>> On 06.04.17 at 17:53, <yu.c.zhang@xxxxxxxxxxxxxxx> 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. :)

You didn't get my point then: The problem is when there still _is_
an ioreq server. You're changing the type even in that case.

Jan


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