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

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 11/15/18 7:34 PM, George Dunlap wrote:
> 
> 
>> 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.

I think I did that because I assumed that it would matter for
ept_get_entry() and misconfigurations, when:

 954     /* This pfn is higher than the highest the p2m map currently
holds */
 955     if ( gfn > p2m->max_mapped_pfn )
 956     {
 957         for ( i = ept->wl; i > 0; --i )
 958             if ( (gfn & ~((1UL << (i * EPT_TABLE_ORDER)) - 1)) >
 959                  p2m->max_mapped_pfn )
 960                 break;
 961         goto out;
 962     }

but I am not certain it is required either. I can certainly remove this
assignment and see if anything breaks.

> Everything else looks good!


Thanks,
Razvan

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