Re: [Xen-devel] [PATCH V11 4/5] p2m: Always use hostp2m when clipping rangesets

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 12/20/18 1:52 PM, Jan Beulich wrote:
>>>> On 20.12.18 at 13:59, <george.dunlap@xxxxxxxxxx> wrote:
>> On 12/20/18 8:20 AM, Jan Beulich wrote:
>>>>>> On 19.12.18 at 18:26, <george.dunlap@xxxxxxxxxx> wrote:
>>>> On 12/13/18 10:43 AM, Jan Beulich wrote:
>>>>>>>> On 13.12.18 at 11:22, <rcojocaru@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel