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

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 12/4/18 2:54 PM, Jan Beulich wrote:
>>>> On 04.12.18 at 13:18, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>> Right, so you're saying that the series would be able to go in provided
>> that the situation is not made worse than it currently is.
> 
> Well, I'm not the maintainer of this code, so in principle the
> series can go in despite my reservations.
> 
>> As far as I can tell, the patch changes nothing functionally from the
>> current state of affairs: when called for the hostp2m it behaves exactly
>> the same.
>>
>> The one difference is the early return, which is certainly not making
>> things worse: rangeset_add_range() will crash the hypervisor in an
>> ASSERT(start <= end). It just happened that that never occured for the
>> hostp2m - so it will continue to not happen for the hostp2m.
>>
>> Could you please provide more details on the case that is making things
>> worse? Which cases of "start" (if that is what you mean) make things worse?
> 
> Okay, I've looked again - start indeed doesn't get clipped. Yet the
> added return path plus the comment next to it still make the
> situation worse imo, as they further the impression that the clipping
> of end is correct. Or in other words, with the change it'll be less
> visible that there's a (perhaps just theoretical, as said) bug here.
> Hence my desire to get the bug addressed while this code is
> being basically re-done anyway.

Understood.

The original Xen code seems to be in patch
437f54d3a33d3787a7cc485eb2b3451e8be49ca7 ("x86/EPT: don't walk page
tables when changing types on a range") which does clip always clip
"end" as pointed out previously, and
90ac32559bfbd08127638ba13f99b5ed565cfc2b ("x86/EPT: don't walk entire
page tables when globally changing types"), which adds code specifically
for logdirty_ranges.

The always-on clipping is done in the first of the two commits (the
previous code just did "for ( gfn = start; gfn < end; ) [...]
p2m_set_entry(p2m, gfn, mfn, order, nt, a);"). The only reference to
max_mapped_pfn is in p2m_change_type_range(). The change appears to be
related to the introduction of ept_change_entry_type_range(). I am not,
unfortunately, familiar with that code and can't tell for sure yet
whether the optimization is also a potential bug. It certainly doesn't
look like clipping end is necessary.

In the interest of moving forward, and since this patch is called
"Always use hostp2m when clipping rangesets" and doesn't appear to make
things worse, I suggest that I clean up the series and send out V11
(with the newly added Tested-by from Tamas), and - also depending on
input from George - we can revisit the max_mapped_pfn clipping
discussion after we at least end up fixing the stuck display with altp2m
that basically prevents Tamas and us from using altp2m.


Thanks,
Razvan

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