Re: [Xen-devel] [PATCH V2] x86/altp2m: propagate ept.ad changes to all active altp2ms

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 10/1/18 1:39 PM, Jan Beulich wrote:
>>>> On 01.10.18 at 11:58, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>> Changes since V1:
>>  - Removed unnecessary p2m_lock() in p2m_init_altp2m_ept().
> 
> This was a step in the right direction, but ...
> 
>>  static void ept_enable_pml(struct p2m_domain *p2m)
>>  {
>> +    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);
>> +    ept_set_ad_sync(p2m, true);
>> +
>> +    vmx_domain_update_eptp(d);
>>  }
>>  
>>  static void ept_disable_pml(struct p2m_domain *p2m)
>>  {
>> +    struct domain *d = p2m->domain;
>> +
>>      /* Domain must have been paused */
>> -    ASSERT(atomic_read(&p2m->domain->pause_count));
>> +    ASSERT(atomic_read(&d->pause_count));
>>  
>> -    vmx_domain_disable_pml(p2m->domain);
>> +    vmx_domain_disable_pml(d);
>>  
>>      /* Disable EPT A/D bit */
>> -    p2m->ept.ad = 0;
>> -    vmx_domain_update_eptp(p2m->domain);
>> +    ept_set_ad_sync(p2m, false);
>> +
>> +    vmx_domain_update_eptp(d);
>>  }
> 
> These two functions used to be called with the p2m lock held,
> while now they aren't anymore. Afaict this introduces a race
> where the opposite ept_set_ad_sync() may be called before
> an original one was follow by the respective
> vmx_domain_update_eptp(), resulting in the A/D enable bit
> being set the wrong way round in the end.
> 
> I realize that George did already point out that this is sort of
> ugly a situation, but the fixing of the issue here shouldn't
> introduce a new race. What's wrong with retaining the
> host p2m lock in p2m_{en,dis}able_hardware_log_dirty()?
> ept_set_ad_sync() then simply wouldn't acquire/release that
> one, but just the altp2m ones.

That is fine with be, in fact the whole change has been prompted by
George's remark that "there would something a bit funny here about
grabbing the main p2m lock in p2m.c, and then grabbing altp2m locks
within the function". If, after these comments, he doesn't mind the
scenario then I'll do that in V3.


Thanks,
Razvan

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