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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.10.18 at 11:58, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> Changes since V1:
>  - Removed unnecessary p2m_lock() in p2m_init_altp2m_ept().

This was a step in the right direction, but ...

>  static void ept_enable_pml(struct p2m_domain *p2m)
>  {
> +    struct domain *d = p2m->domain;
> +
>      /* 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(p2m, true);
> +
> +    vmx_domain_update_eptp(d);
>  }
>  
>  static void ept_disable_pml(struct p2m_domain *p2m)
>  {
> +    struct domain *d = p2m->domain;
> +
>      /* 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(p2m, false);
> +
> +    vmx_domain_update_eptp(d);
>  }

These two functions used to be called with the p2m lock held,
while now they aren't anymore. Afaict this introduces a race
where the opposite ept_set_ad_sync() may be called before
an original one was follow by the respective
vmx_domain_update_eptp(), resulting in the A/D enable bit
being set the wrong way round in the end.

I realize that George did already point out that this is sort of
ugly a situation, but the fixing of the issue here shouldn't
introduce a new race. What's wrong with retaining the
host p2m lock in p2m_{en,dis}able_hardware_log_dirty()?
ept_set_ad_sync() then simply wouldn't acquire/release that
one, but just the altp2m ones.

Jan



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