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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 09/19/2018 02:01 PM, Razvan Cojocaru wrote:
> On 9/19/18 3:15 PM, George Dunlap wrote:
>> Hey Razvan, thanks for doing this, and sorry it's taken so long to respond.
> 
> No problem, thanks for the review!
> 
>>> 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.
>>
>> Another issue is that it doesn't look like you're propagating updates to
>> this shared state either -- if someone enables or disables
>> global_logdirty, or changes default_access, the altp2ms will still have
>> the old value.
>>
>> I wonder if we should collect the various bits that need to be kept in
>> sync between hostp2m/altp2ms, put them all in a 'sync' sub-struct within
>> the p2m, and enforce using a function / macro to modify the values inside.
> 
> Right, I'll get on with the sync structure then.
> 
>>> Another aspect is that, while new modifications should work with
>>> these changes, _old_ modifications (written to the host2pm
>>> _before_ we've created the new altp2m) will, if I understand the
>>> code correctly be lost. That is to say, misconfigurations
>>> performed before p2m_init_altp2m_ept() in the hostp2m will
>>> presumably not trigger the necessary faults after switching to
>>> the new altp2m.
>>
>> You're worried about the following sequence?
>>
>> 1. Misconfigure hostp2m
>> 2. Enable altp2m
>> 3. Switch to altp2m 1
>> 4. Fault on a previously-misconfigured p2m entry
> 
> No, I'm worried that at step 4 the fault will no longer happen, because
> the EPTP index corresponding to altp2m 1 points to an EPT where the
> entries misconfigured in the hostp2m are _not_ misconfigured.
> 
> But actually the sequence I'm worried about is:
> 
> 1. Misconfigure hostp2m
> 2. Create altp2m
> 3. Enable altp2m
> 4. Switch to altp2m 1
> 5. A would-be-fault in the hostp2m no longer occurs

But how would a fault not occur?  The altp2m at this point won't have
*any* entries; any p2m access at all should fault, yes?  At which point
the altp2m code will say, "Oh, look, here's an entry I didn't have; I'll
copy it from the host p2m".  That will call hostp2m->get_entry(), which
will resolve the misconfig.

Or do I need to go back and review the altp2m code again so I have a
clue how it works?

 -George

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