Re: [Xen-devel] [PATCH v9 4/5] x86/ioreq server: Asynchronously reset outstanding p2m_ioreq_server entries.

[Yu Zhang <yu.c.zhang@xxxxxxxxxxxxxxx>]

On 3/22/2017 4:10 PM, Tian, Kevin wrote:
> > From: Yu Zhang [mailto:yu.c.zhang@xxxxxxxxxxxxxxx]
> > Sent: Tuesday, March 21, 2017 10:53 AM
> >
> > After an ioreq server has unmapped, the remaining p2m_ioreq_server
> > entries need to be reset back to p2m_ram_rw. This patch does this
> > asynchronously with the current p2m_change_entry_type_global() interface.
> >
> > This patch also disallows live migration, when there's still any outstanding
> > p2m_ioreq_server entry left. The core reason is our current implementation
> > of p2m_change_entry_type_global() can not tell the state of
> > p2m_ioreq_server entries(can not decide if an entry is to be emulated or to
> > be resynced).
> Don't quite get this point. change_global is triggered only upon
> unmap. At that point there is no ioreq server to emulate the
> write operations on those entries. All the things required is
> just to change the type. What's the exact decision required here?
Well, one situation I can recall is that if another ioreq server maps to 
this type,
and live migration happens later. The resolve_misconfig() code cannot 
differentiate
if an p2m_ioreq_server page is an obsolete to be synced, or a new one to 
be only
emulated.

I gave some explanation on this issue in discussion during Jun 20 - 22 last
year.

http://lists.xenproject.org/archives/html/xen-devel/2016-06/msg02426.html
on Jun 20
and
http://lists.xenproject.org/archives/html/xen-devel/2016-06/msg02575.html
on Jun 21

> btw does it mean that live migration can be still supported as long as
> device model proactively unmaps write-protected pages before
> starting live migration?
Yes.

> > Note: new field entry_count is introduced in struct p2m_domain, to record
> > the number of p2m_ioreq_server p2m page table entries.
> > One nature of these entries is that they only point to 4K sized page frames,
> > because all p2m_ioreq_server entries are originated from p2m_ram_rw ones
> > in p2m_change_type_one(). We do not need to worry about the counting for
> > 2M/1G sized pages.
> >
> > Signed-off-by: Yu Zhang <yu.c.zhang@xxxxxxxxxxxxxxx>
> > ---
> > Cc: Paul Durrant <paul.durrant@xxxxxxxxxx>
> > Cc: Jan Beulich <jbeulich@xxxxxxxx>
> > Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > Cc: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> > Cc: Jun Nakajima <jun.nakajima@xxxxxxxxx>
> > Cc: Kevin Tian <kevin.tian@xxxxxxxxx>
> >
> > changes in v4:
> >    - According to comments from Jan: use ASSERT() instead of 'if'
> >      condition in p2m_change_type_one().
> >    - According to comments from Jan: commit message changes to mention
> >      the p2m_ioreq_server are all based on 4K sized pages.
> >
> > changes in v3:
> >    - Move the synchronously resetting logic into patch 5.
> >    - According to comments from Jan: introduce p2m_check_changeable()
> >      to clarify the p2m type change code.
> >    - According to comments from George: use locks in the same order
> >      to avoid deadlock, call p2m_change_entry_type_global() after unmap
> >      of the ioreq server is finished.
> >
> > changes in v2:
> >    - Move the calculation of ioreq server page entry_cout into
> >      p2m_change_type_one() so that we do not need a seperate lock.
> >      Note: entry_count is also calculated in resolve_misconfig()/
> >      do_recalc(), fortunately callers of both routines have p2m
> >      lock protected already.
> >    - Simplify logic in hvmop_set_mem_type().
> >    - Introduce routine p2m_finish_type_change() to walk the p2m
> >      table and do the p2m reset.
> > ---
> >   xen/arch/x86/hvm/ioreq.c  |  8 ++++++++  xen/arch/x86/mm/hap/hap.c |  9
> > +++++++++  xen/arch/x86/mm/p2m-ept.c |  8 +++++++-
> > xen/arch/x86/mm/p2m-pt.c  | 13 +++++++++++--
> >   xen/arch/x86/mm/p2m.c     | 20 ++++++++++++++++++++
> >   xen/include/asm-x86/p2m.h |  9 ++++++++-
> >   6 files changed, 63 insertions(+), 4 deletions(-)
> >
> > diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c index
> > 746799f..102c6c2 100644
> > --- a/xen/arch/x86/hvm/ioreq.c
> > +++ b/xen/arch/x86/hvm/ioreq.c
> > @@ -949,6 +949,14 @@ int hvm_map_mem_type_to_ioreq_server(struct
> > domain *d, ioservid_t id,
> >
> >       spin_unlock_recursive(&d->arch.hvm_domain.ioreq_server.lock);
> >
> > +    if ( rc == 0 && flags == 0 )
> > +    {
> > +        struct p2m_domain *p2m = p2m_get_hostp2m(d);
> > +
> > +        if ( read_atomic(&p2m->ioreq.entry_count) )
> > +            p2m_change_entry_type_global(d, p2m_ioreq_server, p2m_ram_rw);
> > +    }
> > +
> >       return rc;
> >   }
> >
> > diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
> > index a57b385..6ec950a 100644
> > --- a/xen/arch/x86/mm/hap/hap.c
> > +++ b/xen/arch/x86/mm/hap/hap.c
> > @@ -187,6 +187,15 @@ out:
> >    */
> >   static int hap_enable_log_dirty(struct domain *d, bool_t log_global)  {
> > +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
> > +
> > +    /*
> > +     * Refuse to turn on global log-dirty mode if
> > +     * there's outstanding p2m_ioreq_server pages.
> > +     */
> > +    if ( log_global && read_atomic(&p2m->ioreq.entry_count) )
> > +        return -EBUSY;
> I know this has been discussed before, but didn't remember
> the detail reason - why cannot allow log-dirty mode when
> there are still outstanding p2m_ioreq_server pages? Cannot
> we mark related page as dirty when forwarding write emulation
> request to corresponding ioreq server?
IIUC, changing a paging to p2m_log_dirty will only mark it as read-only 
once,
and after it is marked as dirty, it will be changed back to p2m_ram_rw. 
Also,
handling of a p2m_log_dirty page is quite different, we'd like the 
p2m_ioreq_server
ones be handled like p2m_mmio_dm.

Thanks
Yu

[snip]

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