[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH for-4.12 V2] x86/altp2m: fix HVMOP_altp2m_set_domain_state race
>>> 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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |