|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2 08/16] xen/riscv: dt_processor_cpuid() implementation
On 5/15/25 9:56 AM, Jan Beulich wrote:
(adding Bertrand as the one further DT maintainer, for a respective question below) On 06.05.2025 18:51, Oleksii Kurochko wrote:Implements dt_processor_hartid()There's no such function (anymore).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 dt_get_cpuid() is introduced to deal specifically with reg propery of a CPU device node. Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx> --- Changes in V2: - s/of_get_cpu_hwid()/dt_get_cpu_id(). - Update prototype of dt_get_cpu_hwid(), use pointer-to-const for cpun arg. - Add empty line before last return in dt_get_cpu_hwid(). - s/riscv_of_processor_hartid/dt_processor_cpuid(). - Use pointer-to_const for node argument of dt_processor_cpuid(). - Use for hart_id unsigned long type as according to the spec for RV128 mhartid register will be 128 bit long. - Update commit message and subject. - use 'CPU' instead of 'HART'.Was this is good move? What is returned ...--- 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 dt_processor_cpuid(const struct dt_device_node *node, unsigned long *cpuid);... here isn't a number in Xen's CPU numbering space. From earlier discussions I'm not sure it's a hart ID either, so it may need further clarification (and I'd expect RISC-V to have suitable terminology to tell apart the different entities). From device tree point of view (https://www.kernel.org/doc/Documentation/devicetree/bindings/riscv/cpus.txt) it is just hart which is defined as a hardware execution context, which contains all the state mandated by the RISC-V ISA: a PC and some registers. And also based on this for DT binding: For example, my Intel laptop is described as having one socket with two cores, each of which has two hyper threads. Therefore this system has four harts. They are using just harts and in DT it will look like 4 node with a number in reg property which they call hart. So from DT point of view hartid is pretty fine to use. >From specification point of view (https://riscv.github.io/riscv-isa-manual/snapshot/unprivileged/#_risc_v_hardware_platform_terminology): A RISC-V hardware platform can contain one or more RISC-V-compatible processing cores together with other non-RISC-V-compatible cores, fixed-function accelerators, various physical memory structures, I/O devices, and an interconnect structure to allow the components to communicate. A component is termed a core if it contains an independent instruction fetch unit. A RISC-V- compatible core might support multiple RISC-V-compatible hardware threads, or harts, through multithreading. and (https://riscv.github.io/riscv-isa-manual/snapshot/unprivileged/#_risc_v_software_execution_environments_and_harts): From the perspective of software running in a given execution environment, a hart is a resource that autonomously fetches and executes RISC-V instructions within that execution environment. In this respect, a hart behaves like a hardware thread resource even if time-multiplexed onto real hardware by the execution environment. Some EEIs support the creation and destruction of additional harts, for example, via environment calls to fork new harts. DT's CPU node should be refer to core plus hardware thread (or threads in case of multithreading) in reg property to be close to the spec(?) but basically in DT they IMO just describe what in the sepc is called an independent instruction fetch unit. I still think that it is fine just to use hart_id. If to be close more to a spec the unit_id(?) but it is more confusing IMO with what is use in RISC-V's DT binding.
I think that we can declare ac as unsigned int even dt_n_addr_cells return int as basically it returns be32_to_cpu(*ip) which return unsigned int and it isn't expected to be a big value so overflow of INT_MAX shouldn't happen and it won't affect a size argument of dt_read_number(). In fact, that function is raising another question: If the "size" argument is outside of [0, 2], the value returned is silently truncated. Usually address-cells is 1 or 2, so it isn't expected to be higher (but DT tells that the value is u32 so it could be higher then 2). And I think it could be also explained by bitness of an architecture. I think I see address-cells equal to 3 for something connected to PCI, but probably another one functions should be used to read. More generally - are there any plans to make DT code signed-ness-correct? I will update that to:
@@ -46,6 +46,7 @@ static unsigned long dt_get_cpuid(const struct dt_device_node *cpun)
int dt_processor_cpuid(const struct dt_device_node *node, unsigned long *cpuid)
{
const char *isa;
+ int ret;
if ( !dt_device_is_compatible(node, "riscv") )
{
@@ -57,7 +58,7 @@ int dt_processor_cpuid(const struct dt_device_node *node, unsigned long *cpuid)
if ( *cpuid == ~0UL )
{
printk("Found CPU without CPU ID\n");
- return -ENODEV;
+ return -EINVAL;
}
if ( !dt_device_is_available(node))
@@ -66,16 +67,16 @@ int dt_processor_cpuid(const struct dt_device_node *node, unsigned long *cpuid)
return -ENODEV;
}
- if ( dt_property_read_string(node, "riscv,isa", &isa) )
+ if ( (ret = dt_property_read_string(node, "riscv,isa", &isa)) )
{
printk("CPU with cpuid=%lu has no \"riscv,isa\" property\n", *cpuid);
- return -ENODEV;
+ return ret;
}
if ( isa[0] != 'r' || isa[1] != 'v' )
{
printk("CPU with cpuid=%lu has an invalid ISA of \"%s\"\n", *cpuid, isa);
- return -ENODEV;
+ return -EINVAL;
}
return 0;
I think it's better now.
Thanks.
~ Oleksii
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |