Re: [Xen-devel] [PATCH V6 3/4] x86/mm: allocate logdirty_ranges for altp2ms

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 11/15/18 5:52 PM, George Dunlap wrote:
> 
> 
>> On Nov 14, 2018, at 8:40 PM, Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx> 
>> wrote:
>>
>> This patch is a pre-requisite for the one fixing VGA logdirty
>> freezes when using altp2m. It only concerns itself with the
>> ranges allocation / deallocation / initialization part. While
>> touching the code, I've switched global_logdirty from bool_t
>> to bool.
>>
>> P2m_reset_altp2m() has been refactored to reduce code
>> repetition, and it now takes the p2m lock. Similar
>> refactoring has been done with p2m_activate_altp2m().
> 
> Thanks!  I think this is almost there.  I have a couple of comments about the 
> code below; so since we have to do another round, let me comment on the 
> changelog.
> 
> It doesn’t make much sense to say you’re “refactoring” a function that you 
> are just now introducing in this patch.

Right, the term doesn't apply to p2m_activate_altp2m() - I got carried
away with p2m_reset_altp2m(). :)

> I think I’d say something  more like this:
> 
> ---
> For now, only do allocation/deallocation; keeping them in sync will be done 
> in subsequent patches.
> 
> Logdirty synchronization will only be done for active altp2ms; so allocate 
> logdirty rangesets (copying the host logdirty rangeset) when an altp2m is 
> activated, and free it when deactivated.
> 
> Write a helper function to do altp2m activiation (appropriately handling 
> failures).  Also, refactor p2m_reset_altp2m() so that it can be used to 
> remove redundant codepaths, fixing the locking while we’re at it.
> 
> While we’re here, switch global_logdirty from bool_t to bool.
> ---

Thanks, I'll use the new description.

>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
>> index 418ff85..abdf443 100644
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -2282,6 +2282,34 @@ bool_t p2m_altp2m_lazy_copy(struct vcpu *v, paddr_t 
>> gpa,
>>     return 1;
>> }
>>
>> +static void p2m_reset_altp2m(struct domain *d, unsigned int idx,
>> +                             bool reset_remapped, bool free_logdirty_ranges)
> 
> As Jan says, passing in (…true, false) makes it more difficult to follow the 
> code and see what’s going on.  You could use a ‘flags’ structure, as he 
> mentions; or alternately, you could pass in an argument that was either 
> DEACTIVATE or RESET, and then use that to decide which things to do.

Will do.

>> +{
>> +    struct p2m_domain *p2m;
>> +
>> +    ASSERT(idx < MAX_ALTP2M);
>> +    p2m = d->arch.altp2m_p2m[idx];
>> +
>> +    p2m_lock(p2m);
>> +
>> +    p2m_flush_table_locked(p2m);
>> +    /* Uninit and reinit ept to force TLB shootdown */
>> +
>> +    if ( free_logdirty_ranges )
>> +        p2m_free_logdirty(p2m);
>> +
>> +    ept_p2m_uninit(p2m);
>> +    ept_p2m_init(p2m);
> 
> Nit: The comment about uninit/reinit should be just before the uninit/reinit. 
> :-)

Will move it.

>> +
>> +    if ( reset_remapped )
>> +    {
>> +        p2m->min_remapped_gfn = gfn_x(INVALID_GFN);
>> +        p2m->max_remapped_gfn = 0;
>> +    }
> 
> I don’t think there’s a need to do this conditionally.  In fact, the only 
> reason it’s correct *not* to do this for the other two cases is because in 
> those cases the p2m will go through p2m_init_altp2m_ept() before being used 
> again.  Resetting these is redundant, but harmless, and not worth an extra 
> function argument and conditional to avoid.

I'll remove the if.

>> +static int p2m_init_altp2m_logdirty(struct p2m_domain *p2m)
>> +{
>> +    struct p2m_domain *hostp2m = p2m_get_hostp2m(p2m->domain);
>> +    int rc = p2m_init_logdirty(p2m);
>> +
>> +    if ( rc )
>> +        return rc;
>> +
>> +    /* The following is really just a rangeset copy. */
>> +    return rangeset_merge(p2m->logdirty_ranges, hostp2m->logdirty_ranges);
>> +}
>> +
>> +static int p2m_activate_altp2m(struct domain *d, unsigned int idx)
>> +{
>> +    int rc;
>> +
>> +    ASSERT(idx < MAX_ALTP2M);
>> +    rc = p2m_init_altp2m_logdirty(d->arch.altp2m_p2m[idx]);
>> +
>> +    if ( rc )
>> +        return rc;
>> +
>> +    p2m_init_altp2m_ept(d, idx);
> 
> Is there any reason to make these separate functions?  I had in mind a single 
> p2m_activate_altp2m() function which would do all three things 
> (p2m_init_logdirty, rangeset_merge, and init_altp2m_ept).

Nope, no reason, in fact I did think about just that but wasn't sure it
wasn't deviating from what I thought was requested. I'll make that code
a single function.

> Also, I realize it’s _probably_ not necessary to grab the lock here for the 
> p2m in question (since it shouldn’t be in use, because altp2m_eptp[] is 
> INVALID_MFN); but since it’s not on the hot path, it seems like we might as 
> well grab it just to be safe.
> 
> Everything else looks good, thanks.

Thanks for the review!


Razvan

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