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

[Jan Beulich <jbeulich@xxxxxxxx>]

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).

Jan