Re: [Xen-devel] [PATCH 2/2] x86/altp2m: fix display frozen when switching to a new view early

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 18/10/18 11:07, Razvan Cojocaru wrote:
> When an new altp2m view is created very early on guest boot, the
> display will freeze (although the guest will run normally). This
> may also happen on resizing the display. The reason is the way
> Xen currently (mis)handles logdirty VGA: it intentionally
> misconfigures VGA pages so that they will fault.
>
> The problem is that it only does this in the host p2m. Once we
> switch to a new altp2m, the misconfigured entries will no longer
> fault, so the display will not be updated.
>
> This patch:
> * updates ept_handle_misconfig() to use the active altp2m instead
>   of the hostp2m;
> * allocates new logdirty ranges for each altp2m;
> * has p2m_init_altp2m_ept() copy over max_mapped_pfn,
>   and global_logdirty, and merges the logdirty ranges of the
>   hostp2m into the logdirty range of the altp2m;
> * modifies p2m_change_entry_type_global(), p2m_memory_type_changed
>   and p2m_change_type_range() to propagate their changes to all
>   valid altp2ms.
>
> Signed-off-by: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
> Suggested-by: George Dunlap <george.dunlap@xxxxxxxxxx>

From the looks of this patch, it would be cleaner to split out the patch
for allocating/freeing resources from the patch which implements the
logdirty merging.

On the allocating/freeing side of things specifically, ...

> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
> index fabcd06..28790bf 100644
> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -1434,18 +1437,44 @@ void setup_ept_dump(void)
>      register_keyhandler('D', ept_dump_p2m_table, "dump VT-x EPT tables", 0);
>  }
>  
> -void p2m_init_altp2m_ept(struct domain *d, unsigned int i)
> +int p2m_init_altp2m_ept(struct domain *d, unsigned int i)
>  {
>      struct p2m_domain *p2m = d->arch.altp2m_p2m[i];
>      struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
>      struct ept_data *ept;
> +    int rc;
> +
> +    ASSERT(!p2m->sync.logdirty_ranges);
> +    p2m->sync.logdirty_ranges = rangeset_new(d, "log-dirty",
> +                                             RANGESETF_prettyprint_hex);
> +    if ( !p2m->sync.logdirty_ranges )
> +        return -ENOMEM;
> +
> +    rc = rangeset_merge(p2m->sync.logdirty_ranges,
> +                        hostp2m->sync.logdirty_ranges);
> +    if ( !rc )
> +        return rc;
>  
>      p2m->ept.ad = hostp2m->ept.ad;
> +    p2m->max_mapped_pfn = hostp2m->max_mapped_pfn;
> +    p2m->default_access = hostp2m->default_access;
> +    p2m->domain = hostp2m->domain;
> +
> +    p2m->sync.global_logdirty = hostp2m->sync.global_logdirty;
>      p2m->min_remapped_gfn = gfn_x(INVALID_GFN);
>      p2m->max_remapped_gfn = 0;
>      ept = &p2m->ept;
>      ept->mfn = pagetable_get_pfn(p2m_get_pagetable(p2m));
>      d->arch.altp2m_eptp[i] = ept->eptp;
> +
> +    return 0;
> +}
> +
> +void p2m_uninit_altp2m_ept(struct p2m_domain *p2m)

For naming consistency, this should be p2m_destroy_altp2m_ept()

[EDIT] It looks like the rest of the code has poor consistency.  /sigh

> +{
> +    ASSERT(p2m->sync.logdirty_ranges);
> +    rangeset_destroy(p2m->sync.logdirty_ranges);
> +    p2m->sync.logdirty_ranges = NULL;

Please make all destroy functions idempotent.  i.e.

if ( p2m->sync.logdirty_ranges )
{
    rangeset_destroy(p2m->sync.logdirty_ranges);
    p2m->sync.logdirty_ranges = NULL;
}

and use this destroy function in the cleanup path of init().

I'm currently in the process of doing this to our entire create/destroy
infrastructure, which is a necessary prerequisite to fix the "vcpu
allocation during domain creation" problem we've got.

The longterm plan is to have errors in the init() path return
immediately with no cleanup, and have the top level make one call into
the destroy() path.  For this to work, the destroy path needs to be able
to correctly clean up from any point of initialisation (including none).

>  }
>  
>  unsigned int p2m_find_altp2m_by_eptp(struct domain *d, uint64_t eptp)
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 42b9ef4..e9f8385 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -119,9 +120,9 @@ static int p2m_init_hostp2m(struct domain *d)
>  
>      if ( p2m )
>      {
> -        p2m->logdirty_ranges = rangeset_new(d, "log-dirty",
> -                                            RANGESETF_prettyprint_hex);
> -        if ( p2m->logdirty_ranges )
> +        p2m->sync.logdirty_ranges = rangeset_new(d, "log-dirty",
> +                                                 RANGESETF_prettyprint_hex);

This looks to be common with bits of p2m_init_altp2m_ept().  Why doesn't
that get reused?

> +        if ( p2m->sync.logdirty_ranges )
>          {
>              d->arch.p2m = p2m;
>              return 0;
> @@ -2341,6 +2396,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_uninit_altp2m_ept(d->arch.altp2m_p2m[idx]);

Shouldn't this ideally be called from
ept_p2m_uninit(d->arch.altp2m_p2m[idx]) instead?

~Andrew

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