Re: [PATCH V5 2/3] x86/mm: allocate logdirty_ranges for altp2ms

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 11/13/18 7:57 PM, George Dunlap wrote:
> On 11/11/18 2:07 PM, Razvan Cojocaru 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.
>>
>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
> 
> I've convinced myself that this patch is probably correct now, and as a
> result I've had a chance to look a bit at the resulting code.  Which
> means, unfortunately, that I'm going to be a bit annoying and ask more
> questions that I didn't ask last time.

Thanks for the review, and please ask away. :)

>> ---
>> CC: George Dunlap <george.dunlap@eu.citrix.com>
>> CC: Jan Beulich <jbeulich@suse.com>
>> CC: Andrew Cooper <andrew.cooper3@citrix.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>>
>> ---
>> Changes since V4:
>>  - Always call p2m_free_logdirty() in p2m_free_one() (previously
>>    the call was gated on hap_enabled(p2m->domain) && cpu_has_vmx).
>> ---
>>  xen/arch/x86/mm/p2m.c     | 74 ++++++++++++++++++++++++++++++++++++-----------
>>  xen/include/asm-x86/p2m.h |  2 +-
>>  2 files changed, 58 insertions(+), 18 deletions(-)
>>
>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
>> index 42b9ef4..69536c1 100644
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -59,6 +59,28 @@ static void p2m_nestedp2m_init(struct p2m_domain *p2m)
>>  #endif
>>  }
>>  
>> +static int p2m_init_logdirty(struct p2m_domain *p2m)
>> +{
>> +    if ( p2m->logdirty_ranges )
>> +        return 0;
>> +
>> +    p2m->logdirty_ranges = rangeset_new(p2m->domain, "log-dirty",
>> +                                        RANGESETF_prettyprint_hex);
>> +    if ( !p2m->logdirty_ranges )
>> +        return -ENOMEM;
>> +
>> +    return 0;
>> +}
>> +
>> +static void p2m_free_logdirty(struct p2m_domain *p2m)
>> +{
>> +    if ( !p2m->logdirty_ranges )
>> +        return;
>> +
>> +    rangeset_destroy(p2m->logdirty_ranges);
>> +    p2m->logdirty_ranges = NULL;
>> +}
>> +
>>  /* Init the datastructures for later use by the p2m code */
>>  static int p2m_initialise(struct domain *d, struct p2m_domain *p2m)
>>  {
>> @@ -107,6 +129,7 @@ free_p2m:
>>  
>>  static void p2m_free_one(struct p2m_domain *p2m)
>>  {
>> +    p2m_free_logdirty(p2m);
>>      if ( hap_enabled(p2m->domain) && cpu_has_vmx )
>>          ept_p2m_uninit(p2m);
>>      free_cpumask_var(p2m->dirty_cpumask);
>> @@ -116,19 +139,19 @@ static void p2m_free_one(struct p2m_domain *p2m)
>>  static int p2m_init_hostp2m(struct domain *d)
>>  {
>>      struct p2m_domain *p2m = p2m_init_one(d);
>> +    int rc;
>>  
>> -    if ( p2m )
>> -    {
>> -        p2m->logdirty_ranges = rangeset_new(d, "log-dirty",
>> -                                            RANGESETF_prettyprint_hex);
>> -        if ( p2m->logdirty_ranges )
>> -        {
>> -            d->arch.p2m = p2m;
>> -            return 0;
>> -        }
>> +    if ( !p2m )
>> +        return -ENOMEM;
>> +
>> +    rc = p2m_init_logdirty(p2m);
>> +
>> +    if ( !rc )
>> +        d->arch.p2m = p2m;
>> +    else
>>          p2m_free_one(p2m);
>> -    }
>> -    return -ENOMEM;
>> +
>> +    return rc;
>>  }
>>  
>>  static void p2m_teardown_hostp2m(struct domain *d)
>> @@ -138,7 +161,6 @@ static void p2m_teardown_hostp2m(struct domain *d)
>>  
>>      if ( p2m )
>>      {
>> -        rangeset_destroy(p2m->logdirty_ranges);
>>          p2m_free_one(p2m);
>>          d->arch.p2m = NULL;
>>      }
>> @@ -2279,6 +2301,18 @@ void p2m_flush_altp2m(struct domain *d)
>>      altp2m_list_unlock(d);
>>  }
> 
> I think everything above here could usefully be in its own patch; it
> would make it easier to verify that there were no functional changes in
> the refactoring.

Right, I'll split this patch then.

>> +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);
>> +}
>> +
>>  int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx)
>>  {
>>      int rc = -EINVAL;
>> @@ -2290,8 +2324,9 @@ int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx)
>>  
>>      if ( d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
>>      {
>> -        p2m_init_altp2m_ept(d, idx);
>> -        rc = 0;
>> +        rc = p2m_init_altp2m_logdirty(d->arch.altp2m_p2m[idx]);
>> +        if ( !rc )
>> +            p2m_init_altp2m_ept(d, idx);
>>      }
>>  
>>      altp2m_list_unlock(d);
>> @@ -2310,9 +2345,13 @@ int p2m_init_next_altp2m(struct domain *d, uint16_t *idx)
>>          if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
>>              continue;
>>  
>> -        p2m_init_altp2m_ept(d, i);
>> -        *idx = i;
>> -        rc = 0;
>> +        rc = p2m_init_altp2m_logdirty(d->arch.altp2m_p2m[i]);
>> +
>> +        if ( !rc )
>> +        {
>> +            p2m_init_altp2m_ept(d, i);
>> +            *idx = i;
>> +        }
> 
> It looks like there's a 1-1 correspondence between
> p2m_init_altp2m_logdirty() succeeding and calling p2m_inti_altp2m_ept().
>  Would it make sense to combine them into the same function, maybe named
> "p2m_activate_altp2m()" or something (to distinguish it from
> p2m_init_altp2m())?

Of course, that'll cut back on some redundancy. Always a good thing.

>> @@ -2341,6 +2380,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);
> 
> (In case I forget: Also, this is called without holding the appropriate
> p2m lock. )

Could you please provide more details? I have assumed that at the point
of calling a function called p2m_destroy_altp2m_by_id() it should be
safe to tear the altp2m down without further precaution.

I think you're saying that I should p2m_lock(d->arch.altp2m_p2m[idx])
just for the duration of the p2m_free_logdirty() call?

> I'm a bit suspicious of long strings of these sorts of functions in the
> middle of another function.  It turns out that there are three copies of
> this sequence of function calls (p2m_flush_table -> ept_p2m_uninit ->
> ept_p2m_init):
> 
> * Here (p2m_destroy_altp2m_id), when the user asks for the alt2m index
> to be destroyed
> 
> * In p2m_flush_altp2m(), which is called when altp2m is disabled for a
> domain
> 
> * In p2m_reset_altp2m(), which is called when an entry in the hostp2m is
> set to INVALID_MFN.
> 
> Presumably in p2m_reset_altp2m() we don't want to call
> p2m_free_logdirty(), as the altp2m is still active and we want to keep
> the logdirty ranges around.  But in p2m_flush_altp2m(), I'm pretty sure
> we do want to discard them: when altp2m is enabled again,
> p2m_init_logdirty() will return early, leaving the old rangesets in
> place; if the hostp2m rangesets have changed between the time altp2m was
> disabled and enabled again, the rangeset_merge() may have incorrect results.

I'll call p2m_free_logdirty() in p2m_flush_altp2m() as well.

> At the moment we essentially have two "init" states:
> * After domain creation; altp2m structures allocated, but no rangesets, & c
> * After being enabled for the first time: rangesets mirroring hostp2m,
> p2m_init_altp2m_ept() initialization done
> 
> Is there any particular reason we allocate the p2m structures on domain
> creation, but not logdirty range structures?  It seems like allocating
> altp2m structures on-demand, rather than at domain creation time, might
> make a lot of the reasoning here simpler.

I assume that this question is not addressed to me, since I'm not able
to answer it - I can only assume that having less heap used has been
preferred.


Thanks,
Razvan

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