Re: [Xen-devel] [PATCH for-4.12 V2] x86/altp2m: fix HVMOP_altp2m_set_domain_state race

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.02.19 at 15:00, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4525,7 +4525,7 @@ static int do_altp2m_op(
>      case HVMOP_altp2m_set_domain_state:
>      {
>          struct vcpu *v;
> -        bool_t ostate;
> +        bool ostate, nstate;
>  
>          if ( nestedhvm_enabled(d) )
>          {
> @@ -4534,12 +4534,16 @@ static int do_altp2m_op(
>          }
>  
>          ostate = d->arch.altp2m_active;
> -        d->arch.altp2m_active = !!a.u.domain_state.state;
> +        nstate = !!a.u.domain_state.state;

No need for !! here.

>          /* If the alternate p2m state has changed, handle appropriately */
> -        if ( d->arch.altp2m_active != ostate &&
> +        if ( nstate != ostate &&
>               (ostate || !(rc = p2m_init_altp2m_by_id(d, 0))) )
>          {
> +            /* First mark altp2m as disabled, then altp2m_vcpu_destroy(). */
> +            if ( ostate )
> +                d->arch.altp2m_active = false;

Why the if()? In the opposite case you'd simply write false into
what already holds false.

> @@ -4550,7 +4554,14 @@ static int do_altp2m_op(
>  
>              if ( ostate )
>                  p2m_flush_altp2m(d);
> +            else
> +                /*
> +                 * Wait until altp2m_vcpu_initialise() is done before marking
> +                 * altp2m as being enabled for the domain.
> +                 */
> +                d->arch.altp2m_active = true;

Similarly here you could omit the "else" and simply store "nstate" afaict.

> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2150,13 +2150,13 @@ static bool_t vmx_is_singlestep_supported(void)
>      return !!cpu_has_monitor_trap_flag;
>  }
>  
> -static void vmx_vcpu_update_eptp(struct vcpu *v)
> +static void vmx_vcpu_update_eptp(struct vcpu *v, bool altp2m_enabled)
>  {
>      struct domain *d = v->domain;
>      struct p2m_domain *p2m = NULL;
>      struct ept_data *ept;
>  
> -    if ( altp2m_active(d) )
> +    if ( altp2m_enabled )
>          p2m = p2m_get_altp2m(v);
>      if ( !p2m )
>          p2m = p2m_get_hostp2m(d);

Is this an appropriate transformation? That is, can there not be
any domains for which altp2m_active() returns false despite
altp2m_enabled being true? (Looking at p2m_get_altp2m() I can't
really judge whether index would always be INVALID_ALTP2M in
such a case.)

> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -2332,7 +2332,7 @@ bool_t p2m_switch_vcpu_altp2m_by_id(struct vcpu *v, 
> unsigned int idx)
>              atomic_dec(&p2m_get_altp2m(v)->active_vcpus);
>              vcpu_altp2m(v).p2midx = idx;
>              atomic_inc(&p2m_get_altp2m(v)->active_vcpus);
> -            altp2m_vcpu_update_p2m(v);
> +            altp2m_vcpu_update_p2m(v, altp2m_active(d));
>          }
>          rc = 1;
>      }
> @@ -2573,7 +2573,7 @@ int p2m_switch_domain_altp2m_by_id(struct domain *d, 
> unsigned int idx)
>                  atomic_dec(&p2m_get_altp2m(v)->active_vcpus);
>                  vcpu_altp2m(v).p2midx = idx;
>                  atomic_inc(&p2m_get_altp2m(v)->active_vcpus);
> -                altp2m_vcpu_update_p2m(v);
> +                altp2m_vcpu_update_p2m(v, altp2m_active(d));
>              }

In both cases, isn't altp2m_active() going to return true anyway
when we get there (related to the question above)?

Jan



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