[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



On 2/11/19 12:57 PM, Razvan Cojocaru wrote:
On 2/11/19 12:10 PM, Jan Beulich wrote:
On 11.02.19 at 10:13, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
--- 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);

With the change you now make to p2m_get_altp2m(), this looks to be
a benign change. Which to me would suggest to either leave the code
alone, or to drop the if() (but - again - not its body) altogether. At
which point the code could be further streamlined, as then the NULL
initializer can go away and the assignment (or then perhaps initializer)
could become "p2m = p2m_get_altp2m(v) ?: p2m_get_hostp2m(d)".
(Generally I'd recommend to leave out the change here, and do the
transformation in a follow-on patch.)

Thanks for noticing, actually this appears to invalidate the whole purpose of the patch (I should have tested this more before sumbitting V3, sorry).

The whole point of the new boolean is to have p2m assigned an altp2m regardless of altp2m_active() (hence the change) - which now no longer happens. I got carried away with this change.

The fact that this is so easy to get wrong is the reason why I've preferred the domain_pause() solution. There appears to be no clean way to fix this otherwise, and if this is so easy to misunderstand it'll break just as easily with further changes.

I suppose I could just pass the bool along to p2m_get_altp2m() (and indeed remove the if())...

I think the best that can be done here is to check if altp2m_active() early in p2m_switch_vcpu_altp2m_by_id() and p2m_switch_domain_altp2m_by_id(), then bail if it's not active. Since these are only called by HVMOP_altp2m_* (and thus serialized by the domain lock), it should be enough WRT HVMOP_altp2m_set_domain_state.

This of course means reverting p2m_get_altp2m() to its original non-intuitive state of returning a valid altp2m pointer even when altp2m_active() is false.

I see no other way out of this (aside from the domain_pause() fix).


Thanks,
Razvan

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.