[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.