Re: [PATCH v2 4/6] xen/cpupool: Create different cpupools at boot time

[Juergen Gross <jgross@xxxxxxxx>]

On 11.03.22 12:29, Luca Fancellu wrote:
> > On 11 Mar 2022, at 10:18, Juergen Gross <jgross@xxxxxxxx> wrote:
> >
> > On 11.03.22 10:46, Jan Beulich wrote:
> > > On 11.03.2022 10:29, Juergen Gross wrote:
> > > > On 11.03.22 09:56, Luca Fancellu wrote:
> > > > > > On 11 Mar 2022, at 08:09, Juergen Gross <jgross@xxxxxxxx> wrote:
> > > > > > On 10.03.22 18:10, Luca Fancellu wrote:
> > > > > > > --- /dev/null
> > > > > > > +++ b/xen/common/boot_cpupools.c
> > > > > > > @@ -0,0 +1,212 @@
> > > > > > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > > > > > +/*
> > > > > > > + * xen/common/boot_cpupools.c
> > > > > > > + *
> > > > > > > + * Code to create cpupools at boot time for arm architecture.
> > > > > > Please drop the arm reference here.
> > > > > >
> > > > > > > + *
> > > > > > > + * Copyright (C) 2022 Arm Ltd.
> > > > > > > + */
> > > > > > > +
> > > > > > > +#include <xen/sched.h>
> > > > > > > +
> > > > > > > +#define BTCPUPOOLS_DT_NODE_NO_REG     (-1)
> > > > > > > +#define BTCPUPOOLS_DT_NODE_NO_LOG_CPU (-2)
> > > > > > Move those inside the #ifdef below, please
> > > > > >
> > > > > > > +
> > > > > > > +struct pool_map {
> > > > > > > +    int pool_id;
> > > > > > > +    int sched_id;
> > > > > > > +    struct cpupool *pool;
> > > > > > > +};
> > > > > > > +
> > > > > > > +static struct pool_map __initdata pool_cpu_map[NR_CPUS] =
> > > > > > > +    { [0 ... NR_CPUS-1] = {.pool_id = -1, .sched_id = -1, .pool = NULL} };
> > > > > > > +static unsigned int __initdata next_pool_id;
> > > > > > > +
> > > > > > > +#ifdef CONFIG_ARM
> > > > > > Shouldn't this be CONFIG_HAS_DEVICE_TREE?
> > > > > Yes, the only problem is that in get_logical_cpu_from_hw_id I use the arm 
> > > > > specific
> > > > > cpu_logical_map(…), so what do you think it’s the better way here?
> > > > > Do you think I should have everything under CONFIG_HAS_DEVICE_TREE
> > > > > and get_logical_cpu_from_hw_id under CONFIG_ARM like in this way below?
> > > > Hmm, what is the hwid used for on Arm? I guess this could be similar
> > > > to the x86 acpi-id?
> > > Since there's going to be only one of DT or ACPI, if anything this could
> > > be the APIC ID and then ...
> > > > So I'd rather put get_logical_cpu_from_hw_id() into Arm specific code
> > > > and add a related x86 function to x86 code. Depending on the answer to
> > > > above question this could either be get_cpu_id(), or maybe an identity
> > > > function.
> > > ... a lookup loop over x86_cpu_to_apicid[] (I thought we had a function
> > > doing so, but right now I can't find one).
> > It is the second half of get_cpu_id().
> I was going to say, maybe I can do something like this:
>
> #ifdef CONFIG_ARM
> #define hwid_from_logical_cpu_id(x) cpu_logical_map(x)
> #elif defined(CONFIG_X86)
> #define hwid_from_logical_cpu_id(x) x86_cpu_to_apicid(x)
> #else
> #define hwid_from_logical_cpu_id(x) (-1)
> #end
>
> static int __init get_logical_cpu_from_hw_id(unsigned int hwid)
> {
>      unsigned int i;
>
>      for ( i = 0; i < nr_cpu_ids; i++ )
>          if ( hwid_from_logical_cpu_id(i) == hwid )
>              return i;
>
>      return -1;
> }
>
> Do you think it is acceptable?
I'd rather have this abstraction in some header, but this is
something the related maintainers should decide.

> I see the current get_cpu_id(…) from x86 code is starting from an acpi id to
> lookup the apicid and then it is looking for the logical cpu number.
> In the x86 context, eventually, the reg property of a cpu node would hold an
> Acpi id or an apicid? I would have say the last one but I’m not sure now.
According to Jan ACPI and device tree are mutually exclusive, so the
apicid is probably the correct answer.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature