| Message ID | 20201221181446.7791-2-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/domain: More structured teardown | expand |
On 21.12.2020 19:14, Andrew Cooper wrote: > --- a/xen/common/domain.c > +++ b/xen/common/domain.c > @@ -391,25 +391,7 @@ struct domain *domain_create(domid_t domid, > > TRACE_1D(TRC_DOM0_DOM_ADD, d->domain_id); > > - /* > - * Allocate d->vcpu[] and set ->max_vcpus up early. Various per-domain > - * resources want to be sized based on max_vcpus. > - */ > - if ( !is_system_domain(d) ) > - { > - err = -ENOMEM; > - d->vcpu = xzalloc_array(struct vcpu *, config->max_vcpus); > - if ( !d->vcpu ) > - goto fail; > - > - d->max_vcpus = config->max_vcpus; > - } > - > - lock_profile_register_struct(LOCKPROF_TYPE_PERDOM, d, domid); Wouldn't this also count as "trivial initialization", and hence while moving want to at least be placed ... > - if ( (err = xsm_alloc_security_domain(d)) != 0 ) > - goto fail; > - > + /* Trivial initialisation. */ > atomic_set(&d->refcnt, 1); > RCU_READ_LOCK_INIT(&d->rcu_lock); > spin_lock_init_prof(d, domain_lock); > @@ -434,6 +416,27 @@ struct domain *domain_create(domid_t domid, > INIT_LIST_HEAD(&d->pdev_list); > #endif > > + /* All error paths can depend on the above setup. */ ... ahead of this comment? > + /* > + * Allocate d->vcpu[] and set ->max_vcpus up early. Various per-domain > + * resources want to be sized based on max_vcpus. > + */ > + if ( !is_system_domain(d) ) > + { > + err = -ENOMEM; > + d->vcpu = xzalloc_array(struct vcpu *, config->max_vcpus); > + if ( !d->vcpu ) > + goto fail; > + > + d->max_vcpus = config->max_vcpus; > + } > + > + lock_profile_register_struct(LOCKPROF_TYPE_PERDOM, d, domid); > + > + if ( (err = xsm_alloc_security_domain(d)) != 0 ) > + goto fail; > + > err = -ENOMEM; > if ( !zalloc_cpumask_var(&d->dirty_cpumask) ) > goto fail; > Just as an observation (i.e. unlikely for this patch) I doubt system domains need ->dirty_cpumask set up, and hence this allocation may also want moving down a few lines. Jan
On 22/12/2020 10:10, Jan Beulich wrote: > On 21.12.2020 19:14, Andrew Cooper wrote: >> --- a/xen/common/domain.c >> +++ b/xen/common/domain.c >> @@ -391,25 +391,7 @@ struct domain *domain_create(domid_t domid, >> >> TRACE_1D(TRC_DOM0_DOM_ADD, d->domain_id); >> >> - /* >> - * Allocate d->vcpu[] and set ->max_vcpus up early. Various per-domain >> - * resources want to be sized based on max_vcpus. >> - */ >> - if ( !is_system_domain(d) ) >> - { >> - err = -ENOMEM; >> - d->vcpu = xzalloc_array(struct vcpu *, config->max_vcpus); >> - if ( !d->vcpu ) >> - goto fail; >> - >> - d->max_vcpus = config->max_vcpus; >> - } >> - >> - lock_profile_register_struct(LOCKPROF_TYPE_PERDOM, d, domid); > Wouldn't this also count as "trivial initialization", and hence while > moving want to at least be placed ... > >> - if ( (err = xsm_alloc_security_domain(d)) != 0 ) >> - goto fail; >> - >> + /* Trivial initialisation. */ >> atomic_set(&d->refcnt, 1); >> RCU_READ_LOCK_INIT(&d->rcu_lock); >> spin_lock_init_prof(d, domain_lock); >> @@ -434,6 +416,27 @@ struct domain *domain_create(domid_t domid, >> INIT_LIST_HEAD(&d->pdev_list); >> #endif >> >> + /* All error paths can depend on the above setup. */ > ... ahead of this comment? Can do. > >> + /* >> + * Allocate d->vcpu[] and set ->max_vcpus up early. Various per-domain >> + * resources want to be sized based on max_vcpus. >> + */ >> + if ( !is_system_domain(d) ) >> + { >> + err = -ENOMEM; >> + d->vcpu = xzalloc_array(struct vcpu *, config->max_vcpus); >> + if ( !d->vcpu ) >> + goto fail; >> + >> + d->max_vcpus = config->max_vcpus; >> + } >> + >> + lock_profile_register_struct(LOCKPROF_TYPE_PERDOM, d, domid); >> + >> + if ( (err = xsm_alloc_security_domain(d)) != 0 ) >> + goto fail; >> + >> err = -ENOMEM; >> if ( !zalloc_cpumask_var(&d->dirty_cpumask) ) >> goto fail; >> > Just as an observation (i.e. unlikely for this patch) I doubt > system domains need ->dirty_cpumask set up, and hence this > allocation may also want moving down a few lines. I agree in principle. However, something does (or at least did) reference this for system domains when I did the is_system_domain() cleanup. The fix might not be as trivial as just moving the allocation. ~Andrew
On 22.12.2020 11:24, Andrew Cooper wrote: > On 22/12/2020 10:10, Jan Beulich wrote: >> On 21.12.2020 19:14, Andrew Cooper wrote: >>> --- a/xen/common/domain.c >>> +++ b/xen/common/domain.c >>> @@ -391,25 +391,7 @@ struct domain *domain_create(domid_t domid, >>> >>> TRACE_1D(TRC_DOM0_DOM_ADD, d->domain_id); >>> >>> - /* >>> - * Allocate d->vcpu[] and set ->max_vcpus up early. Various per-domain >>> - * resources want to be sized based on max_vcpus. >>> - */ >>> - if ( !is_system_domain(d) ) >>> - { >>> - err = -ENOMEM; >>> - d->vcpu = xzalloc_array(struct vcpu *, config->max_vcpus); >>> - if ( !d->vcpu ) >>> - goto fail; >>> - >>> - d->max_vcpus = config->max_vcpus; >>> - } >>> - >>> - lock_profile_register_struct(LOCKPROF_TYPE_PERDOM, d, domid); >> Wouldn't this also count as "trivial initialization", and hence while >> moving want to at least be placed ... >> >>> - if ( (err = xsm_alloc_security_domain(d)) != 0 ) >>> - goto fail; >>> - >>> + /* Trivial initialisation. */ >>> atomic_set(&d->refcnt, 1); >>> RCU_READ_LOCK_INIT(&d->rcu_lock); >>> spin_lock_init_prof(d, domain_lock); >>> @@ -434,6 +416,27 @@ struct domain *domain_create(domid_t domid, >>> INIT_LIST_HEAD(&d->pdev_list); >>> #endif >>> >>> + /* All error paths can depend on the above setup. */ >> ... ahead of this comment? > > Can do. At which point Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
diff --git a/xen/common/domain.c b/xen/common/domain.c index 5ec48c3e19..ce3667f1b4 100644 --- a/xen/common/domain.c +++ b/xen/common/domain.c @@ -391,25 +391,7 @@ struct domain *domain_create(domid_t domid, TRACE_1D(TRC_DOM0_DOM_ADD, d->domain_id); - /* - * Allocate d->vcpu[] and set ->max_vcpus up early. Various per-domain - * resources want to be sized based on max_vcpus. - */ - if ( !is_system_domain(d) ) - { - err = -ENOMEM; - d->vcpu = xzalloc_array(struct vcpu *, config->max_vcpus); - if ( !d->vcpu ) - goto fail; - - d->max_vcpus = config->max_vcpus; - } - - lock_profile_register_struct(LOCKPROF_TYPE_PERDOM, d, domid); - - if ( (err = xsm_alloc_security_domain(d)) != 0 ) - goto fail; - + /* Trivial initialisation. */ atomic_set(&d->refcnt, 1); RCU_READ_LOCK_INIT(&d->rcu_lock); spin_lock_init_prof(d, domain_lock); @@ -434,6 +416,27 @@ struct domain *domain_create(domid_t domid, INIT_LIST_HEAD(&d->pdev_list); #endif + /* All error paths can depend on the above setup. */ + + /* + * Allocate d->vcpu[] and set ->max_vcpus up early. Various per-domain + * resources want to be sized based on max_vcpus. + */ + if ( !is_system_domain(d) ) + { + err = -ENOMEM; + d->vcpu = xzalloc_array(struct vcpu *, config->max_vcpus); + if ( !d->vcpu ) + goto fail; + + d->max_vcpus = config->max_vcpus; + } + + lock_profile_register_struct(LOCKPROF_TYPE_PERDOM, d, domid); + + if ( (err = xsm_alloc_security_domain(d)) != 0 ) + goto fail; + err = -ENOMEM; if ( !zalloc_cpumask_var(&d->dirty_cpumask) ) goto fail;
This improves the robustness of the error paths. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Julien Grall <julien@xen.org> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> --- xen/common/domain.c | 41 ++++++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 19 deletions(-)