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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

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.

One advantage of doing this is that those functions automatically handle
superpages, whereas the patch you send here resets all entries to 4k.

Ideally we'd modify the functions to return the order of the entry
changed to make the loop more efficient, but that should probably wait
until after 4.9.

 -George

> 
> Of the three, there's something attractive about #2; but it would take a
> lot of careful thought.  In theory there shouldn't be any harm in
> changing the type of an entry marked "recalc", but with so many callers
> it's difficult to be sure.  I'd never want to make such a change this
> close to a release.
> 
> #3 will introduce a lot of unnecessary code duplication; so I think for
> now #1 is probably the best idea (in particular during a code freeze).
> 
>  -George
> 


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