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 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()

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