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

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 10/30/18 6:22 PM, Jan Beulich wrote:
>>>> On 29.10.18 at 13:40, <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.
> 
> But while looking (briefly only for now) over patch 3 I couldn't
> see any sync-ing of the log-dirty ranges there either. Doesn't
> this need doing either there or here, if you go the copy route?

The syncing is done in patch 3, in p2m_change_entry_type_global(),
p2m_memory_type_changed(), p2m_change_type_range(), where now all active
p2ms (hostp2m + altp2ms) receive the same changes.

On allocating the logdirty_ranges for a new altp2m, the hostp2m (which
is sync with all others) is copied over, and then whatever logdirty
ranges the hypervisor is using for reading should be fine.

That was the thinking at least, am I missing something?

>> @@ -2271,6 +2297,7 @@ void p2m_flush_altp2m(struct domain *d)
>>      {
>>          p2m_flush_table(d->arch.altp2m_p2m[i]);
>>          /* Uninit and reinit ept to force TLB shootdown */
>> +        p2m_free_logdirty(d->arch.altp2m_p2m[i]);
>>          ept_p2m_uninit(d->arch.altp2m_p2m[i]);
>>          ept_p2m_init(d->arch.altp2m_p2m[i]);
>>          d->arch.altp2m_eptp[i] = mfn_x(INVALID_MFN);
>> @@ -2341,6 +2385,7 @@ int p2m_destroy_altp2m_by_id(struct domain *d, 
>> unsigned int idx)
>>          {
>>              p2m_flush_table(d->arch.altp2m_p2m[idx]);
>>              /* Uninit and reinit ept to force TLB shootdown */
>> +            p2m_free_logdirty(d->arch.altp2m_p2m[idx]);
>>              ept_p2m_uninit(d->arch.altp2m_p2m[idx]);
>>              ept_p2m_init(d->arch.altp2m_p2m[idx]);
>>              d->arch.altp2m_eptp[idx] = mfn_x(INVALID_MFN);
>> @@ -2471,6 +2516,7 @@ static void p2m_reset_altp2m(struct p2m_domain *p2m)
>>  {
>>      p2m_flush_table(p2m);
>>      /* Uninit and reinit ept to force TLB shootdown */
>> +    p2m_free_logdirty(p2m);
>>      ept_p2m_uninit(p2m);
>>      ept_p2m_init(p2m);
>>      p2m->min_remapped_gfn = gfn_x(INVALID_GFN);
> 
> For one these look all pretty similar, so I wonder why there's
> no helper function. But that's not something you need to change.
> Yet why are you freeing the log-dirty ranges here? These aren't
> full cleanup paths afaict.

After the first iteration Andrew has suggested that I should free in
ept_p2m_uninit(p2m):

https://lists.xenproject.org/archives/html/xen-devel/2018-10/msg01430.html

at which point I did so for V2, where you've suggested that that
function should be static and live in p2m.c. Which led to V3 where
basically I've plopped this function right before ept_p2m_uninit(p2m)
wherever it was called to match the V2 behaviour.


Thanks,
Razvan

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