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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.09.18 at 17:25, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> On 9/28/18 5:52 PM, Jan Beulich wrote:
>>>>> On 28.09.18 at 13:55, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>>> @@ -1218,34 +1219,67 @@ static void ept_tlb_flush(struct p2m_domain *p2m)
>>>      ept_sync_domain_mask(p2m, p2m->domain->dirty_cpumask);
>>>  }
>>>  
>>> +static void ept_set_ad_sync(struct p2m_domain *p2m, int value)
>>> +{
>>> +    struct domain *d = p2m->domain;
>>> +    unsigned int i;
>>> +
>>> +    if ( likely(!altp2m_active(d)) )
>>> +    {
>>> +        p2m_lock(p2m);
>>> +        p2m->ept.ad = value;
>>> +        p2m_unlock(p2m);
>>> +
>>> +        return;
>>> +    }
>> 
>> Why would you want to skip updating the host p2m's flag when
>> altp2m is active?
> 
> It's not really skipped if I understand the altp2m code correctly: in
> that case the hostp2m is d->arch.altp2m_p2m[0], which is take care of in
> the loop below the code you've quoted.

p2m_init_altp2m() (and other code in p2m.c) suggests otherwise to me.

>>> --- a/xen/arch/x86/mm/p2m.c
>>> +++ b/xen/arch/x86/mm/p2m.c
>>> @@ -360,11 +360,7 @@ void p2m_enable_hardware_log_dirty(struct domain *d)
>>>      struct p2m_domain *p2m = p2m_get_hostp2m(d);
>>>  
>>>      if ( p2m->enable_hardware_log_dirty )
>>> -    {
>>> -        p2m_lock(p2m);
>>>          p2m->enable_hardware_log_dirty(p2m);
>>> -        p2m_unlock(p2m);
>>> -    }
>>>  }
>>>  
>>>  void p2m_disable_hardware_log_dirty(struct domain *d)
>>> @@ -372,11 +368,7 @@ void p2m_disable_hardware_log_dirty(struct domain *d)
>>>      struct p2m_domain *p2m = p2m_get_hostp2m(d);
>>>  
>>>      if ( p2m->disable_hardware_log_dirty )
>>> -    {
>>> -        p2m_lock(p2m);
>>>          p2m->disable_hardware_log_dirty(p2m);
>>> -        p2m_unlock(p2m);
>>> -    }
>>>  }
>> 
>> I don't understand how this removal can be correct.
> 
> Do you mean because the lock is supposed to protect the
> vmx_domain_disable_pml(d); and vmx_domain_update_eptp(d); calls as well?

Not just there - I think you need to keep at least a read lock on
the host p2m until you've managed to update all the altp2m-s.

Jan



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