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

[George Dunlap <George.Dunlap@xxxxxxxxxx>]

> On Nov 14, 2018, at 8:40 PM, Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx> 
> 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;
> * modifies p2m_change_entry_type_global(), p2m_memory_type_changed
>  and p2m_change_type_range() to propagate their changes to all
>  valid altp2ms.
> 
> With the introduction of altp2m fields in p2m_memory_type_changed()
> the whole function has been put under CONFIG_HVM.
> 
> Signed-off-by: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
> Suggested-by: George Dunlap <george.dunlap@xxxxxxxxxx>
> Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>

Just one more question...

> 
> ---
> 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 V5:
> - Added Kevin's Reviewed-by.
> - Added a note on p2m_memory_type_changed() being under CONFIG_HVM.
> ---
> 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;

Why we need to copy this value?

I’ve just spent the afternoon tracing around the source code, and while I’m 
pretty sure it won’t hurt, I’m also not sure why it’s necessary.

Everything else looks good!

 -George

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