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 17:30, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> On 2/8/19 5:50 PM, Jan Beulich wrote:
>>>>> On 08.02.19 at 15:00, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>>>          /* 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.
> 
> The value written into d->arch.altp2m_active is not the point here. The
> point is that if ( ostate ), then we are disabling altp2m (because the
> if above this one makes sure ostate != nstate).
> 
> So in the disable case, first I wanted to set d->arch.altp2m_active to
> false (which immediately causes altp2m_active(d) to return false as
> well), and then actually perform the work.

Sure - I'm not asking to remove everything above, just the if()
(and keep what is now the body). That is because
- in the enable case the value is false and writing false into it is
  benign,
- in the disable case you want to actively change from true to
  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.
> 
> As above, in the enable case I wanted to first setup altp2m on all VCPUs
> with altp2m_vcpu_initialise(), and only after that set
> d->arch.altp2m_active = true.
> 
> In summary, if ( ostate ) we want to set d->arch.altp2m_active before
> the for (we're disabling altp2m), and if ( !ostate ) (which is the else
> above) we want to set d->arch.altp2m_active after said for.
> 
> We can indeed store nstate. I just thought things look clearer with
> "true" and "false", but if you prefer there's no problem assigning nstate.

Well, as always with mm and altp2m code, I'm not the maintainer, so
I don't have the final say. It's just that I think unnecessary conditionals
would better be avoided, even if they're not on a performance critical
path.

>>> --- 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.)
> 
> Yes, it should be completely safe (please see below for details).
> 
>>> --- 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)?
> 
> Yes, it will return true. In order for it to return false,
> altp2m_vcpu_destroy() would have had to run on that VCPU, which (among
> other things) calls altp2m_vcpu_reset(), which does v->p2midx =
> INVALID_ALTP2M.

Hmm, according to the change further up in this patch, altp2m_active()
returns false before v->p2midx can be set to INVALID_ALTP2M (you
don't change this property). So (related to the change further up)
there's a period of time during which p2m_get_altp2m() will return
non-NULL despite altp2m_active() returning false. Afaict, that is.

Jan

> There's an if() above (not shown in your reply) that says "if ( idx !=
> vcpu_altp2m(v).p2midx )", so, indeed, by the time we end up here we can
> reasonably assume that altp2m_active(d) will return true.
> 
> I've just put in "altp2m_active(d)" to make sure there's absolutely no
> functional change here, but AFAICT it can be safely replaced with "true".
> 
> 
> Thanks,
> Razvan




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