Re: [Xen-devel] [PATCH v2 2/4] x86: split PV dom0 builder to pv/dom0_builder.c

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.03.17 at 14:10, <wei.liu2@xxxxxxxxxx> wrote:
> On Mon, Mar 20, 2017 at 04:40:49AM -0600, Jan Beulich wrote:
>> >>> On 16.03.17 at 18:54, <wei.liu2@xxxxxxxxxx> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel