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

[Yu Zhang <yu.c.zhang@linux.intel.com>]

On 3/22/2017 4:10 PM, Tian, Kevin wrote:
>> From: Yu Zhang [mailto:yu.c.zhang@linux.intel.com]
>> 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@linux.intel.com>
>> ---
>> Cc: Paul Durrant <paul.durrant@citrix.com>
>> Cc: Jan Beulich <jbeulich@suse.com>
>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>> Cc: George Dunlap <george.dunlap@eu.citrix.com>
>> Cc: Jun Nakajima <jun.nakajima@intel.com>
>> Cc: Kevin Tian <kevin.tian@intel.com>
>>
>> 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@lists.xen.org
https://lists.xen.org/xen-devel