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

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 10/5/18 12:01 PM, Jan Beulich wrote:
>>>> On 05.10.18 at 10:41, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>> On 10/5/18 11:17 AM, Jan Beulich wrote:
>>>>>> On 04.10.18 at 18:40, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>>>> On 10/4/18 7:04 PM, Jan Beulich wrote:
>>>>>>>> On 02.10.18 at 17:17, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>>>>>>  static void ept_enable_pml(struct p2m_domain *p2m)
>>>>>>  {
>>>>>> +    struct domain *d = p2m->domain;
>>>>>> +    struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
>>>>>> +
>>>>>> +    p2m_lock(hostp2m);
>>>>>> +
>>>>>>      /* 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(hostp2m, true);
>>>>>> +
>>>>>> +    vmx_domain_update_eptp(d);
>>>>>> +
>>>>>> +    p2m_unlock(hostp2m);
>>>>>>  }
>>>>>>  
>>>>>>  static void ept_disable_pml(struct p2m_domain *p2m)
>>>>>>  {
>>>>>> +    struct domain *d = p2m->domain;
>>>>>> +    struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
>>>>>> +
>>>>>> +    p2m_lock(hostp2m);
>>>>>> +
>>>>>>      /* 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(hostp2m, false);
>>>>>> +
>>>>>> +    vmx_domain_update_eptp(d);
>>>>>> +
>>>>>> +    p2m_unlock(hostp2m);
>>>>>>  }
>>>>>
>>>>> While in certain cases I would appreciate such transformations,
>>>>> I'm afraid the switch from p2m->domain to d in these two
>>>>> functions is hiding the meat of the change pretty well. In
>>>>> particular it is only now that I notice that you go from passed in
>>>>> p2m to domain to hostp2m. This makes me assume some altp2m
>>>>> could come in here too. Is it really intended for a change to
>>>>> an altp2m to be propagate to the hostp2m (and all other
>>>>> altp2m-s)? I can see why altp2m-s want to stay in sync (in
>>>>> certain regards) with the hostp2m, but for a sync the other
>>>>> way around there need to be deeper reasons.
>>>>>
>>>>> I admit that part of the problem here might be that the whole
>>>>> function hierarchy you change is tied to log-dirty enabling/
>>>>> disabling, but I'm not convinced PML as well as A/D enabled
>>>>> status has to always match global(?) log-dirty enabled status.
>>>>>
>>>>> But I'm not the maintainer of this code, so please don't
>>>>> interpret my response as a strict request for change.
>>>>
>>>> As far as I've understood from George, they do all need to be kept in
>>>> sync. And I see no reason why an altp2m couldn't be passed in there as
>>>> well - are you referring to the fact that p2m->domain is not right in
>>>> that case? I should probably add p2m->domain = hostp2m->domain; in
>>>> p2m_init_altp2m_ept() in this patch, I think that slipped when I've
>>>> split the patches.
>>>
>>> No, I don't think the domain pointer can conflict. Instead I think
>>> there could be reasons why one view may want to have A/D
>>> and/or PML activated without this being wanted/needed on all
>>> others, let alone the host one. But I've meanwhile realized that
>>> this may also merely be a function naming issue:
>>> ept_{en,dis}able_pml are not overly helpful names for something
>>> to be put in directly as {en,dis}able_hardware_log_dirty hooks.
>>> By their names, the functions should act on just the specified
>>> P2M imo. The hook handlers, otoh, would validly sync the setting
>>> to all P2Ms, as log-dirty mode is a domain-wide setting.
>>
>> Would sending out V4 with ept_{en,dis}able_pml() renamed to
>> ept_{en,dis}able_hardware_log_dirty() be a reasonable solution to the
>> problem?
> 
> Afaic - I'd prefer the functions to remain as they are, with _new_
> ept_{en,dis}able_hardware_log_dirty() introduced, to be put in
> the hook pointers. The new functions would then call the existing
> ones for all P2Ms (with the host p2m lock acquired). The question
> then is what to do with the calls to the domain-scope
> vmx_domain_{en,dis}able_pml(); looking at its implementation I
> think it could simply stay where it is. The boolean
> vmx_domain_pml_enabled() would need to eventually change to
> an enable count, but that's nothing you need to be concerned
> about for your purposes.
> 
> But again - please check what maintainers want before going
> this route.

I see, in which case George please let me know if you're okay with Jan's
suggestion when you get a chance to read the thread.


Thanks,
Razvan

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