[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v10 5/6] x86/ioreq server: Asynchronously reset outstanding p2m_ioreq_server entries.
On 4/6/2017 1:18 AM, 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:In theory, yes, the guest *shouldn't* do this. But what if the guest OSOn 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 recordthe number of p2m_ioreq_server p2m page table entries. One nature of these entries is that they only point to 4K sized page frames, becauseall p2m_ioreq_server entries are originated from p2m_ram_rw ones inp2m_change_type_one(). We do not need to worry about the counting for2M/1G sized pages.Assuming that all p2m_ioreq_server entries are *created* byp2m_change_type_one() may valid, but can you assume that they are onlyever *removed* by p2m_change_type_one() (or recalculation)? What happens, for instance, if a guest balloons out one of the rampages? I don't immediately see anything preventing a p2m_ioreq_serverpage from being ballooned out, nor anything on thedecrease_reservation() path decreasing p2m->ioreq.entry_count. Or didI 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.makes a mistake? Or, what if the ioreq server makes a mistake andplaces a watch on a page that *isn't* allocated by the device driver, orforgets 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_rwbefore being set to p2m_invalid, just like the normal path. Will this bea 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_serverHere 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 wrongHere in step 4. the ioreq.entry_count decreases. Oh. I figured out. This entry is not invalidated yet if ioreq is not unmapped. Sorry. Isn't this what we are expecting? YuScenario 2: Bug in the kernel 1. Guest driver allocates page A 2. dm marks A as p2m_ioreq_server 3. Guest kernel tries to balloon out page B, but makes a calculation mistake and balloons out A instead; now ioreq.entry_count is wrong Scenario 3: Off-by-one bug in devicemodel 1. Guest driver allocates pages A-D 2. dm makes a mistake and marks pages A-E as p2m_ioreq_server (one extra page) 3. guest kernel balloons out page E; now ioreq.entry_count is wrong Scenario 4: "Leak" in devicemodel 1. Guest driver allocates page A 2. dm marks A as p2m_ioreq_server 3. Guest driver is done with page A, but DM forgets to reset it to p2m_ram_rw 4. Guest driver frees A to guest kernel 5. Guest kernel balloons out page A; now ioreq.entry_count is wrong I could keep going on; there are *lots* of bugs in the driver, the kernel, or the devicemodel which could cause pages marked p2m_ioreq_server to end up being ballooned out; which under the current code would make ioreq.entry_count wrong. It's the hypervisor's job to do the right thing even when other components have bugs in them. This is why I initially suggested keeping count in atomic_write_ept_entry() -- no matter how the entry is changed,we always know exactly how many entries of type p2m_ioreq_server we have. Well, count in atomic_write_ept_entry() only works for ept. Besides, it requires interface changes - need to pass the p2m.Another thought is - now in XenGT, PoD is disabled to make sure gfn->mfn does not change. So how about we disable ballooning if ioreq.entry_count is not 0? Yu -George_______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |