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

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

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?


Thanks,
Razvan

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