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

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 12/3/18 10:49 AM, Jan Beulich wrote:
>>>> On 30.11.18 at 22:59, <rcojocaru@bitdefender.com> 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