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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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(). It may well be that there are more bugs
here (like ept_set_entry() not honoring this, but then again
this is perhaps something the callers should already take care
of), but that's the behavior I'd expect, and why I think the
range should not be clipped for the purpose of insertion into
the rangeset.

> But of course that's no different than what would happen if
> max_mapped_pfn were 0x2000, but gfns 0x1000-11ff just happened to be empty.
> 
> Under normal circumstances, neither of these situations should happen;
> and in neither case will catastrophic consequences happen (unless you
> were relying on hap_track_dirty_vram for something other than tracking
> dirty vram).
> 
> I'm inclined to say that ideally, change_type_range should pass an error
> up if end > max_mapped_pfn.
> 
> But of course, it doesn't return an error at the moment, so that's out
> of scope for this series.
> 
> I take it, Jan, that in the absence of changing the behavior, you'd like
> the comment to look something like this?
> 
> "Always clip the rangeset down to the host p2m.  NB that this means the
> logdirty_range will also be clipped, so in the future gfns in
> (host_max_pfn, end) range won't be affected by change_entry_type_global.
>  We should probably return an error in this case instead, as it's almost
> certainly a mistake; but that's left as a clean-up for another time."

Well, not exactly. IMO at least p2m_change_type_range(...,
0, ULONG_MAX) should match p2m_change_entry_type_global(),
with the exception of the rangeset modification (which in the
p2m_change_entry_type_global() global case is replaced by
modifying p2m->global_logdirty).

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel