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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

Jan


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