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

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 11/29/18 3:58 PM, Jan Beulich wrote:
>>>> On 29.11.18 at 14:23, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>> On 11/29/18 12:04 PM, Jan Beulich wrote:
>>>>>> On 28.11.18 at 22:56, <rcojocaru@xxxxxxxxxxxxxxx> 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",
 987                rc, 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel