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

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 9/19/18 3:44 PM, George Dunlap wrote:
> On 09/19/2018 01:15 PM, George Dunlap wrote:
>> On 09/03/2018 09:25 AM, 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.
>>
>> Hey Razvan, thanks for doing this, and sorry it's taken so long to respond.
>>
>>> This patch:
>>>
>>> * updates ept_handle_misconfig() to use the active altp2m instead
>>>   of the hostp2m;
>>
>> This is probably necessary.
>>
>>> * has p2m_init_altp2m_ept() copy over max_mapped_pfn,
>>>   logdirty_ranges, global_logdirty, ept.ad and default_access
>>>   from the hostp2m (the latter more for completeness than for any
>>>   other reason).
>>
>> I think this is probably the right approach.  These values change
>> rarely, but after a misconfig are read repeatedly.  So it's probably a
>> lot more efficient to propagate changes when they happen, rather than
>> trying to keep a single master copy.  However...
>>
>>> We should discuss if just copying over
>>>   logdirty_ranges (which is a pointer) is sufficient, or if
>>>   this code requires more synchronization).
>>
>> It's clearly not sufficient. :-)  The logdirty_ranges struct is
>> protected by the lock of the p2m structure that contains it; if you
>> point to it from a different p2m structure, then you'll have
>> inconsistent logging, and you'll have problems if one vcpu is reading
>> the structure while another is modifying it.
> 
> ...and therefore, if we believe that it's more efficient to duplicate
> structures than to share it and use a lock, we need to do a deep copy of
> the data structure on altp2m creation, and propagate changes as we do
> for the other "synced" data.

The biggest problem here is p2m->logdirty_ranges. This patch will
(justly) not work, because struct rangeset is only forward-declared in
xen/rangeset.h, so an incomplete type here:

-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;

+    if ( !p2m->logdirty_ranges )
+        p2m->logdirty_ranges = rangeset_new(d, "log-dirty",
+                                            RANGESETF_prettyprint_hex);
+    if ( !p2m->logdirty_ranges )
+        return -ENOMEM;
+
+    *p2m->logdirty_ranges = *hostp2m->logdirty_ranges;
+
     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->global_logdirty = hostp2m->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;
+}

But that's not even the biggest problem: even if that would compile, it
would still be wrong, because logdirty_pages has pointers of its own,
which means that two bitwise-copied distinct rangesets can still point
to the same data and thus be vulnerable to race conditions and wanting
synchronization.

Furthermore there's no rangeset_copy() function in sight in rangeset.h
(though there is a rangeset_swap()).

Would you like me to add a rangeset_copy() function (presumably another
intermediary patch) and proceed in that manner?


Thanks,
Razvan

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