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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 10/01/2018 12:11 PM, Razvan Cojocaru wrote:
> 
> 
> 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.

I think I would rather grab the main p2m locks in
ept_{enable,disable}_pml().  Wouldn't hurt to have an
ASSERT(p2m_is_locked_by_me()) in ept_set_ad_sync() as well.

Thanks,
 -George

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