| Message ID | ab592d50ad161ffed3950bdf58ade49ae90a3c0e.1744126720.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | riscv: introduce basic UART support and interrupts for hypervisor mode | expand |
On 08.04.2025 17:57, Oleksii Kurochko wrote: > @@ -13,3 +16,68 @@ void __init smp_clear_cpu_maps(void) > cpumask_set_cpu(0, &cpu_online_map); > cpumask_copy(&cpu_present_map, &cpu_possible_map); > } > + > +/** > + * of_get_cpu_hwid - Get the hardware ID from a CPU device node > + * > + * @cpun: CPU number(logical index) for which device node is required > + * @thread: The local thread number to get the hardware ID for. > + * > + * Return: The hardware ID for the CPU node or ~0ULL if not found. > + */ > +static uint64_t of_get_cpu_hwid(struct dt_device_node *cpun, unsigned int thread) What does the "of" prefix stand for here? Looking at the function body I'm really at a loss. (I was first guessing something like OpenFirmware, but there's nothing here that would support that.) As you're only fetching data - can cpun be pointer-to-const? > +{ > + const __be32 *cell; > + int ac; > + uint32_t len; > + > + ac = dt_n_addr_cells(cpun); > + cell = dt_get_property(cpun, "reg", &len); > + if ( !cell || !ac || ((sizeof(*cell) * ac * (thread + 1)) > len) ) > + return ~0ULL; I'm sorry for my lack of DT knowledge, but what's "thread" representing here? You only pass in 0 below, so it's unclear whether it's what one might expect (the thread number on a multi-threaded core). > + cell += ac * thread; > + return dt_read_number(cell, ac); Nit (you know what) > +/* > + * Returns the hart ID of the given device tree node, or -ENODEV if the node > + * isn't an enabled and valid RISC-V hart node. > + */ > +int riscv_of_processor_hartid(struct dt_device_node *node, unsigned long *hart) Similar question as above: What's "of" and what significance does the "riscv" prefix have in RISC-V code? Const-ness question again for "node". > +{ > + const char *isa; > + > + if ( !dt_device_is_compatible(node, "riscv") ) > + { > + printk("Found incompatible CPU\n"); > + return -ENODEV; > + } > + > + *hart = (unsigned long) of_get_cpu_hwid(node, 0); > + if ( *hart == ~0UL ) While for RV64 this won't matter, the difference in types (uint64_t returned, unsigned long used) is still puzzling me. What's the deal? Jan
On 4/10/25 5:53 PM, Jan Beulich wrote: > On 08.04.2025 17:57, Oleksii Kurochko wrote: >> @@ -13,3 +16,68 @@ void __init smp_clear_cpu_maps(void) >> cpumask_set_cpu(0, &cpu_online_map); >> cpumask_copy(&cpu_present_map, &cpu_possible_map); >> } >> + >> +/** >> + * of_get_cpu_hwid - Get the hardware ID from a CPU device node >> + * >> + * @cpun: CPU number(logical index) for which device node is required >> + * @thread: The local thread number to get the hardware ID for. >> + * >> + * Return: The hardware ID for the CPU node or ~0ULL if not found. >> + */ >> +static uint64_t of_get_cpu_hwid(struct dt_device_node *cpun, unsigned int thread) > What does the "of" prefix stand for here? Looking at the function body I'm > really at a loss. (I was first guessing something like OpenFirmware, but > there's nothing here that would support that.) I copy this function from Linux kernel. But you are right, "of" means OpenFirmware or Open Firmware Device Tree and is used for the functions which work with device tree. I'll rename to dt_get_cpu_hwid() to follow the naming of device tree's functions name in Xen. > > As you're only fetching data - can cpun be pointer-to-const? Sure, it can be. I'll update that. > >> +{ >> + const __be32 *cell; >> + int ac; >> + uint32_t len; >> + >> + ac = dt_n_addr_cells(cpun); >> + cell = dt_get_property(cpun, "reg", &len); >> + if ( !cell || !ac || ((sizeof(*cell) * ac * (thread + 1)) > len) ) >> + return ~0ULL; > I'm sorry for my lack of DT knowledge, but what's "thread" representing here? > You only pass in 0 below, so it's unclear whether it's what one might expect > (the thread number on a multi-threaded core). Based on the DT specification alone, the|`reg`| value could refer to either a CPU or a thread ID: ``` The value of reg is a <prop-encoded-array> that defines a unique CPU/thread id for the CPU/threads represented by the CPU node. If a CPU supports more than one thread (i.e. multiple streams of execution) the reg prop-erty is an array with 1 element per thread. ``` My understanding is that the term/thread/ was used in the Linux kernel to cover both cases. When SMT isn't supported, the CPU can be considered to have a single thread. For example, RISC-V uses the term/hardware thread/ to describe a hart (i.e., a CPU). Interestingly, the Linux kernel always uses|thread = 0|. We could potentially drop this ambiguity and introduce an|ASSERT()| to check that the|`reg`| property contains only one entry, representing the HART (CPU) ID: ``` Software can determine the number of threads by dividing the size of reg by the parent node’s #address-cells. If `|reg`| has more than one entry, it would simply SMT support is required. ``` Does that approach make sense, or should we stick with the current implementation? > >> + cell += ac * thread; >> + return dt_read_number(cell, ac); > Nit (you know what) > >> +/* >> + * Returns the hart ID of the given device tree node, or -ENODEV if the node >> + * isn't an enabled and valid RISC-V hart node. >> + */ >> +int riscv_of_processor_hartid(struct dt_device_node *node, unsigned long *hart) > Similar question as above: What's "of" and what significance does the "riscv" > prefix have in RISC-V code? I will drop usage of 'of' in Xen and change it to 'dt'. > > Const-ness question again for "node". > >> +{ >> + const char *isa; >> + >> + if ( !dt_device_is_compatible(node, "riscv") ) >> + { >> + printk("Found incompatible CPU\n"); >> + return -ENODEV; >> + } >> + >> + *hart = (unsigned long) of_get_cpu_hwid(node, 0); >> + if ( *hart == ~0UL ) > While for RV64 this won't matter, the difference in types (uint64_t returned, > unsigned long used) is still puzzling me. What's the deal? No specific reason, just overlooked that moment. I think we could use just drop this cast. The reason for uint64_t as a return type is that dt_read_number() returns u64. ~ Oleksii
On 15.04.2025 15:39, Oleksii Kurochko wrote: > On 4/10/25 5:53 PM, Jan Beulich wrote: >> On 08.04.2025 17:57, Oleksii Kurochko wrote: >>> +{ >>> + const __be32 *cell; >>> + int ac; >>> + uint32_t len; >>> + >>> + ac = dt_n_addr_cells(cpun); >>> + cell = dt_get_property(cpun, "reg", &len); >>> + if ( !cell || !ac || ((sizeof(*cell) * ac * (thread + 1)) > len) ) >>> + return ~0ULL; >> I'm sorry for my lack of DT knowledge, but what's "thread" representing here? >> You only pass in 0 below, so it's unclear whether it's what one might expect >> (the thread number on a multi-threaded core). > > Based on the DT specification alone, the|`reg`| value could refer to either a CPU or a thread ID: > ``` > The value of reg is a <prop-encoded-array> that defines a unique CPU/thread id for > the CPU/threads represented by the CPU node. If a CPU supports more than one thread > (i.e. multiple streams of execution) the reg prop-erty is an array with 1 element > per thread. > ``` > > My understanding is that the term/thread/ was used in the Linux kernel to cover both > cases. > When SMT isn't supported, the CPU can be considered to have a single thread. > For example, RISC-V uses the term/hardware thread/ to describe a hart (i.e., a CPU). > > Interestingly, the Linux kernel always uses|thread = 0|. > > We could potentially drop this ambiguity and introduce an|ASSERT()| to check that > the|`reg`| property contains only one entry, representing the HART (CPU) ID: > ``` > Software can determine the number of threads by dividing the size of reg by the parent > node’s #address-cells. If `|reg`| has more than one entry, it would simply SMT support > is required. > ``` > > Does that approach make sense, or should we stick with the current implementation? If extra enabling is required to make multi-thread CPUs work, then panic()ing (not so much ASSERT()ing) may make sense, for the time being. Better would be if we could use all threads in a system right away. Jan
diff --git a/xen/arch/riscv/include/asm/smp.h b/xen/arch/riscv/include/asm/smp.h index 188c033718..9b68f1e27a 100644 --- a/xen/arch/riscv/include/asm/smp.h +++ b/xen/arch/riscv/include/asm/smp.h @@ -26,6 +26,9 @@ static inline void set_cpuid_to_hartid(unsigned long cpuid, void setup_tp(unsigned int cpuid); +struct dt_device_node; +int riscv_of_processor_hartid(struct dt_device_node *node, unsigned long *hart); + void smp_clear_cpu_maps(void); #endif diff --git a/xen/arch/riscv/smpboot.c b/xen/arch/riscv/smpboot.c index 0f4dcc28e1..3193639f00 100644 --- a/xen/arch/riscv/smpboot.c +++ b/xen/arch/riscv/smpboot.c @@ -1,5 +1,8 @@ #include <xen/cpumask.h> +#include <xen/device_tree.h> +#include <xen/errno.h> #include <xen/init.h> +#include <xen/types.h> cpumask_t cpu_online_map; cpumask_t cpu_present_map; @@ -13,3 +16,68 @@ void __init smp_clear_cpu_maps(void) cpumask_set_cpu(0, &cpu_online_map); cpumask_copy(&cpu_present_map, &cpu_possible_map); } + +/** + * of_get_cpu_hwid - Get the hardware ID from a CPU device node + * + * @cpun: CPU number(logical index) for which device node is required + * @thread: The local thread number to get the hardware ID for. + * + * Return: The hardware ID for the CPU node or ~0ULL if not found. + */ +static uint64_t of_get_cpu_hwid(struct dt_device_node *cpun, unsigned int thread) +{ + const __be32 *cell; + int ac; + uint32_t len; + + ac = dt_n_addr_cells(cpun); + cell = dt_get_property(cpun, "reg", &len); + if ( !cell || !ac || ((sizeof(*cell) * ac * (thread + 1)) > len) ) + return ~0ULL; + + cell += ac * thread; + return dt_read_number(cell, ac); +} + +/* + * Returns the hart ID of the given device tree node, or -ENODEV if the node + * isn't an enabled and valid RISC-V hart node. + */ +int riscv_of_processor_hartid(struct dt_device_node *node, unsigned long *hart) +{ + const char *isa; + + if ( !dt_device_is_compatible(node, "riscv") ) + { + printk("Found incompatible CPU\n"); + return -ENODEV; + } + + *hart = (unsigned long) of_get_cpu_hwid(node, 0); + if ( *hart == ~0UL ) + { + printk("Found CPU without hart ID\n"); + return -ENODEV; + } + + if ( !dt_device_is_available(node)) + { + printk("CPU with hartid=%lu is not available\n", *hart); + return -ENODEV; + } + + if ( dt_property_read_string(node, "riscv,isa", &isa) ) + { + printk("CPU with hartid=%lu has no \"riscv,isa\" property\n", *hart); + return -ENODEV; + } + + if ( isa[0] != 'r' || isa[1] != 'v' ) + { + printk("CPU with hartid=%lu has an invalid ISA of \"%s\"\n", *hart, isa); + return -ENODEV; + } + + return 0; +}
Implements riscv_of_processor_hartid() to get the hart ID of the given device tree node and do some checks if CPU is available and given device tree node has proper riscv,isa property. As a helper function of_get_cpu_hwid() is introduced to deal specifically with reg propery of a CPU device node. Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- xen/arch/riscv/include/asm/smp.h | 3 ++ xen/arch/riscv/smpboot.c | 68 ++++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+)