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

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

On 4/5/2017 10:41 PM, George Dunlap wrote:
> On Sun, Apr 2, 2017 at 1:24 PM, Yu Zhang <yu.c.zhang@linux.intel.com> wrote:
>> 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.
>>
>> 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.
> Assuming that all p2m_ioreq_server entries are *created* by
> p2m_change_type_one() may valid, but can you assume that they are only
> ever *removed* by p2m_change_type_one() (or recalculation)?
>
> What happens, for instance, if a guest balloons out one of the ram
> pages?  I don't immediately see anything preventing a p2m_ioreq_server
> page from being ballooned out, nor anything on the
> decrease_reservation() path decreasing p2m->ioreq.entry_count.  Or did
> I miss something?
>
> Other than that, only one minor comment...

Thanks for your thorough consideration, George. But I do not think we need to worry about this:

If the emulation is in process, the balloon driver cannot get a p2m_ioreq_server page - because
it is already allocated.

And even when emulation is finished, the balloon driver successfully get this page, and triggers
decrease_reservation, the purpose is to remove the current mapping relation between the gfn
and mfn in p2m. So IIUC, p2m_remove_page() will be triggered if everything is goes fine, and then
p2m_set_entry(), which will trigger the recalc logic eventually, either in ept_set_entry() or
p2m_pt_set_entry(). Then the entry_count will be updated in the recalc logic.

>> 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.
> Grammar nit: if *there are* outstanding pages.

Oh, right. Thanks

B.R.
Yu
>   -George
>


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