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

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Razvan Cojocaru [mailto:rcojocaru@xxxxxxxxxxxxxxx]
> Sent: Sunday, November 11, 2018 10:07 PM
> 
> 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;
> * 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>
> 
> ---
> CC: Jun Nakajima <jun.nakajima@xxxxxxxxx>
> CC: Kevin Tian <kevin.tian@xxxxxxxxx>
> 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:
>  - Now ASSERT()ing that altp2m is _not_ active in
>    p2m_pt_handle_deferred_changes(), with added comment.
>  - p2m_memory_type_changed() and p2m_change_type_range() now
>    process altp2ms with the hostp2m lock taken.
> ---
>  xen/arch/x86/mm/p2m-ept.c |   8 ++++
>  xen/arch/x86/mm/p2m-pt.c  |   8 ++++
>  xen/arch/x86/mm/p2m.c     | 115
> ++++++++++++++++++++++++++++++++++++++--------
>  xen/include/asm-x86/p2m.h |   6 +--
>  4 files changed, 114 insertions(+), 23 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
> index fabcd06..e6fa85f 100644
> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -657,6 +657,9 @@ bool_t ept_handle_misconfig(uint64_t gpa)
>      bool_t spurious;
>      int rc;
> 
> +    if ( altp2m_active(curr->domain) )
> +        p2m = p2m_get_altp2m(curr);
> +
>      p2m_lock(p2m);
> 
>      spurious = curr->arch.hvm.vmx.ept_spurious_misconfig;
> @@ -1440,6 +1443,11 @@ void p2m_init_altp2m_ept(struct domain *d,
> unsigned int i)
>      struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
>      struct ept_data *ept;
> 
> +    p2m->max_mapped_pfn = hostp2m->max_mapped_pfn;
> +    p2m->default_access = hostp2m->default_access;
> +    p2m->domain = hostp2m->domain;
> +
> +    p2m->global_logdirty = hostp2m->global_logdirty;
>      p2m->ept.ad = hostp2m->ept.ad;
>      p2m->min_remapped_gfn = gfn_x(INVALID_GFN);
>      p2m->max_remapped_gfn = 0;
> diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
> index 55df185..3828088 100644
> --- a/xen/arch/x86/mm/p2m-pt.c
> +++ b/xen/arch/x86/mm/p2m-pt.c
> @@ -29,6 +29,7 @@
>  #include <xen/event.h>
>  #include <xen/trace.h>
>  #include <public/vm_event.h>
> +#include <asm/altp2m.h>
>  #include <asm/domain.h>
>  #include <asm/page.h>
>  #include <asm/paging.h>
> @@ -464,6 +465,13 @@ int p2m_pt_handle_deferred_changes(uint64_t
> gpa)
>      struct p2m_domain *p2m = p2m_get_hostp2m(current->domain);
>      int rc;
> 
> +    /*
> +     * Should altp2m ever be enabled for NPT / shadow use, this code
> +     * should be updated to make use of the active altp2m, like
> +     * ept_handle_misconfig().
> +     */
> +    ASSERT(!altp2m_active(current->domain));
> +
>      p2m_lock(p2m);
>      rc = do_recalc(p2m, PFN_DOWN(gpa));
>      p2m_unlock(p2m);
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 69536c1..c8561ba 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -277,7 +277,6 @@ int p2m_init(struct domain *d)
>  int p2m_is_logdirty_range(struct p2m_domain *p2m, unsigned long start,
>                            unsigned long end)
>  {
> -    ASSERT(p2m_is_hostp2m(p2m));
>      if ( p2m->global_logdirty ||
>           rangeset_contains_range(p2m->logdirty_ranges, start, end) )
>          return 1;
> @@ -286,31 +285,79 @@ int p2m_is_logdirty_range(struct p2m_domain
> *p2m, unsigned long start,
>      return 0;
>  }
> 
> +static void change_entry_type_global(struct p2m_domain *p2m,
> +                                     p2m_type_t ot, p2m_type_t nt)
> +{
> +    p2m->change_entry_type_global(p2m, ot, nt);
> +    p2m->global_logdirty = (nt == p2m_ram_logdirty);
> +}
> +
>  void p2m_change_entry_type_global(struct domain *d,
>                                    p2m_type_t ot, p2m_type_t nt)
>  {
> -    struct p2m_domain *p2m = p2m_get_hostp2m(d);
> +    struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
> 
>      ASSERT(ot != nt);
>      ASSERT(p2m_is_changeable(ot) && p2m_is_changeable(nt));
> 
> -    p2m_lock(p2m);
> -    p2m->change_entry_type_global(p2m, ot, nt);
> -    p2m->global_logdirty = (nt == p2m_ram_logdirty);
> -    p2m_unlock(p2m);
> +    p2m_lock(hostp2m);
> +
> +    change_entry_type_global(hostp2m, ot, nt);
> +
> +#ifdef CONFIG_HVM
> +    if ( unlikely(altp2m_active(d)) )
> +    {
> +        unsigned int i;
> +
> +        for ( i = 0; i < MAX_ALTP2M; i++ )
> +            if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
> +            {
> +                struct p2m_domain *altp2m = d->arch.altp2m_p2m[i];
> +
> +                p2m_lock(altp2m);
> +                change_entry_type_global(altp2m, ot, nt);
> +                p2m_unlock(altp2m);
> +            }
> +    }
> +#endif
> +
> +    p2m_unlock(hostp2m);
> +}
> +
> +#ifdef CONFIG_HVM
> +/* There's already a memory_type_changed() in asm/mtrr.h. */
> +static void _memory_type_changed(struct p2m_domain *p2m)
> +{
> +    if ( p2m->memory_type_changed )
> +        p2m->memory_type_changed(p2m);
>  }
> 
>  void p2m_memory_type_changed(struct domain *d)

why making whole p2m_memory_type_changed under CONFIG_HVM?

>  {
> -    struct p2m_domain *p2m = p2m_get_hostp2m(d);
> +    struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
> 
> -    if ( p2m->memory_type_changed )
> +    p2m_lock(hostp2m);
> +
> +    _memory_type_changed(hostp2m);
> +
> +    if ( unlikely(altp2m_active(d)) )
>      {
> -        p2m_lock(p2m);
> -        p2m->memory_type_changed(p2m);
> -        p2m_unlock(p2m);
> +        unsigned int i;
> +
> +        for ( i = 0; i < MAX_ALTP2M; i++ )
> +            if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
> +            {
> +                struct p2m_domain *altp2m = d->arch.altp2m_p2m[i];
> +
> +                p2m_lock(altp2m);
> +                _memory_type_changed(altp2m);
> +                p2m_unlock(altp2m);
> +            }
>      }
> +
> +    p2m_unlock(hostp2m);
>  }
> +#endif
> 
>  int p2m_set_ioreq_server(struct domain *d,
>                           unsigned int flags,
> @@ -992,18 +1039,14 @@ int p2m_change_type_one(struct domain *d,
> unsigned long gfn_l,
>  }
> 
>  /* Modify the p2m type of a range of gfns from ot to nt. */
> -void p2m_change_type_range(struct domain *d,
> -                           unsigned long start, unsigned long end,
> -                           p2m_type_t ot, p2m_type_t nt)
> +static void change_type_range(struct p2m_domain *p2m,
> +                              unsigned long start, unsigned long end,
> +                              p2m_type_t ot, p2m_type_t nt)
>  {
>      unsigned long gfn = start;
> -    struct p2m_domain *p2m = p2m_get_hostp2m(d);
> +    struct domain *d = p2m->domain;
>      int rc = 0;
> 
> -    ASSERT(ot != nt);
> -    ASSERT(p2m_is_changeable(ot) && p2m_is_changeable(nt));
> -
> -    p2m_lock(p2m);
>      p2m->defer_nested_flush = 1;
> 
>      if ( unlikely(end > p2m->max_mapped_pfn) )
> @@ -1047,7 +1090,39 @@ void p2m_change_type_range(struct domain *d,
>      p2m->defer_nested_flush = 0;
>      if ( nestedhvm_enabled(d) )
>          p2m_flush_nestedp2m(d);
> -    p2m_unlock(p2m);
> +}
> +
> +void p2m_change_type_range(struct domain *d,
> +                           unsigned long start, unsigned long end,
> +                           p2m_type_t ot, p2m_type_t nt)
> +{
> +    struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
> +
> +    ASSERT(ot != nt);
> +    ASSERT(p2m_is_changeable(ot) && p2m_is_changeable(nt));
> +
> +    p2m_lock(hostp2m);
> +
> +    change_type_range(hostp2m, start, end, ot, nt);
> +
> +#ifdef CONFIG_HVM
> +    if ( unlikely(altp2m_active(d)) )
> +    {
> +        unsigned int i;
> +
> +        for ( i = 0; i < MAX_ALTP2M; i++ )
> +            if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
> +            {
> +                struct p2m_domain *altp2m = d->arch.altp2m_p2m[i];
> +
> +                p2m_lock(altp2m);
> +                change_type_range(altp2m, start, end, ot, nt);
> +                p2m_unlock(altp2m);
> +            }
> +    }
> +#endif
> +
> +    p2m_unlock(hostp2m);
>  }
> 
>  /*
> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
> index c7f5710..be5b7a2 100644
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -630,9 +630,6 @@ int p2m_finish_type_change(struct domain *d,
>                             gfn_t first_gfn,
>                             unsigned long max_nr);
> 
> -/* Report a change affecting memory types. */
> -void p2m_memory_type_changed(struct domain *d);
> -
>  int p2m_is_logdirty_range(struct p2m_domain *, unsigned long start,
>                            unsigned long end);
> 
> @@ -663,6 +660,9 @@ void p2m_pod_dump_data(struct domain *d);
> 
>  #ifdef CONFIG_HVM
> 
> +/* Report a change affecting memory types. */
> +void p2m_memory_type_changed(struct domain *d);
> +
>  /* Called by p2m code when demand-populating a PoD page */
>  bool
>  p2m_pod_demand_populate(struct p2m_domain *p2m, gfn_t gfn,
> unsigned int order);
> --
> 2.7.4


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