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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -991,8 +991,11 @@ static mfn_t ept_get_entry(struct p2m_domain *p2m,
>  
>      if ( is_epte_valid(ept_entry) )
>      {
> -        *t = p2m_recalc_type(recalc || ept_entry->recalc,
> -                             ept_entry->sa_p2mt, p2m, gfn);
> +        if ( !(q & P2M_PRE_RECALC) )

Here and elsewhere you want to use likely(). And in fact I wonder
whether overall it wouldn't be better to pass q (or just the boolean
resulting from q & P2M_PRE_RECALC) to p2m_recalc_type(),
avoiding these recurring if/else-s you add.

> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1020,6 +1020,8 @@ void p2m_finish_type_change(struct domain *d,
>      p2m_type_t t;
>      unsigned long gfn = gfn_x(first_gfn);
>      unsigned long last_gfn = gfn + max_nr - 1;
> +    mfn_t mfn;
> +    p2m_access_t a;

Please put these variable declarations ...

> @@ -1029,10 +1031,10 @@ void p2m_finish_type_change(struct domain *d,
>      last_gfn = min(last_gfn, p2m->max_mapped_pfn);
>      while ( gfn <= last_gfn )
>      {

... here.

Jan


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