* [PATCH 0/3] xen: Improvements to the vcpu create/destroy paths @ 2018-09-06 19:25 Andrew Cooper 2018-09-06 19:25 ` [PATCH 1/3] xen/vcpu: Rename the common interfaces for consistency Andrew Cooper ` (2 more replies) 0 siblings, 3 replies; 16+ messages in thread From: Andrew Cooper @ 2018-09-06 19:25 UTC (permalink / raw) To: Xen-devel Cc: Stefano Stabellini, Wei Liu, Andrew Cooper, Julien Grall, Jan Beulich, Roger Pau Monné This is the start of the work to make the vcpu_destroy() code idempotent, so vcpu construction can be moved into domain_create() and successfully unwound on error. Andrew Cooper (3): xen/vcpu: Rename the common interfaces for consistency xen/vcpu: Introduce vcpu_destroy() xen/vcpu: Rework sanity checks in vcpu_create() xen/arch/arm/domain.c | 6 ++-- xen/arch/arm/domain_build.c | 4 +-- xen/arch/arm/setup.c | 1 - xen/arch/x86/dom0_build.c | 2 +- xen/arch/x86/domain.c | 4 +-- xen/arch/x86/setup.c | 1 - xen/common/domain.c | 80 ++++++++++++++++++++++++++++++--------------- xen/common/domctl.c | 2 +- xen/common/schedule.c | 4 +-- xen/include/xen/domain.h | 10 +++--- 10 files changed, 69 insertions(+), 45 deletions(-) -- 2.1.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/3] xen/vcpu: Rename the common interfaces for consistency 2018-09-06 19:25 [PATCH 0/3] xen: Improvements to the vcpu create/destroy paths Andrew Cooper @ 2018-09-06 19:25 ` Andrew Cooper 2018-09-07 9:52 ` Jan Beulich ` (2 more replies) 2018-09-06 19:25 ` [PATCH 2/3] xen/vcpu: Introduce vcpu_destroy() Andrew Cooper 2018-09-06 19:25 ` [PATCH 3/3] xen/vcpu: Rework sanity checks in vcpu_create() Andrew Cooper 2 siblings, 3 replies; 16+ messages in thread From: Andrew Cooper @ 2018-09-06 19:25 UTC (permalink / raw) To: Xen-devel Cc: Stefano Stabellini, Wei Liu, Andrew Cooper, Julien Grall, Jan Beulich, Roger Pau Monné The vcpu functions are far less consistent than the domain side of things, and in particular, has vcpu_destroy() for architecture specific functionality. Perform the following renames: * alloc_vcpu => vcpu_create * vcpu_initialise => arch_vcpu_create * vcpu_destroy => arch_vcpu_destroy which makes the vcpu hierarchy consistent with the domain hierarchy. No functional change. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Wei Liu <wei.liu2@citrix.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Julien Grall <julien.grall@arm.com> --- xen/arch/arm/domain.c | 6 +++--- xen/arch/arm/domain_build.c | 4 ++-- xen/arch/x86/dom0_build.c | 2 +- xen/arch/x86/domain.c | 4 ++-- xen/common/domain.c | 6 +++--- xen/common/domctl.c | 2 +- xen/common/schedule.c | 4 ++-- xen/include/xen/domain.h | 10 +++++----- 8 files changed, 19 insertions(+), 19 deletions(-) diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index 4baecc2..feebbf5 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -538,7 +538,7 @@ void free_vcpu_struct(struct vcpu *v) free_xenheap_pages(v, get_order_from_bytes(sizeof(*v))); } -int vcpu_initialise(struct vcpu *v) +int arch_vcpu_create(struct vcpu *v) { int rc = 0; @@ -583,11 +583,11 @@ int vcpu_initialise(struct vcpu *v) return rc; fail: - vcpu_destroy(v); + arch_vcpu_destroy(v); return rc; } -void vcpu_destroy(struct vcpu *v) +void arch_vcpu_destroy(struct vcpu *v) { vcpu_timer_destroy(v); vcpu_vgic_free(v); diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 2a383c8..5057ad8 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -74,7 +74,7 @@ unsigned int __init dom0_max_vcpus(void) struct vcpu *__init alloc_dom0_vcpu0(struct domain *dom0) { - return alloc_vcpu(dom0, 0, 0); + return vcpu_create(dom0, 0, 0); } static unsigned int __init get_11_allocation_size(paddr_t size) @@ -2232,7 +2232,7 @@ int __init construct_dom0(struct domain *d) for ( i = 1, cpu = 0; i < d->max_vcpus; i++ ) { cpu = cpumask_cycle(cpu, &cpu_online_map); - if ( alloc_vcpu(d, i, cpu) == NULL ) + if ( vcpu_create(d, i, cpu) == NULL ) { printk("Failed to allocate dom0 vcpu %d on pcpu %d\n", i, cpu); break; diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c index 423fdec..86eb7db 100644 --- a/xen/arch/x86/dom0_build.c +++ b/xen/arch/x86/dom0_build.c @@ -134,7 +134,7 @@ struct vcpu *__init dom0_setup_vcpu(struct domain *d, unsigned int prev_cpu) { unsigned int cpu = cpumask_cycle(prev_cpu, &dom0_cpus); - struct vcpu *v = alloc_vcpu(d, vcpu_id, cpu); + struct vcpu *v = vcpu_create(d, vcpu_id, cpu); if ( v ) { diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c index 313ebb3..d67a047 100644 --- a/xen/arch/x86/domain.c +++ b/xen/arch/x86/domain.c @@ -322,7 +322,7 @@ void free_vcpu_struct(struct vcpu *v) free_xenheap_page(v); } -int vcpu_initialise(struct vcpu *v) +int arch_vcpu_create(struct vcpu *v) { struct domain *d = v->domain; int rc; @@ -382,7 +382,7 @@ int vcpu_initialise(struct vcpu *v) return rc; } -void vcpu_destroy(struct vcpu *v) +void arch_vcpu_destroy(struct vcpu *v) { xfree(v->arch.vm_event); v->arch.vm_event = NULL; diff --git a/xen/common/domain.c b/xen/common/domain.c index 78c450e..14c8d00 100644 --- a/xen/common/domain.c +++ b/xen/common/domain.c @@ -123,7 +123,7 @@ static void vcpu_info_reset(struct vcpu *v) v->vcpu_info_mfn = INVALID_MFN; } -struct vcpu *alloc_vcpu( +struct vcpu *vcpu_create( struct domain *d, unsigned int vcpu_id, unsigned int cpu_id) { struct vcpu *v; @@ -165,7 +165,7 @@ struct vcpu *alloc_vcpu( if ( sched_init_vcpu(v, cpu_id) != 0 ) goto fail_wq; - if ( vcpu_initialise(v) != 0 ) + if ( arch_vcpu_create(v) != 0 ) { sched_destroy_vcpu(v); fail_wq: @@ -870,7 +870,7 @@ static void complete_domain_destroy(struct rcu_head *head) if ( (v = d->vcpu[i]) == NULL ) continue; tasklet_kill(&v->continue_hypercall_tasklet); - vcpu_destroy(v); + arch_vcpu_destroy(v); sched_destroy_vcpu(v); destroy_waitqueue_vcpu(v); } diff --git a/xen/common/domctl.c b/xen/common/domctl.c index ed047b7..2facbdb 100644 --- a/xen/common/domctl.c +++ b/xen/common/domctl.c @@ -571,7 +571,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) cpumask_any(online) : cpumask_cycle(d->vcpu[i-1]->processor, online); - if ( alloc_vcpu(d, i, cpu) == NULL ) + if ( vcpu_create(d, i, cpu) == NULL ) goto maxvcpu_out; } diff --git a/xen/common/schedule.c b/xen/common/schedule.c index 05281d6..1ef5be9 100644 --- a/xen/common/schedule.c +++ b/xen/common/schedule.c @@ -1645,7 +1645,7 @@ static int cpu_schedule_up(unsigned int cpu) return 0; if ( idle_vcpu[cpu] == NULL ) - alloc_vcpu(idle_vcpu[0]->domain, cpu, cpu); + vcpu_create(idle_vcpu[0]->domain, cpu, cpu); else { struct vcpu *idle = idle_vcpu[cpu]; @@ -1817,7 +1817,7 @@ void __init scheduler_init(void) BUG_ON(IS_ERR(idle_domain)); idle_domain->vcpu = idle_vcpu; idle_domain->max_vcpus = nr_cpu_ids; - if ( alloc_vcpu(idle_domain, 0, 0) == NULL ) + if ( vcpu_create(idle_domain, 0, 0) == NULL ) BUG(); this_cpu(schedule_data).sched_priv = SCHED_OP(&ops, alloc_pdata, 0); BUG_ON(IS_ERR(this_cpu(schedule_data).sched_priv)); diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h index 5593495..6df6a58 100644 --- a/xen/include/xen/domain.h +++ b/xen/include/xen/domain.h @@ -13,7 +13,7 @@ typedef union { struct compat_vcpu_guest_context *cmp; } vcpu_guest_context_u __attribute__((__transparent_union__)); -struct vcpu *alloc_vcpu( +struct vcpu *vcpu_create( struct domain *d, unsigned int vcpu_id, unsigned int cpu_id); unsigned int dom0_max_vcpus(void); @@ -47,13 +47,13 @@ void free_pirq_struct(void *); /* * Initialise/destroy arch-specific details of a VCPU. - * - vcpu_initialise() is called after the basic generic fields of the + * - arch_vcpu_create() is called after the basic generic fields of the * VCPU structure are initialised. Many operations can be applied to the * VCPU at this point (e.g., vcpu_pause()). - * - vcpu_destroy() is called only if vcpu_initialise() previously succeeded. + * - arch_vcpu_destroy() is called only if arch_vcpu_create() previously succeeded. */ -int vcpu_initialise(struct vcpu *v); -void vcpu_destroy(struct vcpu *v); +int arch_vcpu_create(struct vcpu *v); +void arch_vcpu_destroy(struct vcpu *v); int map_vcpu_info(struct vcpu *v, unsigned long gfn, unsigned offset); void unmap_vcpu_info(struct vcpu *v); -- 2.1.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] xen/vcpu: Rename the common interfaces for consistency 2018-09-06 19:25 ` [PATCH 1/3] xen/vcpu: Rename the common interfaces for consistency Andrew Cooper @ 2018-09-07 9:52 ` Jan Beulich 2018-09-13 10:29 ` Roger Pau Monné 2018-09-17 1:08 ` Julien Grall 2 siblings, 0 replies; 16+ messages in thread From: Jan Beulich @ 2018-09-07 9:52 UTC (permalink / raw) To: Andrew Cooper Cc: Julien Grall, Stefano Stabellini, Wei Liu, Xen-devel, Roger Pau Monne >>> On 06.09.18 at 21:25, <andrew.cooper3@citrix.com> wrote: > The vcpu functions are far less consistent than the domain side of things, and > in particular, has vcpu_destroy() for architecture specific functionality. > > Perform the following renames: > > * alloc_vcpu => vcpu_create > * vcpu_initialise => arch_vcpu_create > * vcpu_destroy => arch_vcpu_destroy > > which makes the vcpu hierarchy consistent with the domain hierarchy. > > No functional change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Acked-by: Jan Beulich <jbeulich@suse.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] xen/vcpu: Rename the common interfaces for consistency 2018-09-06 19:25 ` [PATCH 1/3] xen/vcpu: Rename the common interfaces for consistency Andrew Cooper 2018-09-07 9:52 ` Jan Beulich @ 2018-09-13 10:29 ` Roger Pau Monné 2018-09-17 1:08 ` Julien Grall 2 siblings, 0 replies; 16+ messages in thread From: Roger Pau Monné @ 2018-09-13 10:29 UTC (permalink / raw) To: Andrew Cooper Cc: Julien Grall, Stefano Stabellini, Wei Liu, Jan Beulich, Xen-devel On Thu, Sep 06, 2018 at 08:25:32PM +0100, Andrew Cooper wrote: > The vcpu functions are far less consistent than the domain side of things, and > in particular, has vcpu_destroy() for architecture specific functionality. > > Perform the following renames: > > * alloc_vcpu => vcpu_create > * vcpu_initialise => arch_vcpu_create > * vcpu_destroy => arch_vcpu_destroy > > which makes the vcpu hierarchy consistent with the domain hierarchy. > > No functional change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> > diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h > index 5593495..6df6a58 100644 > --- a/xen/include/xen/domain.h > +++ b/xen/include/xen/domain.h > @@ -13,7 +13,7 @@ typedef union { > struct compat_vcpu_guest_context *cmp; > } vcpu_guest_context_u __attribute__((__transparent_union__)); > > -struct vcpu *alloc_vcpu( > +struct vcpu *vcpu_create( > struct domain *d, unsigned int vcpu_id, unsigned int cpu_id); > > unsigned int dom0_max_vcpus(void); > @@ -47,13 +47,13 @@ void free_pirq_struct(void *); > > /* > * Initialise/destroy arch-specific details of a VCPU. > - * - vcpu_initialise() is called after the basic generic fields of the > + * - arch_vcpu_create() is called after the basic generic fields of the > * VCPU structure are initialised. Many operations can be applied to the > * VCPU at this point (e.g., vcpu_pause()). > - * - vcpu_destroy() is called only if vcpu_initialise() previously succeeded. > + * - arch_vcpu_destroy() is called only if arch_vcpu_create() previously succeeded. Line is too long. Thanks, Roger. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] xen/vcpu: Rename the common interfaces for consistency 2018-09-06 19:25 ` [PATCH 1/3] xen/vcpu: Rename the common interfaces for consistency Andrew Cooper 2018-09-07 9:52 ` Jan Beulich 2018-09-13 10:29 ` Roger Pau Monné @ 2018-09-17 1:08 ` Julien Grall 2 siblings, 0 replies; 16+ messages in thread From: Julien Grall @ 2018-09-17 1:08 UTC (permalink / raw) To: Andrew Cooper, Xen-devel Cc: Stefano Stabellini, Wei Liu, Jan Beulich, Roger Pau Monné Hi, On 09/06/2018 08:25 PM, Andrew Cooper wrote: > The vcpu functions are far less consistent than the domain side of things, and > in particular, has vcpu_destroy() for architecture specific functionality. > > Perform the following renames: > > * alloc_vcpu => vcpu_create > * vcpu_initialise => arch_vcpu_create > * vcpu_destroy => arch_vcpu_destroy > > which makes the vcpu hierarchy consistent with the domain hierarchy. > > No functional change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Acked-by: Julien Grall <julien.grall@arm.com> Cheers, > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Wei Liu <wei.liu2@citrix.com> > CC: Roger Pau Monné <roger.pau@citrix.com> > CC: Stefano Stabellini <sstabellini@kernel.org> > CC: Julien Grall <julien.grall@arm.com> > --- > xen/arch/arm/domain.c | 6 +++--- > xen/arch/arm/domain_build.c | 4 ++-- > xen/arch/x86/dom0_build.c | 2 +- > xen/arch/x86/domain.c | 4 ++-- > xen/common/domain.c | 6 +++--- > xen/common/domctl.c | 2 +- > xen/common/schedule.c | 4 ++-- > xen/include/xen/domain.h | 10 +++++----- > 8 files changed, 19 insertions(+), 19 deletions(-) > > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c > index 4baecc2..feebbf5 100644 > --- a/xen/arch/arm/domain.c > +++ b/xen/arch/arm/domain.c > @@ -538,7 +538,7 @@ void free_vcpu_struct(struct vcpu *v) > free_xenheap_pages(v, get_order_from_bytes(sizeof(*v))); > } > > -int vcpu_initialise(struct vcpu *v) > +int arch_vcpu_create(struct vcpu *v) > { > int rc = 0; > > @@ -583,11 +583,11 @@ int vcpu_initialise(struct vcpu *v) > return rc; > > fail: > - vcpu_destroy(v); > + arch_vcpu_destroy(v); > return rc; > } > > -void vcpu_destroy(struct vcpu *v) > +void arch_vcpu_destroy(struct vcpu *v) > { > vcpu_timer_destroy(v); > vcpu_vgic_free(v); > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 2a383c8..5057ad8 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -74,7 +74,7 @@ unsigned int __init dom0_max_vcpus(void) > > struct vcpu *__init alloc_dom0_vcpu0(struct domain *dom0) > { > - return alloc_vcpu(dom0, 0, 0); > + return vcpu_create(dom0, 0, 0); > } > > static unsigned int __init get_11_allocation_size(paddr_t size) > @@ -2232,7 +2232,7 @@ int __init construct_dom0(struct domain *d) > for ( i = 1, cpu = 0; i < d->max_vcpus; i++ ) > { > cpu = cpumask_cycle(cpu, &cpu_online_map); > - if ( alloc_vcpu(d, i, cpu) == NULL ) > + if ( vcpu_create(d, i, cpu) == NULL ) > { > printk("Failed to allocate dom0 vcpu %d on pcpu %d\n", i, cpu); > break; > diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c > index 423fdec..86eb7db 100644 > --- a/xen/arch/x86/dom0_build.c > +++ b/xen/arch/x86/dom0_build.c > @@ -134,7 +134,7 @@ struct vcpu *__init dom0_setup_vcpu(struct domain *d, > unsigned int prev_cpu) > { > unsigned int cpu = cpumask_cycle(prev_cpu, &dom0_cpus); > - struct vcpu *v = alloc_vcpu(d, vcpu_id, cpu); > + struct vcpu *v = vcpu_create(d, vcpu_id, cpu); > > if ( v ) > { > diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c > index 313ebb3..d67a047 100644 > --- a/xen/arch/x86/domain.c > +++ b/xen/arch/x86/domain.c > @@ -322,7 +322,7 @@ void free_vcpu_struct(struct vcpu *v) > free_xenheap_page(v); > } > > -int vcpu_initialise(struct vcpu *v) > +int arch_vcpu_create(struct vcpu *v) > { > struct domain *d = v->domain; > int rc; > @@ -382,7 +382,7 @@ int vcpu_initialise(struct vcpu *v) > return rc; > } > > -void vcpu_destroy(struct vcpu *v) > +void arch_vcpu_destroy(struct vcpu *v) > { > xfree(v->arch.vm_event); > v->arch.vm_event = NULL; > diff --git a/xen/common/domain.c b/xen/common/domain.c > index 78c450e..14c8d00 100644 > --- a/xen/common/domain.c > +++ b/xen/common/domain.c > @@ -123,7 +123,7 @@ static void vcpu_info_reset(struct vcpu *v) > v->vcpu_info_mfn = INVALID_MFN; > } > > -struct vcpu *alloc_vcpu( > +struct vcpu *vcpu_create( > struct domain *d, unsigned int vcpu_id, unsigned int cpu_id) > { > struct vcpu *v; > @@ -165,7 +165,7 @@ struct vcpu *alloc_vcpu( > if ( sched_init_vcpu(v, cpu_id) != 0 ) > goto fail_wq; > > - if ( vcpu_initialise(v) != 0 ) > + if ( arch_vcpu_create(v) != 0 ) > { > sched_destroy_vcpu(v); > fail_wq: > @@ -870,7 +870,7 @@ static void complete_domain_destroy(struct rcu_head *head) > if ( (v = d->vcpu[i]) == NULL ) > continue; > tasklet_kill(&v->continue_hypercall_tasklet); > - vcpu_destroy(v); > + arch_vcpu_destroy(v); > sched_destroy_vcpu(v); > destroy_waitqueue_vcpu(v); > } > diff --git a/xen/common/domctl.c b/xen/common/domctl.c > index ed047b7..2facbdb 100644 > --- a/xen/common/domctl.c > +++ b/xen/common/domctl.c > @@ -571,7 +571,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) > cpumask_any(online) : > cpumask_cycle(d->vcpu[i-1]->processor, online); > > - if ( alloc_vcpu(d, i, cpu) == NULL ) > + if ( vcpu_create(d, i, cpu) == NULL ) > goto maxvcpu_out; > } > > diff --git a/xen/common/schedule.c b/xen/common/schedule.c > index 05281d6..1ef5be9 100644 > --- a/xen/common/schedule.c > +++ b/xen/common/schedule.c > @@ -1645,7 +1645,7 @@ static int cpu_schedule_up(unsigned int cpu) > return 0; > > if ( idle_vcpu[cpu] == NULL ) > - alloc_vcpu(idle_vcpu[0]->domain, cpu, cpu); > + vcpu_create(idle_vcpu[0]->domain, cpu, cpu); > else > { > struct vcpu *idle = idle_vcpu[cpu]; > @@ -1817,7 +1817,7 @@ void __init scheduler_init(void) > BUG_ON(IS_ERR(idle_domain)); > idle_domain->vcpu = idle_vcpu; > idle_domain->max_vcpus = nr_cpu_ids; > - if ( alloc_vcpu(idle_domain, 0, 0) == NULL ) > + if ( vcpu_create(idle_domain, 0, 0) == NULL ) > BUG(); > this_cpu(schedule_data).sched_priv = SCHED_OP(&ops, alloc_pdata, 0); > BUG_ON(IS_ERR(this_cpu(schedule_data).sched_priv)); > diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h > index 5593495..6df6a58 100644 > --- a/xen/include/xen/domain.h > +++ b/xen/include/xen/domain.h > @@ -13,7 +13,7 @@ typedef union { > struct compat_vcpu_guest_context *cmp; > } vcpu_guest_context_u __attribute__((__transparent_union__)); > > -struct vcpu *alloc_vcpu( > +struct vcpu *vcpu_create( > struct domain *d, unsigned int vcpu_id, unsigned int cpu_id); > > unsigned int dom0_max_vcpus(void); > @@ -47,13 +47,13 @@ void free_pirq_struct(void *); > > /* > * Initialise/destroy arch-specific details of a VCPU. > - * - vcpu_initialise() is called after the basic generic fields of the > + * - arch_vcpu_create() is called after the basic generic fields of the > * VCPU structure are initialised. Many operations can be applied to the > * VCPU at this point (e.g., vcpu_pause()). > - * - vcpu_destroy() is called only if vcpu_initialise() previously succeeded. > + * - arch_vcpu_destroy() is called only if arch_vcpu_create() previously succeeded. > */ > -int vcpu_initialise(struct vcpu *v); > -void vcpu_destroy(struct vcpu *v); > +int arch_vcpu_create(struct vcpu *v); > +void arch_vcpu_destroy(struct vcpu *v); > > int map_vcpu_info(struct vcpu *v, unsigned long gfn, unsigned offset); > void unmap_vcpu_info(struct vcpu *v); > -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/3] xen/vcpu: Introduce vcpu_destroy() 2018-09-06 19:25 [PATCH 0/3] xen: Improvements to the vcpu create/destroy paths Andrew Cooper 2018-09-06 19:25 ` [PATCH 1/3] xen/vcpu: Rename the common interfaces for consistency Andrew Cooper @ 2018-09-06 19:25 ` Andrew Cooper 2018-09-07 9:53 ` Jan Beulich 2018-09-13 10:32 ` Roger Pau Monné 2018-09-06 19:25 ` [PATCH 3/3] xen/vcpu: Rework sanity checks in vcpu_create() Andrew Cooper 2 siblings, 2 replies; 16+ messages in thread From: Andrew Cooper @ 2018-09-06 19:25 UTC (permalink / raw) To: Xen-devel Cc: Stefano Stabellini, Wei Liu, Andrew Cooper, Julien Grall, Jan Beulich, Roger Pau Monné Like _domain_destroy(), this will eventually idempotently free all parts of a struct vcpu. While breaking apart the failure path of vcpu_create(), rework the codeflow to be in a line at the end of the function for clarity. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Wei Liu <wei.liu2@citrix.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Julien Grall <julien.grall@arm.com> --- xen/common/domain.c | 42 ++++++++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 20 deletions(-) diff --git a/xen/common/domain.c b/xen/common/domain.c index 14c8d00..a9df589 100644 --- a/xen/common/domain.c +++ b/xen/common/domain.c @@ -123,6 +123,16 @@ static void vcpu_info_reset(struct vcpu *v) v->vcpu_info_mfn = INVALID_MFN; } +static void vcpu_destroy(struct vcpu *v) +{ + free_cpumask_var(v->cpu_hard_affinity); + free_cpumask_var(v->cpu_hard_affinity_tmp); + free_cpumask_var(v->cpu_hard_affinity_saved); + free_cpumask_var(v->cpu_soft_affinity); + + free_vcpu_struct(v); +} + struct vcpu *vcpu_create( struct domain *d, unsigned int vcpu_id, unsigned int cpu_id) { @@ -147,7 +157,7 @@ struct vcpu *vcpu_create( !zalloc_cpumask_var(&v->cpu_hard_affinity_tmp) || !zalloc_cpumask_var(&v->cpu_hard_affinity_saved) || !zalloc_cpumask_var(&v->cpu_soft_affinity) ) - goto fail_free; + goto fail; if ( is_idle_domain(d) ) { @@ -166,18 +176,7 @@ struct vcpu *vcpu_create( goto fail_wq; if ( arch_vcpu_create(v) != 0 ) - { - sched_destroy_vcpu(v); - fail_wq: - destroy_waitqueue_vcpu(v); - fail_free: - free_cpumask_var(v->cpu_hard_affinity); - free_cpumask_var(v->cpu_hard_affinity_tmp); - free_cpumask_var(v->cpu_hard_affinity_saved); - free_cpumask_var(v->cpu_soft_affinity); - free_vcpu_struct(v); - return NULL; - } + goto fail_sched; d->vcpu[vcpu_id] = v; if ( vcpu_id != 0 ) @@ -197,6 +196,15 @@ struct vcpu *vcpu_create( domain_update_node_affinity(d); return v; + + fail_sched: + sched_destroy_vcpu(v); + fail_wq: + destroy_waitqueue_vcpu(v); + fail: + vcpu_destroy(v); + + return NULL; } static int late_hwdom_init(struct domain *d) @@ -898,13 +906,7 @@ static void complete_domain_destroy(struct rcu_head *head) for ( i = d->max_vcpus - 1; i >= 0; i-- ) if ( (v = d->vcpu[i]) != NULL ) - { - free_cpumask_var(v->cpu_hard_affinity); - free_cpumask_var(v->cpu_hard_affinity_tmp); - free_cpumask_var(v->cpu_hard_affinity_saved); - free_cpumask_var(v->cpu_soft_affinity); - free_vcpu_struct(v); - } + vcpu_destroy(v); if ( d->target != NULL ) put_domain(d->target); -- 2.1.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] xen/vcpu: Introduce vcpu_destroy() 2018-09-06 19:25 ` [PATCH 2/3] xen/vcpu: Introduce vcpu_destroy() Andrew Cooper @ 2018-09-07 9:53 ` Jan Beulich 2018-09-13 10:32 ` Roger Pau Monné 1 sibling, 0 replies; 16+ messages in thread From: Jan Beulich @ 2018-09-07 9:53 UTC (permalink / raw) To: Andrew Cooper Cc: Julien Grall, Stefano Stabellini, Wei Liu, Xen-devel, Roger Pau Monne >>> On 06.09.18 at 21:25, <andrew.cooper3@citrix.com> wrote: > Like _domain_destroy(), this will eventually idempotently free all parts of a > struct vcpu. > > While breaking apart the failure path of vcpu_create(), rework the codeflow to > be in a line at the end of the function for clarity. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] xen/vcpu: Introduce vcpu_destroy() 2018-09-06 19:25 ` [PATCH 2/3] xen/vcpu: Introduce vcpu_destroy() Andrew Cooper 2018-09-07 9:53 ` Jan Beulich @ 2018-09-13 10:32 ` Roger Pau Monné 1 sibling, 0 replies; 16+ messages in thread From: Roger Pau Monné @ 2018-09-13 10:32 UTC (permalink / raw) To: Andrew Cooper Cc: Julien Grall, Stefano Stabellini, Wei Liu, Jan Beulich, Xen-devel On Thu, Sep 06, 2018 at 08:25:33PM +0100, Andrew Cooper wrote: > Like _domain_destroy(), this will eventually idempotently free all parts of a > struct vcpu. > > While breaking apart the failure path of vcpu_create(), rework the codeflow to > be in a line at the end of the function for clarity. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 3/3] xen/vcpu: Rework sanity checks in vcpu_create() 2018-09-06 19:25 [PATCH 0/3] xen: Improvements to the vcpu create/destroy paths Andrew Cooper 2018-09-06 19:25 ` [PATCH 1/3] xen/vcpu: Rename the common interfaces for consistency Andrew Cooper 2018-09-06 19:25 ` [PATCH 2/3] xen/vcpu: Introduce vcpu_destroy() Andrew Cooper @ 2018-09-06 19:25 ` Andrew Cooper 2018-09-06 20:07 ` Jason Andryuk ` (2 more replies) 2 siblings, 3 replies; 16+ messages in thread From: Andrew Cooper @ 2018-09-06 19:25 UTC (permalink / raw) To: Xen-devel Cc: Stefano Stabellini, Wei Liu, Andrew Cooper, Julien Grall, Jan Beulich, Roger Pau Monné Poisoning idle_vcpu[0] with the sanity debug value isn't actually a clever idea, because it passes a NULL pointer check but isn't a usable vcpu. It is also the reason for the (!is_idle_domain(d) || vcpu_id) part of the existing sanity BUG_ON(). Now that d->max_vcpus is appropriately set up before vcpu_create() is called, we can properly range check the requested vcpu_id. Drop the BUG_ON() and replace it with code which is runtime safe but non-fatal. While v0 must be the first allocated vcpu for for_each_vcpu() to work, it isn't a requirement for the threading the vcpu into the linked list, so update the threading code to be more generic, and add a comment explaining why we need to search for prev_id. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Wei Liu <wei.liu2@citrix.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Julien Grall <julien.grall@arm.com> --- xen/arch/arm/setup.c | 1 - xen/arch/x86/setup.c | 1 - xen/common/domain.c | 32 ++++++++++++++++++++++++++++---- 3 files changed, 28 insertions(+), 6 deletions(-) diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index 01aaaab..d06ac40 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -704,7 +704,6 @@ void __init start_xen(unsigned long boot_phys_offset, set_processor_id(0); /* needed early, for smp_processor_id() */ set_current((struct vcpu *)0xfffff000); /* debug sanity */ - idle_vcpu[0] = current; setup_virtual_regions(NULL, NULL); /* Initialize traps early allow us to get backtrace when an error occurred */ diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index a2f22a1..5e1e8ae 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -691,7 +691,6 @@ void __init noreturn __start_xen(unsigned long mbi_p) set_processor_id(0); set_current(INVALID_VCPU); /* debug sanity. */ - idle_vcpu[0] = current; init_shadow_spec_ctrl_state(); percpu_init_areas(); diff --git a/xen/common/domain.c b/xen/common/domain.c index a9df589..d23b54a 100644 --- a/xen/common/domain.c +++ b/xen/common/domain.c @@ -138,7 +138,19 @@ struct vcpu *vcpu_create( { struct vcpu *v; - BUG_ON((!is_idle_domain(d) || vcpu_id) && d->vcpu[vcpu_id]); + /* + * Sanity check some input expectations: + * - d->max_vcpus and d->vcpu[] should be set up + * - vcpu_id should be bounded by d->max_vcpus + * - v0 must be the first-allocated vcpu + * - No previous vcpu with this id should be allocated + */ + if ( !d->max_vcpus || !d->vcpu || vcpu_id >= d->max_vcpus || + (vcpu_id > 0 && !d->vcpu[0]) || d->vcpu[vcpu_id] ) + { + ASSERT_UNREACHABLE(); + return NULL; + } if ( (v = alloc_vcpu_struct()) == NULL ) return NULL; @@ -178,15 +190,27 @@ struct vcpu *vcpu_create( if ( arch_vcpu_create(v) != 0 ) goto fail_sched; + /* Insert the vcpu into the domain's vcpu list. */ d->vcpu[vcpu_id] = v; if ( vcpu_id != 0 ) { int prev_id = v->vcpu_id - 1; + + /* + * Look for the previously allocated vcpu, and splice into the + * next_in_list single linked list. + * + * All domains other than IDLE have tightly packed vcpu_id's. IDLE + * vcpu_id's are derived from hardware CPU id's and can be sparse. + */ while ( (prev_id >= 0) && (d->vcpu[prev_id] == NULL) ) prev_id--; - BUG_ON(prev_id < 0); - v->next_in_list = d->vcpu[prev_id]->next_in_list; - d->vcpu[prev_id]->next_in_list = v; + + if ( prev_id >= 0 ) + { + v->next_in_list = d->vcpu[prev_id]->next_in_list; + d->vcpu[prev_id]->next_in_list = v; + } } /* Must be called after making new vcpu visible to for_each_vcpu(). */ -- 2.1.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] xen/vcpu: Rework sanity checks in vcpu_create() 2018-09-06 19:25 ` [PATCH 3/3] xen/vcpu: Rework sanity checks in vcpu_create() Andrew Cooper @ 2018-09-06 20:07 ` Jason Andryuk 2018-09-06 23:01 ` Andrew Cooper 2018-09-07 10:15 ` Jan Beulich 2018-09-11 16:46 ` [PATCH v2] " Andrew Cooper 2 siblings, 1 reply; 16+ messages in thread From: Jason Andryuk @ 2018-09-06 20:07 UTC (permalink / raw) To: Andrew Cooper Cc: sstabellini, Wei Liu, xen-devel, julien.grall, Jan Beulich, roger.pau On Thu, Sep 6, 2018 at 3:28 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > Poisoning idle_vcpu[0] with the sanity debug value isn't actually a clever > idea, because it passes a NULL pointer check but isn't a usable vcpu. It is > also the reason for the (!is_idle_domain(d) || vcpu_id) part of the existing > sanity BUG_ON(). > > Now that d->max_vcpus is appropriately set up before vcpu_create() is called, > we can properly range check the requested vcpu_id. Drop the BUG_ON() and > replace it with code which is runtime safe but non-fatal. > > While v0 must be the first allocated vcpu for for_each_vcpu() to work, it > isn't a requirement for the threading the vcpu into the linked list, so update > the threading code to be more generic, and add a comment explaining why we > need to search for prev_id. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Wei Liu <wei.liu2@citrix.com> > CC: Roger Pau Monné <roger.pau@citrix.com> > CC: Stefano Stabellini <sstabellini@kernel.org> > CC: Julien Grall <julien.grall@arm.com> > --- > xen/arch/arm/setup.c | 1 - > xen/arch/x86/setup.c | 1 - > xen/common/domain.c | 32 ++++++++++++++++++++++++++++---- > 3 files changed, 28 insertions(+), 6 deletions(-) > > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > index 01aaaab..d06ac40 100644 > --- a/xen/arch/arm/setup.c > +++ b/xen/arch/arm/setup.c > @@ -704,7 +704,6 @@ void __init start_xen(unsigned long boot_phys_offset, > set_processor_id(0); /* needed early, for smp_processor_id() */ > > set_current((struct vcpu *)0xfffff000); /* debug sanity */ > - idle_vcpu[0] = current; > > setup_virtual_regions(NULL, NULL); > /* Initialize traps early allow us to get backtrace when an error occurred */ > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c > index a2f22a1..5e1e8ae 100644 > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -691,7 +691,6 @@ void __init noreturn __start_xen(unsigned long mbi_p) > > set_processor_id(0); > set_current(INVALID_VCPU); /* debug sanity. */ > - idle_vcpu[0] = current; xen/arch/x86/tboot.c:tboot_shutdown() has: if ( idle_vcpu[0] != INVALID_VCPU ) write_ptbase(idle_vcpu[0]); With your change, this should be update to check for non-NULL. Regards, Jason _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] xen/vcpu: Rework sanity checks in vcpu_create() 2018-09-06 20:07 ` Jason Andryuk @ 2018-09-06 23:01 ` Andrew Cooper 0 siblings, 0 replies; 16+ messages in thread From: Andrew Cooper @ 2018-09-06 23:01 UTC (permalink / raw) To: Jason Andryuk Cc: sstabellini, Wei Liu, xen-devel, julien.grall, Jan Beulich, roger.pau On 06/09/18 21:07, Jason Andryuk wrote: > On Thu, Sep 6, 2018 at 3:28 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote: >> Poisoning idle_vcpu[0] with the sanity debug value isn't actually a clever >> idea, because it passes a NULL pointer check but isn't a usable vcpu. It is >> also the reason for the (!is_idle_domain(d) || vcpu_id) part of the existing >> sanity BUG_ON(). >> >> Now that d->max_vcpus is appropriately set up before vcpu_create() is called, >> we can properly range check the requested vcpu_id. Drop the BUG_ON() and >> replace it with code which is runtime safe but non-fatal. >> >> While v0 must be the first allocated vcpu for for_each_vcpu() to work, it >> isn't a requirement for the threading the vcpu into the linked list, so update >> the threading code to be more generic, and add a comment explaining why we >> need to search for prev_id. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> --- >> CC: Jan Beulich <JBeulich@suse.com> >> CC: Wei Liu <wei.liu2@citrix.com> >> CC: Roger Pau Monné <roger.pau@citrix.com> >> CC: Stefano Stabellini <sstabellini@kernel.org> >> CC: Julien Grall <julien.grall@arm.com> >> --- >> xen/arch/arm/setup.c | 1 - >> xen/arch/x86/setup.c | 1 - >> xen/common/domain.c | 32 ++++++++++++++++++++++++++++---- >> 3 files changed, 28 insertions(+), 6 deletions(-) >> >> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c >> index 01aaaab..d06ac40 100644 >> --- a/xen/arch/arm/setup.c >> +++ b/xen/arch/arm/setup.c >> @@ -704,7 +704,6 @@ void __init start_xen(unsigned long boot_phys_offset, >> set_processor_id(0); /* needed early, for smp_processor_id() */ >> >> set_current((struct vcpu *)0xfffff000); /* debug sanity */ >> - idle_vcpu[0] = current; >> >> setup_virtual_regions(NULL, NULL); >> /* Initialize traps early allow us to get backtrace when an error occurred */ >> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c >> index a2f22a1..5e1e8ae 100644 >> --- a/xen/arch/x86/setup.c >> +++ b/xen/arch/x86/setup.c >> @@ -691,7 +691,6 @@ void __init noreturn __start_xen(unsigned long mbi_p) >> >> set_processor_id(0); >> set_current(INVALID_VCPU); /* debug sanity. */ >> - idle_vcpu[0] = current; > xen/arch/x86/tboot.c:tboot_shutdown() has: > if ( idle_vcpu[0] != INVALID_VCPU ) > write_ptbase(idle_vcpu[0]); > > With your change, this should be update to check for non-NULL. Oh - very good catch. Thanks. Looking at the code, ideally it would change to write_cr3(idle_pg_table), but that isn't going to include the additional CR4 changes. Leaving this in this form (with a NULL) check is probably best. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] xen/vcpu: Rework sanity checks in vcpu_create() 2018-09-06 19:25 ` [PATCH 3/3] xen/vcpu: Rework sanity checks in vcpu_create() Andrew Cooper 2018-09-06 20:07 ` Jason Andryuk @ 2018-09-07 10:15 ` Jan Beulich 2018-09-07 19:17 ` Andrew Cooper 2018-09-11 16:46 ` [PATCH v2] " Andrew Cooper 2 siblings, 1 reply; 16+ messages in thread From: Jan Beulich @ 2018-09-07 10:15 UTC (permalink / raw) To: Andrew Cooper Cc: Julien Grall, Stefano Stabellini, Wei Liu, Xen-devel, Roger Pau Monne >>> On 06.09.18 at 21:25, <andrew.cooper3@citrix.com> wrote: > While v0 must be the first allocated vcpu for for_each_vcpu() to work, it > isn't a requirement for the threading the vcpu into the linked list, so update > the threading code to be more generic, and add a comment explaining why we > need to search for prev_id. I'm afraid I can't bring this in line with the code change: > @@ -178,15 +190,27 @@ struct vcpu *vcpu_create( > if ( arch_vcpu_create(v) != 0 ) > goto fail_sched; > > + /* Insert the vcpu into the domain's vcpu list. */ > d->vcpu[vcpu_id] = v; > if ( vcpu_id != 0 ) There still is this conditional, and you don't add any else to it. Hence afaics if vCPU 0 was created after vCPU 1, vCPU 0's next_in_list would not be made point to vCPU 1. That's not what I'd call "more generic". But the question is what use the next_in_list field is in the first place, when the entries there are sorted by ID anyway: Why can't we simply use v->domain->vcpu[] instead? In the common case v->domain->vcpu[v->vcpu_id+1] is not going to be NULL anyway, and I don't think for_each_vcpu() would become that much more complicated that way. > { > int prev_id = v->vcpu_id - 1; > + > + /* > + * Look for the previously allocated vcpu, and splice into the > + * next_in_list single linked list. I'm also not very happy about the use of "previously" here: This (to me as a non-native speaker) in no way means "the one with the next lowest ID". Jan > + * All domains other than IDLE have tightly packed vcpu_id's. IDLE > + * vcpu_id's are derived from hardware CPU id's and can be sparse. > + */ > while ( (prev_id >= 0) && (d->vcpu[prev_id] == NULL) ) > prev_id--; > - BUG_ON(prev_id < 0); > - v->next_in_list = d->vcpu[prev_id]->next_in_list; > - d->vcpu[prev_id]->next_in_list = v; > + > + if ( prev_id >= 0 ) > + { > + v->next_in_list = d->vcpu[prev_id]->next_in_list; > + d->vcpu[prev_id]->next_in_list = v; > + } > } > > /* Must be called after making new vcpu visible to for_each_vcpu(). */ > -- > 2.1.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] xen/vcpu: Rework sanity checks in vcpu_create() 2018-09-07 10:15 ` Jan Beulich @ 2018-09-07 19:17 ` Andrew Cooper 0 siblings, 0 replies; 16+ messages in thread From: Andrew Cooper @ 2018-09-07 19:17 UTC (permalink / raw) To: Jan Beulich Cc: Julien Grall, Stefano Stabellini, Wei Liu, Xen-devel, Roger Pau Monne On 07/09/18 11:15, Jan Beulich wrote: >>>> On 06.09.18 at 21:25, <andrew.cooper3@citrix.com> wrote: >> While v0 must be the first allocated vcpu for for_each_vcpu() to work, it >> isn't a requirement for the threading the vcpu into the linked list, so update >> the threading code to be more generic, and add a comment explaining why we >> need to search for prev_id. > I'm afraid I can't bring this in line with the code change: > >> @@ -178,15 +190,27 @@ struct vcpu *vcpu_create( >> if ( arch_vcpu_create(v) != 0 ) >> goto fail_sched; >> >> + /* Insert the vcpu into the domain's vcpu list. */ >> d->vcpu[vcpu_id] = v; >> if ( vcpu_id != 0 ) > There still is this conditional, and you don't add any else to it. Hence > afaics if vCPU 0 was created after vCPU 1, vCPU 0's next_in_list > would not be made point to vCPU 1. That's not what I'd call "more > generic". You are completely correct. I don't know what I was thinking when saying that this is more generic. I suspect it also means that, depending on hardware CPU numbering, for_each_vcpu(idle) may not work, but perhaps that is not an issue as the idle domains pointer is actually private to scheduler_init(). It is available from either current->domain, or idle_vcpu[$X]->domain, but I think its reasonable to expect that we never use for_each_vcpu() over the idle domain. > But the question is what use the next_in_list field is in the first place, > when the entries there are sorted by ID anyway: Why can't we > simply use v->domain->vcpu[] instead? In the common case > v->domain->vcpu[v->vcpu_id+1] is not going to be NULL anyway, > and I don't think for_each_vcpu() would become that much more > complicated that way. It would, because every iteration you'd need a check of v->vcpu_id against d->max_vcpus to avoid walking off the end of d->vcpu[] > >> { >> int prev_id = v->vcpu_id - 1; >> + >> + /* >> + * Look for the previously allocated vcpu, and splice into the >> + * next_in_list single linked list. > I'm also not very happy about the use of "previously" here: This (to > me as a non-native speaker) in no way means "the one with the next > lowest ID". Given the simplifying assumption of not the idle domain, the setting up of the single linked list can be made much more concise, as we know the IDs are packed and allocated in ascending order. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2] xen/vcpu: Rework sanity checks in vcpu_create() 2018-09-06 19:25 ` [PATCH 3/3] xen/vcpu: Rework sanity checks in vcpu_create() Andrew Cooper 2018-09-06 20:07 ` Jason Andryuk 2018-09-07 10:15 ` Jan Beulich @ 2018-09-11 16:46 ` Andrew Cooper 2018-09-12 11:38 ` Jason Andryuk 2018-09-12 15:00 ` Jan Beulich 2 siblings, 2 replies; 16+ messages in thread From: Andrew Cooper @ 2018-09-11 16:46 UTC (permalink / raw) To: Xen-devel Cc: Stefano Stabellini, Wei Liu, Jason Andryuk, Andrew Cooper, Julien Grall, Jan Beulich, Roger Pau Monné Poisoning idle_vcpu[0] with the sanity debug value isn't actually a clever idea, because it passes a NULL pointer check but isn't a usable vcpu. It is also the reason for the (!is_idle_domain(d) || vcpu_id) part of the existing sanity BUG_ON(). Now that d->max_vcpus is appropriately set up before vcpu_create() is called, we can properly range check the requested vcpu_id. Drop the BUG_ON() and replace it with code which is runtime safe but non-fatal. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Wei Liu <wei.liu2@citrix.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Julien Grall <julien.grall@arm.com> CC: Jason Andryuk <jandryuk@gmail.com> v2: * Fix the tboot check following the un-poisioning of idle_vcpu[0] * Exclude the idle domain from the next_in_list list, and vastly simplify the linking logic. --- xen/arch/arm/setup.c | 1 - xen/arch/x86/setup.c | 1 - xen/arch/x86/tboot.c | 2 +- xen/common/domain.c | 28 ++++++++++++++++++---------- 4 files changed, 19 insertions(+), 13 deletions(-) diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index ea2495a..a80032c 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -704,7 +704,6 @@ void __init start_xen(unsigned long boot_phys_offset, set_processor_id(0); /* needed early, for smp_processor_id() */ set_current((struct vcpu *)0xfffff000); /* debug sanity */ - idle_vcpu[0] = current; setup_virtual_regions(NULL, NULL); /* Initialize traps early allow us to get backtrace when an error occurred */ diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index 2fbf7d5..2081592 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -691,7 +691,6 @@ void __init noreturn __start_xen(unsigned long mbi_p) set_processor_id(0); set_current(INVALID_VCPU); /* debug sanity. */ - idle_vcpu[0] = current; init_shadow_spec_ctrl_state(); percpu_init_areas(); diff --git a/xen/arch/x86/tboot.c b/xen/arch/x86/tboot.c index f3fdee4..fff00bf 100644 --- a/xen/arch/x86/tboot.c +++ b/xen/arch/x86/tboot.c @@ -395,7 +395,7 @@ void tboot_shutdown(uint32_t shutdown_type) * During early boot, we can be called by panic before idle_vcpu[0] is * setup, but in that case we don't need to change page tables. */ - if ( idle_vcpu[0] != INVALID_VCPU ) + if ( idle_vcpu[0] ) write_ptbase(idle_vcpu[0]); ((void(*)(void))(unsigned long)g_tboot_shared->shutdown_entry)(); diff --git a/xen/common/domain.c b/xen/common/domain.c index 66054b3..a0d950c 100644 --- a/xen/common/domain.c +++ b/xen/common/domain.c @@ -138,7 +138,21 @@ struct vcpu *vcpu_create( { struct vcpu *v; - BUG_ON((!is_idle_domain(d) || vcpu_id) && d->vcpu[vcpu_id]); + /* + * Sanity check some input expectations: + * - d->max_vcpus and d->vcpu[] should be set up + * - vcpu_id should be bounded by d->max_vcpus + * - Vcpus should be tightly packed and allocated in ascending order + * (except for the idle domain). + * - No previous vcpu with this id should be allocated + */ + if ( !d->max_vcpus || !d->vcpu || vcpu_id >= d->max_vcpus || + (!is_idle_domain(d) && vcpu_id && !d->vcpu[vcpu_id - 1]) || + d->vcpu[vcpu_id] ) + { + ASSERT_UNREACHABLE(); + return NULL; + } if ( (v = alloc_vcpu_struct()) == NULL ) return NULL; @@ -178,16 +192,10 @@ struct vcpu *vcpu_create( if ( arch_vcpu_create(v) != 0 ) goto fail_sched; + /* Insert the vcpu into the domain's vcpu list. */ d->vcpu[vcpu_id] = v; - if ( vcpu_id != 0 ) - { - int prev_id = v->vcpu_id - 1; - while ( (prev_id >= 0) && (d->vcpu[prev_id] == NULL) ) - prev_id--; - BUG_ON(prev_id < 0); - v->next_in_list = d->vcpu[prev_id]->next_in_list; - d->vcpu[prev_id]->next_in_list = v; - } + if ( !is_idle_domain(d) && vcpu_id > 0 ) + d->vcpu[vcpu_id - 1]->next_in_list = v; /* Must be called after making new vcpu visible to for_each_vcpu(). */ vcpu_check_shutdown(v); -- 2.1.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2] xen/vcpu: Rework sanity checks in vcpu_create() 2018-09-11 16:46 ` [PATCH v2] " Andrew Cooper @ 2018-09-12 11:38 ` Jason Andryuk 2018-09-12 15:00 ` Jan Beulich 1 sibling, 0 replies; 16+ messages in thread From: Jason Andryuk @ 2018-09-12 11:38 UTC (permalink / raw) To: Andrew Cooper Cc: sstabellini, Wei Liu, xen-devel, julien.grall, Jan Beulich, roger.pau On Tue, Sep 11, 2018 at 12:48 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > Poisoning idle_vcpu[0] with the sanity debug value isn't actually a clever > idea, because it passes a NULL pointer check but isn't a usable vcpu. It is > also the reason for the (!is_idle_domain(d) || vcpu_id) part of the existing > sanity BUG_ON(). > > Now that d->max_vcpus is appropriately set up before vcpu_create() is called, > we can properly range check the requested vcpu_id. Drop the BUG_ON() and > replace it with code which is runtime safe but non-fatal. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Wei Liu <wei.liu2@citrix.com> > CC: Roger Pau Monné <roger.pau@citrix.com> > CC: Stefano Stabellini <sstabellini@kernel.org> > CC: Julien Grall <julien.grall@arm.com> > CC: Jason Andryuk <jandryuk@gmail.com> > > v2: > * Fix the tboot check following the un-poisioning of idle_vcpu[0] > * Exclude the idle domain from the next_in_list list, and vastly simplify the > linking logic. Reviewed-by: Jason Andryuk <jandryuk@gmail.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] xen/vcpu: Rework sanity checks in vcpu_create() 2018-09-11 16:46 ` [PATCH v2] " Andrew Cooper 2018-09-12 11:38 ` Jason Andryuk @ 2018-09-12 15:00 ` Jan Beulich 1 sibling, 0 replies; 16+ messages in thread From: Jan Beulich @ 2018-09-12 15:00 UTC (permalink / raw) To: Andrew Cooper Cc: Stefano Stabellini, Wei Liu, Jason Andryuk, Xen-devel, Julien Grall, Roger Pau Monne >>> On 11.09.18 at 18:46, <andrew.cooper3@citrix.com> wrote: > Poisoning idle_vcpu[0] with the sanity debug value isn't actually a clever > idea, because it passes a NULL pointer check but isn't a usable vcpu. It is > also the reason for the (!is_idle_domain(d) || vcpu_id) part of the existing > sanity BUG_ON(). But you completely discount the intended effect of this poisoning: During early boot, a NULL deref is liable to not fault, while a deref of INVALID_VCPU is always going to (on x86 at least). > --- a/xen/common/domain.c > +++ b/xen/common/domain.c > @@ -138,7 +138,21 @@ struct vcpu *vcpu_create( This patch does not look to be based on current staging. > { > struct vcpu *v; > > - BUG_ON((!is_idle_domain(d) || vcpu_id) && d->vcpu[vcpu_id]); > + /* > + * Sanity check some input expectations: > + * - d->max_vcpus and d->vcpu[] should be set up > + * - vcpu_id should be bounded by d->max_vcpus > + * - Vcpus should be tightly packed and allocated in ascending order > + * (except for the idle domain). > + * - No previous vcpu with this id should be allocated > + */ > + if ( !d->max_vcpus || !d->vcpu || vcpu_id >= d->max_vcpus || > + (!is_idle_domain(d) && vcpu_id && !d->vcpu[vcpu_id - 1]) || Note how you still require an is_idle_domain() special case here anyway. > + d->vcpu[vcpu_id] ) > + { > + ASSERT_UNREACHABLE(); > + return NULL; I assume you consider it acceptable for release builds to report back -ENOMEM in the (hopefully indeed impossible) case of execution going this path? > @@ -178,16 +192,10 @@ struct vcpu *vcpu_create( > if ( arch_vcpu_create(v) != 0 ) > goto fail_sched; > > + /* Insert the vcpu into the domain's vcpu list. */ > d->vcpu[vcpu_id] = v; > - if ( vcpu_id != 0 ) > - { > - int prev_id = v->vcpu_id - 1; > - while ( (prev_id >= 0) && (d->vcpu[prev_id] == NULL) ) > - prev_id--; > - BUG_ON(prev_id < 0); > - v->next_in_list = d->vcpu[prev_id]->next_in_list; > - d->vcpu[prev_id]->next_in_list = v; > - } > + if ( !is_idle_domain(d) && vcpu_id > 0 ) > + d->vcpu[vcpu_id - 1]->next_in_list = v; While before this change for_each_vcpu(idle_domain) was broken only for the (currently impossible on x86 at least) case of CPU0 not being online (nor parked), afaict it will now be broken altogether, leading to NULL deref-s when used. Is that really what you want (if so, the description should say so, and why that's okay)? Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2018-09-17 1:08 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-09-06 19:25 [PATCH 0/3] xen: Improvements to the vcpu create/destroy paths Andrew Cooper 2018-09-06 19:25 ` [PATCH 1/3] xen/vcpu: Rename the common interfaces for consistency Andrew Cooper 2018-09-07 9:52 ` Jan Beulich 2018-09-13 10:29 ` Roger Pau Monné 2018-09-17 1:08 ` Julien Grall 2018-09-06 19:25 ` [PATCH 2/3] xen/vcpu: Introduce vcpu_destroy() Andrew Cooper 2018-09-07 9:53 ` Jan Beulich 2018-09-13 10:32 ` Roger Pau Monné 2018-09-06 19:25 ` [PATCH 3/3] xen/vcpu: Rework sanity checks in vcpu_create() Andrew Cooper 2018-09-06 20:07 ` Jason Andryuk 2018-09-06 23:01 ` Andrew Cooper 2018-09-07 10:15 ` Jan Beulich 2018-09-07 19:17 ` Andrew Cooper 2018-09-11 16:46 ` [PATCH v2] " Andrew Cooper 2018-09-12 11:38 ` Jason Andryuk 2018-09-12 15:00 ` Jan Beulich
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.