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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

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@xxxxxxxxxxxxxxx>

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.

> 
> ---
> CC: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> CC: Jan Beulich <jbeulich@xxxxxxxx>
> CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> 
> ---
> 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.

> +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())?

> @@ -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. )

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.

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.

 -George

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