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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.10.18 at 17:17, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> +static void ept_set_ad_sync(struct p2m_domain *hostp2m, bool value)
> +{
> +    struct domain *d = hostp2m->domain;
> +
> +    ASSERT(p2m_is_hostp2m(hostp2m));
> +    ASSERT(p2m_locked_by_me(hostp2m));
> +
> +    hostp2m->ept.ad = value;
> +
> +    if ( unlikely(altp2m_active(d)) )
> +    {
> +        unsigned int i;
> +
> +        for ( i = 0; i < MAX_ALTP2M; i++ )
> +        {
> +            struct p2m_domain *p2m;
> +
> +            if ( d->arch.altp2m_eptp[i] == mfn_x(INVALID_MFN) )
> +                continue;
> +
> +            p2m = d->arch.altp2m_p2m[i];
> +
> +            p2m_lock(p2m);
> +            p2m->ept.ad = value;
> +            p2m_unlock(p2m);

Just one further general remark here, coming back to whether [0]
represent the hostp2m: How would acquiring the lock here not
deadlock (the hostp2m is already locked, after all) if that were the
case?

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

Jan



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