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: Wednesday, November 14, 2018 3:33 PM
> 
> On 11/14/18 7:55 AM, Tian, Kevin wrote:
> >> 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?
> 
> It has been requested by Jan and Wei in V3:
> 
> https://lists.xenproject.org/archives/html/xen-devel/2018-
> 10/msg02495.html
> 

then it's good to mention it in patch description.

Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel