Message ID | 20170320131033.72avwxs6kgb4ogg7@citrix.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
>>> On 20.03.17 at 14:10, <wei.liu2@citrix.com> wrote: > On Mon, Mar 20, 2017 at 04:40:49AM -0600, Jan Beulich wrote: >> >>> On 16.03.17 at 18:54, <wei.liu2@citrix.com> wrote: >> > @@ -154,11 +155,11 @@ static void __init parse_dom0_nodes(const char *s) >> > } >> > custom_param("dom0_nodes", parse_dom0_nodes); >> > >> > -static cpumask_t __initdata dom0_cpus; >> > +cpumask_t __initdata dom0_cpus; >> >> I'd prefer if this variable remained static, and I think this is doable: >> >> > -static struct vcpu *__init setup_dom0_vcpu(struct domain *d, >> > - unsigned int vcpu_id, >> > - unsigned int cpu) >> > +struct vcpu *__init dom0_setup_vcpu(struct domain *d, >> > + unsigned int vcpu_id, >> > + unsigned int cpu) >> > { >> >> It's needed only by the callers of this function afaics, and the >> cpumask_first() / cpumask_cycle() invocations could easily move >> into the function, with the callers updating their "cpu" variables >> from v->processor (with v assumed to be a variable to store the >> return value of the function, and checked to be non-NULL). >> > > Like this? Almost (I'd have avoided using cpumask_last(), but that's a matter of taste): > @@ -1155,8 +1157,11 @@ static int __init construct_dom0_pv( > cpu = v->processor; > for ( i = 1; i < d->max_vcpus; i++ ) > { > - cpu = cpumask_cycle(cpu, &dom0_cpus); > - setup_dom0_vcpu(d, i, cpu); > + struct vcpu *p = setup_dom0_vcpu(d, i, cpu); > + if ( !p ) > + panic("Cannot allocate vcpu%u for Dom0", i); > + > + cpu = p->processor; > } Let's avoid panic()ing here, even if there's not much chance of making it much further. Either simply break out of the loop (on the assumption that further setup_dom0_vcpu()s won't succeed either) or simply make the assignment of cpu conditional upon p. Also p wants to be pointer-to-const here, and a blank line is needed between declaration and statements. Jan
diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c index 1c723c9ef1..102d3daea1 100644 --- a/xen/arch/x86/dom0_build.c +++ b/xen/arch/x86/dom0_build.c @@ -158,8 +158,9 @@ static cpumask_t __initdata dom0_cpus; static struct vcpu *__init setup_dom0_vcpu(struct domain *d, unsigned int vcpu_id, - unsigned int cpu) + unsigned int prev_cpu) { + unsigned int cpu = cpumask_cycle(prev_cpu, &dom0_cpus); struct vcpu *v = alloc_vcpu(d, vcpu_id, cpu); if ( v ) @@ -215,7 +216,8 @@ struct vcpu *__init alloc_dom0_vcpu0(struct domain *dom0) return NULL; dom0->max_vcpus = max_vcpus; - return setup_dom0_vcpu(dom0, 0, cpumask_first(&dom0_cpus)); + return setup_dom0_vcpu(dom0, 0, + cpumask_last(&dom0_cpus) /* so it wraps around to first pcpu */); } #ifdef CONFIG_SHADOW_PAGING @@ -1155,8 +1157,11 @@ static int __init construct_dom0_pv( cpu = v->processor; for ( i = 1; i < d->max_vcpus; i++ ) { - cpu = cpumask_cycle(cpu, &dom0_cpus); - setup_dom0_vcpu(d, i, cpu); + struct vcpu *p = setup_dom0_vcpu(d, i, cpu); + if ( !p ) + panic("Cannot allocate vcpu%u for Dom0", i); + + cpu = p->processor; } d->arch.paging.mode = 0; @@ -1902,8 +1907,11 @@ static int __init pvh_setup_cpus(struct domain *d, paddr_t entry, cpu = v->processor; for ( i = 1; i < d->max_vcpus; i++ ) { - cpu = cpumask_cycle(cpu, &dom0_cpus); - setup_dom0_vcpu(d, i, cpu); + struct vcpu *p = setup_dom0_vcpu(d, i, cpu); + if ( !p ) + panic("Cannot allocate vcpu%u for Dom0", i); + + cpu = p->processor; } rc = arch_set_info_hvm_guest(v, &cpu_ctx);