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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 05/04/17 18:18, Yu Zhang wrote:
> 
> 
> On 4/6/2017 1:01 AM, George Dunlap wrote:
>> On 05/04/17 17:32, Yu Zhang wrote:
>>>
>>> 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@xxxxxxxxxxxxxxx>
>>>>>> 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?
>> No, I'm talking about before the ioreq server detaches.
> Sorry, I do not get it. Take scenario 1 for example:
>> Scenario 1: Bug in driver
>> 1. Guest driver allocates page A
>> 2. dm marks A as p2m_ioreq_server
> Here in step 2. the ioreq.entry_count increases;
>> 3. Guest driver accidentally frees A to the kernel
>> 4. guest kernel balloons out page A; ioreq.entry_count is wrong
> 
> Here in step 4. the ioreq.entry_count decreases.
> Isn't this what we are expecting?

So step 4 happens via xen/common/memory.c:decrease_reservation().  Where
along that path will ioreq.entry_count be decreased for the page freed?

Maybe it is, but I didn't see it when I looked.  As far as I can tell,
after step 4, ioreq.entry_count would still be 1, but that the actual
number of p2m_ioreq_server entries would be 0.

 -George


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