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/6/2017 12:35 AM, George Dunlap wrote:
> On 05/04/17 17:22, Yu Zhang wrote:
>>
>> 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.
> In theory, yes, the guest *shouldn't* do this.  But what if the guest OS
> makes a mistake?  Or, what if the ioreq server makes a mistake and
> places a watch on a page that *isn't* allocated by the device driver, or
> forgets to change a page type back to ram when the device driver frees
> it back to the guest kernel?

Then the lazy p2m change code will be triggered, and this page is reset 
to p2m_ram_rw
before being set to p2m_invalid, just like the normal path. Will this be 
a problem?

> It's the hypervisor's job to do the right thing even if the guest and
> the device model don't.
>
>> 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.
> Yes, once the lazy type change has been made, we can rely on the
> recalculation logic to make sure that the types are changed appropriately.

Yep. So my understanding is that as long as p2m_set_entry() is used to 
change the p2m,
we do not need to worry about this.

B.R.
Yu
>
>   -George
>
>


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