On 12/20/18 1:52 PM, Jan Beulich wrote: >>>> On 20.12.18 at 13:59, <george.dun...@citrix.com> wrote: >> On 12/20/18 8:20 AM, Jan Beulich wrote: >>>>>> On 19.12.18 at 18:26, <george.dun...@citrix.com> wrote: >>>> On 12/13/18 10:43 AM, Jan Beulich wrote: >>>>>>>> On 13.12.18 at 11:22, <rcojoc...@bitdefender.com> wrote: >>>>>> For my own part, I see no reason why not clipping end should not work >>>>>> when updating the ranges only (as long as start continues to be <= >>>>>> unclipped_end). >>>>>> >>>>>> Would that modification + testing of it help this series continue? >>>>> >>>>> I think so, at least as far as I'm concerned. But I think we really need >>>>> George's opinion as well. >>>> >>>> We are going off into the weeds a little bit here I think. >>>> >>>> If I understand Jan's concern properly, he's concerned about a situation >>>> like this: >>>> >>>> [start] p2m->max_mapped_pfn == 0xfff >>>> 1. change_type_range ram => logdirty, [0x900, 0x1200) >>>> >>>> Obviously the actual p2m entries can only be changed from 0x900 to >>>> 0xfff; but what about the logdirty ranges? At the moment, the result >>>> will be a rangeset with [0x900, 0xfff]. >>>> >>>> Jan is asking whether the rangeset should instead be [0x900, 0x11ff]. >>>> >>>> So the time when it would matter would be a situation like the following: >>>> >>>> 2. p2m_set_entry(0x1100, M) >>>> >>>> 3. change_entry_type_global(ram => logdirty) >>>> >>>> 4. change_entry_type_global(logdirty => ram) >>>> >>>> Under the current regime gfn 0x1100 would be have type ram_rw both after >>>> step 2, and after step 4. >>>> >>>> If we used Jan's suggestion, then it would be marked as ram_rw after >>>> step 2, and logdirty after step 4. >>> >>> Afaict it would be marked logdirty also after step 2, at least >>> effectively (to the outside world), due to ept_get_entry()'s call >>> to p2m_recalc_type(). >> >> That's not what I'm seeing. Let's consider the ept entry for gfn 0x1100 >> at/after the various stages: >> >> [start]: empty (valid bit clear) >> 1. change_type_range doesn't touch this, so still empty. >> 2. ept_set_entry(M) >> - Calls recalc_type(). This will walk the ept table down to the >> particular ept entry, resolving the `recalc` bit at each level. >> - Finally it will set the entry to point to M, with the recalc bit >> clear, and the entry *not* misconfigured. > > Well of course - if the caller specified p2m_ram_rw. But this is > wrong for the caller to do for any page inside the logdirty > range. ept_set_entry() is a function which is not itself > implementing policy; it depends on higher levels getting things > right together with the page tables being in proper state.
But nobody is doing any checking right now. Under what circumstances *would* p2m_set_entry() with p2m_ram_logdirty be called? On the other hand, p2m_set_entry() with p2m_ram_rw could easily happen if the guest calls decrease_reservation and then populate_physmap on a page in the logdirty range. I also disagree about the layering argument. Everything else about managing logdirty setup in the p2m tables is handled by p2m-[e]pt.c; swizzling p2m_ram_rw into p2m_ram_logdirty in [e]pt_set_entry() would be the most obvious and consistent thing to do. > IOW what you describe might match the current > situation, but I'm unconvinced it's intended/wanted behavior. Well no, I wouldn't say it's wanted behavior. The point was to look at the actual current behavior, and see if there was a simple change we could make to make things consistent. It turns out, there's not. At the moment I'm only working 4-ish more days between now and the code freeze, and we're arguing over whether the comment should say, "We should probably do X instead" or "We should probably do Y instead." Can we just for now take the text as I proposed it? You can argue about the right thing to do when we do the alleged clean-up. -George _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
