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

["Zhang, Xiong Y" <xiong.y.zhang@xxxxxxxxx>]

> >>> 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.
[Zhang, Xiong Y] Sorry, although they solve the same issue, the solution is
totally different, so I didn't mark this as V2, I will mark the following
as v2 with this solution.
During DomU reboot, it will first unmap ioreq server in shutdown process,
then it call map ioreq server in boot process. The following sentence in
p2m_set_ioreq_server() result mapping ioreq server failure, then DomU
couldn't continue booting.
If ( read_atomic(&p->ioreq.entry_count))
   goto out;
> 
> > 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.
[Zhang, Xiong Y] Good suggestion, I will pass (q & P2M_PRE_RECALC) to
recalc bool parameter in p2m_recalc_type();
> 
> > --- 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.
[Zhang, Xiong Y] ok, thanks
> 
> Jan


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