Re: [Xen-devel] [PATCH V10 4/5] p2m: Always use hostp2m when clipping rangesets
On 12/4/18 2:54 PM, Jan Beulich wrote:
On 04.12.18 at 13:18, 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH V10 4/5] p2m: Always use hostp2m when clipping rangesets
>>> On 04.12.18 at 13:18, 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. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH V10 4/5] p2m: Always use hostp2m when clipping rangesets
On 12/3/18 10:49 AM, Jan Beulich wrote:
On 30.11.18 at 22:59, wrote:
>> On 11/29/18 3:58 PM, Jan Beulich wrote:
>>> Altp2m-s don't matter here at all. My point is that the present,
>>> unpatched p2m_change_type_range() updates the log-dirty
>>> ranges with the unclipped [start,end), but calls
>>> p2m->change_entry_type_range() with a possibly reduced
>>> range. Any subsequent caller of p2m_is_logdirty_range() may
>>> thus be mislead if the rangeset update now also used only the
>>> clipped range.
>>
>> I've been reading and re-reading the code and I'm still not sure I follow:
>>
>> 973 if ( unlikely(end > p2m->max_mapped_pfn) )
>> 974 {
>> 975 if ( !gfn )
>> 976 {
>> 977 p2m->change_entry_type_global(p2m, ot, nt);
>> 978 gfn = end;
>> 979 }
>> 980 end = p2m->max_mapped_pfn + 1;
>>
>> end is being clipped here ...
>>
>> 981 }
>> 982 if ( gfn < end )
>> 983 rc = p2m->change_entry_type_range(p2m, ot, nt, gfn, end - 1);
>>
>> ... and the if() above is not an else if(), so if ( unlikely(end >
>> p2m->max_mapped_pfn) ) we always clip end. What this new patch does in
>> that regard is just making sure it uses the hostp2m's max_mapped_pfn
>> instead of the altp2m's.
>
> Oh, good point. I was focussing too much on "start", the clipping
> of which is prevented by having the "gfn" local variable. And I
> think current code is wrong then too (and I further think your
> change then just extends badness to certain cases of "start"). So
> unless this can be explained as correct behavior, I'd hope for
> the situation to at least not be made worse than it is. Ideally it
> would be improved, but I realize the incentive may be low as it's
> presumably just a theoretical consideration.
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.
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?
Thanks,
Razvan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH V10 4/5] p2m: Always use hostp2m when clipping rangesets
>>> On 30.11.18 at 22:59, wrote:
> On 11/29/18 3:58 PM, Jan Beulich wrote:
>> Altp2m-s don't matter here at all. My point is that the present,
>> unpatched p2m_change_type_range() updates the log-dirty
>> ranges with the unclipped [start,end), but calls
>> p2m->change_entry_type_range() with a possibly reduced
>> range. Any subsequent caller of p2m_is_logdirty_range() may
>> thus be mislead if the rangeset update now also used only the
>> clipped range.
>
> I've been reading and re-reading the code and I'm still not sure I follow:
>
> 973 if ( unlikely(end > p2m->max_mapped_pfn) )
> 974 {
> 975 if ( !gfn )
> 976 {
> 977 p2m->change_entry_type_global(p2m, ot, nt);
> 978 gfn = end;
> 979 }
> 980 end = p2m->max_mapped_pfn + 1;
>
> end is being clipped here ...
>
> 981 }
> 982 if ( gfn < end )
> 983 rc = p2m->change_entry_type_range(p2m, ot, nt, gfn, end - 1);
>
> ... and the if() above is not an else if(), so if ( unlikely(end >
> p2m->max_mapped_pfn) ) we always clip end. What this new patch does in
> that regard is just making sure it uses the hostp2m's max_mapped_pfn
> instead of the altp2m's.
Oh, good point. I was focussing too much on "start", the clipping
of which is prevented by having the "gfn" local variable. And I
think current code is wrong then too (and I further think your
change then just extends badness to certain cases of "start"). So
unless this can be explained as correct behavior, I'd hope for
the situation to at least not be made worse than it is. Ideally it
would be improved, but I realize the incentive may be low as it's
presumably just a theoretical consideration.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH V10 4/5] p2m: Always use hostp2m when clipping rangesets
On 11/29/18 3:58 PM, Jan Beulich wrote:
On 29.11.18 at 14:23, wrote:
>> On 11/29/18 12:04 PM, Jan Beulich wrote:
>> On 28.11.18 at 22:56, wrote:
Changes since V9:
- Removed the patch RFC (replaced by a printk(XENLOG_G_WARNING).
- Reused start and end in change_type_range() and removed the
intermediary variables range_start and range_end.
- Added an extra explanation for the if ( start > end ) return;
code in the comment.
>>>
>>> This last item isn't really taking care of the comments I gave on v9.
>>> The _incoming_ start being larger than the _incoming_ end is
>>> something worth to point out. But you put that check after clipping
>>> end. Furthermore it looks like you continue to break the case
>>> where ->max_mapped_pfn increases subsequently, i.e. you still
>>> don't update the rangeset with the unmodified incoming values.
>>> Or otherwise I would have expected an explanation (as a reply
>>> to my comments, not necessarily by adding to description or
>>> comments of the patch here) why either this is not an issue or I'm
>>> misreading anything.
>>
>> max_mapped_pfn _should_ end up being >= the logdirty range upper bound,
>> since AFAICT the logdirty ranges are tied to ept_set_entry() calls,
>> which always end up calling p2m_altp2m_propagate_change() when they
>> occur on the hostp2m (which in turn calls p2m_set_entry() on the
>> altp2ms, and so on).
>
> Altp2m-s don't matter here at all. My point is that the present,
> unpatched p2m_change_type_range() updates the log-dirty
> ranges with the unclipped [start,end), but calls
> p2m->change_entry_type_range() with a possibly reduced
> range. Any subsequent caller of p2m_is_logdirty_range() may
> thus be mislead if the rangeset update now also used only the
> clipped range.
I've been reading and re-reading the code and I'm still not sure I follow:
973 if ( unlikely(end > p2m->max_mapped_pfn) )
974 {
975 if ( !gfn )
976 {
977 p2m->change_entry_type_global(p2m, ot, nt);
978 gfn = end;
979 }
980 end = p2m->max_mapped_pfn + 1;
end is being clipped here ...
981 }
982 if ( gfn < end )
983 rc = p2m->change_entry_type_range(p2m, ot, nt, gfn, end - 1);
... and the if() above is not an else if(), so if ( unlikely(end >
p2m->max_mapped_pfn) ) we always clip end. What this new patch does in
that regard is just making sure it uses the hostp2m's max_mapped_pfn
instead of the altp2m's.
984 if ( rc )
985 {
986 printk(XENLOG_G_ERR "Error %d changing Dom%d GFNs [%lx,%lx]
from %d to %d\n",
987rc, d->domain_id, start, end - 1, ot, nt);
988 domain_crash(d);
989 }
990
991 switch ( nt )
992 {
993 case p2m_ram_rw:
994 if ( ot == p2m_ram_logdirty )
995 rc = rangeset_remove_range(p2m->logdirty_ranges, start,
end - 1);
996 break;
997 case p2m_ram_logdirty:
998 if ( ot == p2m_ram_rw )
999 rc = rangeset_add_range(p2m->logdirty_ranges, start,
end - 1);
1000 break;
1001 default:
1002 break;
1003 }
Then above it calls rangeset_remove_range() or rangeset_add_range() with
the clipped end. rangeset_add_range() ASSERT()s that start <= end, so
we've established that if ( start > end ) return; is at least healthy
for that.
I could move the if ( start > end ) return; below the
p2m->change_entry_type_global(p2m, ot, nt); call so that the code uses
the same flow as it does now. But that would only matter for the case
when start == 0 and end < 0 (which is impossible, with end being an
unsigned long).
The current code already checks if ( gfn < end ) (where gfn is start in
the new patch) before calling p2m->change_entry_type_range() (again,
with the clipped end), so in that respect it's not different at all from
the current logic.
In light of all of that, I'm reading your comment to mean that you think
that the current logic is flawed because the actual work inside
p2m_change_type_range() is done on a clipped range - so you'd like to
either have the new patch refrain from clipping anything, or an
explanation as to why this is proper behaviour (and I was wrong to pay
special attention to the if() returning early you've mentioned in your
original review). Am I correct?
Thanks,
Razvan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH V10 4/5] p2m: Always use hostp2m when clipping rangesets
>>> On 29.11.18 at 14:23, wrote: > On 11/29/18 12:04 PM, Jan Beulich wrote: > On 28.11.18 at 22:56, wrote: >>> Changes since V9: >>> - Removed the patch RFC (replaced by a printk(XENLOG_G_WARNING). >>> - Reused start and end in change_type_range() and removed the >>>intermediary variables range_start and range_end. >>> - Added an extra explanation for the if ( start > end ) return; >>>code in the comment. >> >> This last item isn't really taking care of the comments I gave on v9. >> The _incoming_ start being larger than the _incoming_ end is >> something worth to point out. But you put that check after clipping >> end. Furthermore it looks like you continue to break the case >> where ->max_mapped_pfn increases subsequently, i.e. you still >> don't update the rangeset with the unmodified incoming values. >> Or otherwise I would have expected an explanation (as a reply >> to my comments, not necessarily by adding to description or >> comments of the patch here) why either this is not an issue or I'm >> misreading anything. > > max_mapped_pfn _should_ end up being >= the logdirty range upper bound, > since AFAICT the logdirty ranges are tied to ept_set_entry() calls, > which always end up calling p2m_altp2m_propagate_change() when they > occur on the hostp2m (which in turn calls p2m_set_entry() on the > altp2ms, and so on). Altp2m-s don't matter here at all. My point is that the present, unpatched p2m_change_type_range() updates the log-dirty ranges with the unclipped [start,end), but calls p2m->change_entry_type_range() with a possibly reduced range. Any subsequent caller of p2m_is_logdirty_range() may thus be mislead if the rangeset update now also used only the clipped range. > Also, apologies if I'm speaking out of turn and the question was really > addressed to George (who is the proper author of the patch). That's okay, since you've made the change in question. I did expect George to reply to my comments, but you volunteering to take care of addressing them is fine with me. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH V10 4/5] p2m: Always use hostp2m when clipping rangesets
>>> On 28.11.18 at 22:56, wrote: > Changes since V9: > - Removed the patch RFC (replaced by a printk(XENLOG_G_WARNING). > - Reused start and end in change_type_range() and removed the >intermediary variables range_start and range_end. > - Added an extra explanation for the if ( start > end ) return; >code in the comment. This last item isn't really taking care of the comments I gave on v9. The _incoming_ start being larger than the _incoming_ end is something worth to point out. But you put that check after clipping end. Furthermore it looks like you continue to break the case where ->max_mapped_pfn increases subsequently, i.e. you still don't update the rangeset with the unmodified incoming values. Or otherwise I would have expected an explanation (as a reply to my comments, not necessarily by adding to description or comments of the patch here) why either this is not an issue or I'm misreading anything. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
