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

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 12/5/18 6:30 PM, Jan Beulich wrote:
>>>> On 05.12.18 at 10:18, <rcojocaru@bitdefender.com> wrote:
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -1002,30 +1002,43 @@ int p2m_change_type_one(struct domain *d, unsigned long gfn_l,
>>      return rc;
>>  }
>>  
>> -/* Modify the p2m type of a range of gfns from ot to nt. */
>> +/* Modify the p2m type of [start, end) from ot to nt. */
>>  static void change_type_range(struct p2m_domain *p2m,
>>                                unsigned long start, unsigned long end,
>>                                p2m_type_t ot, p2m_type_t nt)
>>  {
>> -    unsigned long gfn = start;
>>      struct domain *d = p2m->domain;
>> +    const unsigned long host_max_pfn = p2m_get_hostp2m(d)->max_mapped_pfn;
>>      int rc = 0;
>>  
>> -    if ( unlikely(end > p2m->max_mapped_pfn) )
>> -    {
>> -        if ( !gfn )
>> -        {
>> -            p2m->change_entry_type_global(p2m, ot, nt);
>> -            gfn = end;
>> -        }
>> -        end = p2m->max_mapped_pfn + 1;
>> -    }
>> -    if ( gfn < end )
>> -        rc = p2m->change_entry_type_range(p2m, ot, nt, gfn, end - 1);
>> +    --end;
>> +
>> +    if ( start >= host_max_pfn )
>> +        printk(XENLOG_G_WARNING "Dom%d logdirty rangeset clipped to max_mapped_pfn\n",
>> +               d->domain_id);
>> +
>> +    /* Always clip the rangeset down to the host p2m. */
>> +    if ( unlikely(end > host_max_pfn) )
>> +        end = host_max_pfn;
>> +
>> +    /* If the requested range is out of scope, return doing nothing. */
>> +    if ( start > end )
>> +        return;
> 
> My prior comment remains: Even if there's no change in behavior
> (and you avoid the assertion), especially due to the comment the
> impression results (at least to me) that all is well here, when it
> really is a (latent) bug to "do nothing" in this case. George, so far
> this was a discussion between Razvan and me - do you have an
> opinion either way here?

Obviously I can't speak for George, but to reiterate my previous
analysis, it looks like this patch has added the clipping:

http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=437f54d3a33d3787a7cc485eb2b3451e8be49ca7

and this patch has added the global_logdirty ranges code:

http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=90ac32559bfbd08127638ba13f99b5ed565cfc2b

but left the clipping code in (possibly accidentally). You may have some
insight into that, being their author, although it's been a few years
since then.

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?


Thanks,
Razvan

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