Re: [Xen-devel] [PATCH] x86/ioreq server: Fix DomU couldn't reboot when using p2m_ioreq_server p2m_type

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.05.17 at 15:55, <george.dunlap@xxxxxxxxxx> wrote:
> On 08/05/17 14:26, George Dunlap wrote:
>> On 05/05/17 15:40, Jan Beulich wrote:
>>>>>> On 05.05.17 at 05:52, <xiong.y.zhang@xxxxxxxxx> wrote:
>>>> 'commit 1679e0df3df6 ("x86/ioreq server: asynchronously reset
>>>> outstanding p2m_ioreq_server entries")' will call
>>>> p2m_change_entry_type_global() which set entry.recalc=1. Then
>>>> the following get_entry(p2m_ioreq_server) will return
>>>> p2m_ram_rw type.
>>>> But 'commit 6d774a951696 ("x86/ioreq server: synchronously reset
>>>> outstanding p2m_ioreq_server entries when an ioreq server unmaps")'
>>>> assume get_entry(p2m_ioreq_server) will return p2m_ioreq_server
>>>> type, then reset p2m_ioreq_server entries. The fact is the assumption
>>>> isn't true, and sysnchronously reset function couldn't work. Then
>>>> ioreq.entry_count is larger than zero after an ioreq server unmaps,
>>>> finally this results DomU couldn't reboot.
>>>
>>> I've had trouble understanding this part already on v1 (btw, why is
>>> this one not tagged v2?), and since I still can't figure it I have to ask:
>>> Why is it that guest reboot is being impacted here? From what I recall
>>> a non-zero count should only prevent migration.
>>>
>>>> This patch add a P2M_PRE_RECALC flag to p2m_query_t, then
>>>> get_entry(P2M_PRE_RECALC) will return p2m_ioreq_server type
>>>> for p2m_ioreq_server pfn, and finally change mem type through set_entry.
>>>
>>> This looks to be a relatively little impact change, but nevertheless
>>> I'm wondering whether someone else (George?) may be able to
>>> think of some more elegant solution (I have to admit that, having
>>> suggested the one here, I can't).
>> 
>> So the basic problem is that the implementation-specific get_entry()
>> will return the theoretical new type without changing it, and
>> finish_type_change() is in the generic code which calls these
>> implementation-specific p2m functions.
>> 
>> I think the possible solutions are:
>> 
>> 1. Add a flag to the get_entry() call to return the real entry (i.e.,
>> the solution in this patch)
>> 
>> 2. Have get_entry() update entries as it reads them
>> 
>> 3. Implement an implementation-specific finish_type_change()
> 
> Actually -- there are already implementation-specific versions for
> individual gpas: p2m-pt.c:do_recalc() and p2m-ept.c:resolve_misconfig().
>  They even already have the same function signature
> 
> If we made a new p2m hook, p2m->recalc(), we could simply loop around
> calling that instead of doing the get_entry() / set_entry() cycle.

Ah, that's a neat idea indeed.

Jan


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