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:50, <yu.c.zhang@xxxxxxxxxxxxxxx> 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@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.
> 
> 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.

Jan


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