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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

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@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. :)

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.

 -George

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