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

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 10/1/18 2:23 PM, George Dunlap wrote:
> 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.

I'll do that. Thanks!


Razvan

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