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

Please note the additional step 2. At this point, the hostp2m has been
misconfigured, but the creation of altp2m came too late - so the
misconfiguration of the hostp2m could not have propagated to altp2m 1,
since it didn't yet exist when it was misconfigured.

Does not switching to altp2m 1 then lose the EPT misconfiguration?

> Just one comment on the code...
> 
>>  static void ept_enable_pml(struct p2m_domain *p2m)
>>  {
>> +    unsigned int i;
>> +    struct domain *d = p2m->domain;
>> +
>>      /* Domain must have been paused */
>> -    ASSERT(atomic_read(&p2m->domain->pause_count));
>> +    ASSERT(atomic_read(&d->pause_count));
>>  
>>      /*
>>       * No need to return whether vmx_domain_enable_pml has succeeded, as
>>       * ept_p2m_type_to_flags will do the check, and write protection will be
>>       * used if PML is not enabled.
>>       */
>> -    if ( vmx_domain_enable_pml(p2m->domain) )
>> +    if ( vmx_domain_enable_pml(d) )
>>          return;
>>  
>>      /* Enable EPT A/D bit for PML */
>>      p2m->ept.ad = 1;
>> -    vmx_domain_update_eptp(p2m->domain);
>> +
>> +    if ( altp2m_active(d) )
>> +        for ( i = 0; i < MAX_ALTP2M; i++ )
>> +        {
>> +            if ( d->arch.altp2m_eptp[i] == mfn_x(INVALID_MFN) )
>> +                continue;
>> +
>> +            p2m = d->arch.altp2m_p2m[i];
>> +            p2m->ept.ad = 1;
>> +        }
> 
> You're not grabbing the respective p2m locks here -- I'm pretty sure
> this will end up being three separate instructions (read, set ad bit,
> write).
> 
> But there would something a bit funny here about grabbing the main p2m
> lock in p2m.c, and then grabbing altp2m locks within the function.  But
> on the other hand, you clearly only want to call this...
> 
>> +    vmx_domain_update_eptp(d);
> 
> ...once.  Some refactoring might be wanted.

I'll see about that as well.


Thanks,
Razvan

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