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 4:08 PM, George Dunlap wrote:
> 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?

No no, I think you're right - I'd in any case bet on your knowledge of
this over mine, I was just trying to make sure I'm not losing sight of
anything.

I'll start working on the next version of the patch.


Thanks again,
Razvan

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