Re: [PATCH V7 2/5] x86/mm: allocate logdirty_ranges for altp2ms

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 11/20/18 12:27 PM, Jan Beulich wrote:
>>>> On 20.11.18 at 11:02, <rcojocaru@bitdefender.com> wrote:
>> On 11/20/18 11:05 AM, Jan Beulich wrote:
>>>>>> On 19.11.18 at 18:26, <rcojocaru@bitdefender.com> wrote:
>>>> 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.
>>>
>>> Perhaps this should have been a separate patch again, such
>>> that e.g. ...
>>>
>>>> +static void p2m_reset_altp2m(struct domain *d, unsigned int idx,
>>>> +                             enum altp2m_reset_type reset_type)
>>>> +{
>>>> +    struct p2m_domain *p2m;
>>>> +
>>>> +    ASSERT(idx < MAX_ALTP2M);
>>>> +    p2m = d->arch.altp2m_p2m[idx];
>>>> +
>>>> +    p2m_lock(p2m);
>>>> +
>>>> +    p2m_flush_table_locked(p2m);
>>>> +
>>>> +    if ( reset_type == ALTP2M_DEACTIVATE )
>>>> +        p2m_free_logdirty(p2m);
>>>> +
>>>> +    /* Uninit and reinit ept to force TLB shootdown */
>>>> +    ept_p2m_uninit(p2m);
>>>> +    ept_p2m_init(p2m);
>>>> +
>>>> +    p2m->min_remapped_gfn = gfn_x(INVALID_GFN);
>>>> +    p2m->max_remapped_gfn = 0;
>>>
>>> ... the addition of these can be properly associated with either
>>> part of the change. Looking at the code you remove from e.g.
>>> p2m_flush_altp2m() it's not part of the refactoring, but of what
>>> this patch's actual purpose is. But this is guesswork of mine
>>> without the split and without the addition getting explained,
>>> not the least because this getting moved here from the original
>>> instance of the function might also mean that it's part of the
>>> refactoring, but would then need to be done only in the
>>> ALTP2M_RESET case.
>>
>> If you mean that p2m->min_remapped_gfn = gfn_x(INVALID_GFN); and
>> p2m->max_remapped_gfn = 0; should only happen on the ALTP2M_RESET case,
>> while that is technically true (and should follow from a verbatim
>> refactoring), George has pointed out that the assignments are in that
>> case unnecessary but harmless, and so the conditional is not worth it.
>>
>> I can add the if and treat the RESET case explicitly if that's required.
> 
> No, I'm specifically not requiring this. What I'm requiring is that the
> description match the changes. Which in turn would be easier if
> the refactoring was a separate patch.

I'll split this patch.


Thanks,
Razvan

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