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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

> To be honest, I have thought about changing the function signature and
> pass in a domain - however I didn't because this appears to be actually
> tied to a platform-independent callback, and it's likely that the p2m
> parameter makes more sense for those. (Also, all the other callbacks
> take a p2m parameter, which probably serves a similar purpose to C++'s
> "this".)

Yeah, this model should perhaps be kept as is.

Jan


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