Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve support for RISC-V architecture on Linux #148

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ ARM_SRCS = [

RISCV_SRCS = [
"src/riscv/uarch.c",
"src/riscv/cache.c",
]

# Platform-specific sources and headers
Expand Down Expand Up @@ -86,9 +87,11 @@ LINUX_ARM32_SRCS = LINUX_ARM_SRCS + ["src/arm/linux/aarch32-isa.c"]
LINUX_ARM64_SRCS = LINUX_ARM_SRCS + ["src/arm/linux/aarch64-isa.c"]

LINUX_RISCV_SRCS = [
"src/riscv/linux/cpuinfo.c",
"src/riscv/linux/hwcap.c",
"src/riscv/linux/init.c",
"src/riscv/linux/riscv-isa.c",
"src/riscv/linux/riscv-hw.c",
"src/riscv/linux/riscv-isa.c",
]

ANDROID_ARM_SRCS = [
Expand Down
7 changes: 5 additions & 2 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -212,12 +212,15 @@ IF(CPUINFO_SUPPORTED_PLATFORM)
ENDIF()
ELSEIF(CPUINFO_TARGET_PROCESSOR MATCHES "^(riscv(32|64))$")
LIST(APPEND CPUINFO_SRCS
src/riscv/cache.c
src/riscv/uarch.c)
IF(CMAKE_SYSTEM_NAME STREQUAL "Linux")
LIST(APPEND CPUINFO_SRCS
src/riscv/linux/cpuinfo.c
src/riscv/linux/hwcap.c
src/riscv/linux/init.c
src/riscv/linux/riscv-hw.c
src/riscv/linux/riscv-isa.c)
src/riscv/linux/riscv-hw.c
src/riscv/linux/riscv-isa.c)
ENDIF()
ENDIF()

Expand Down
5 changes: 5 additions & 0 deletions configure.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,13 @@ def main(args):
"arm/android/properties.c",
]
if build.target.is_riscv:
sources += [
"riscv/cache.c",
]
if build.target.is_linux:
sources += [
"riscv/linux/cpuinfo.c",
"riscv/linux/hwcap.c",
"riscv/linux/init.c",
"riscv/linux/riscv-isa.c",
]
Expand Down
4 changes: 4 additions & 0 deletions include/cpuinfo-mock.h
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,10 @@ struct cpuinfo_mock_property {
#if CPUINFO_ARCH_ARM
void CPUINFO_ABI cpuinfo_set_hwcap2(uint32_t hwcap2);
#endif

#if CPUINFO_ARCH_RISCV32 || CPUINFO_ARCH_RISCV64
void CPUINFO_ABI cpuinfo_set_hwcap(uint32_t hwcap);
#endif
#endif

#if defined(__ANDROID__)
Expand Down
17 changes: 5 additions & 12 deletions include/cpuinfo.h
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@
#define CPUINFO_ARCH_RISCV32 1
#elif (__riscv_xlen == 64)
#define CPUINFO_ARCH_RISCV64 1
#else
#error "Unexpected __riscv_xlen"
#endif
#endif

Expand Down Expand Up @@ -556,6 +558,9 @@ enum cpuinfo_uarch {

/** HiSilicon TaiShan v110 (Huawei Kunpeng 920 series processors). */
cpuinfo_uarch_taishan_v110 = 0x00C00100,

/** SiFive U74-MC Standard Core. */
cpuinfo_uarch_u74_mc = 0x00D00100,
};

struct cpuinfo_processor {
Expand Down Expand Up @@ -1904,10 +1909,6 @@ static inline bool cpuinfo_has_arm_sve2(void) {
*/
/* RV32I/64I/128I Base ISA. */
bool i;
#if CPUINFO_ARCH_RISCV32
/* RV32E Base ISA. */
bool e;
#endif
/* Integer Multiply/Divide Extension. */
bool m;
/* Atomic Extension. */
Expand All @@ -1933,14 +1934,6 @@ static inline bool cpuinfo_has_riscv_i(void) {
#endif
}

static inline bool cpuinfo_has_riscv_e(void) {
#if CPUINFO_ARCH_RISCV32
return cpuinfo_isa.e;
#else
return false;
#endif
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this removed because it's not something folks need to switch on? If so, I'm also curious if we want to take this opportunity to remove everything that's not optional. I was a bit over-zealous when adding all the extensions, but when I was chatting about this with some folks they pointed out that the likelihood of anyone showing up with a chipset that doesn't support IMAFD is basically zero. Should we just limit this whole structure to 'c' and 'v' for now?

I'm not too strongly opinionated about this, but more surface area = more maintenance in the long-term.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed this because it was not used in any place, and in particular it was never captured by the cpuinfo_riscv_linux_decode_isa_from_hwcap method.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to see everything removed that is not immediately useful. Attempting to run without 'base isa' is not useful.


static inline bool cpuinfo_has_riscv_m(void) {
#if CPUINFO_ARCH_RISCV32 || CPUINFO_ARCH_RISCV64
return cpuinfo_isa.m;
Expand Down
2 changes: 1 addition & 1 deletion src/linux/api.h
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@

typedef bool (*cpuinfo_cpulist_callback)(uint32_t, uint32_t, void*);
CPUINFO_INTERNAL bool cpuinfo_linux_parse_cpulist(const char* filename, cpuinfo_cpulist_callback callback, void* context);
typedef bool (*cpuinfo_smallfile_callback)(const char*, const char*, void*);
typedef bool (*cpuinfo_smallfile_callback)(const char*, const char*, const char*, void*);
CPUINFO_INTERNAL bool cpuinfo_linux_parse_small_file(const char* filename, size_t buffer_size, cpuinfo_smallfile_callback, void* context);
typedef bool (*cpuinfo_line_callback)(const char*, const char*, void*, uint64_t);
CPUINFO_INTERNAL bool cpuinfo_linux_parse_multiline_file(const char* filename, size_t buffer_size, cpuinfo_line_callback, void* context);
Expand Down
10 changes: 5 additions & 5 deletions src/linux/processors.c
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ inline static bool is_whitespace(char c) {
static const uint32_t default_max_processors_count = CPU_SETSIZE;
#endif

static bool uint32_parser(const char* text_start, const char* text_end, void* context) {
static bool uint32_parser(const char* filename, const char* text_start, const char* text_end, void* context) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense, but do you mind submitting a separate PR to accomplish that?

Copy link
Contributor Author

@GlassOfWhiskey GlassOfWhiskey Dec 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created #211 with these improvements. I'll rebase as soon as it will be merged.

if (text_start == text_end) {
cpuinfo_log_error("failed to parse file %s: file is empty", KERNEL_MAX_FILENAME);
return false;
Expand All @@ -98,13 +98,13 @@ static bool uint32_parser(const char* text_start, const char* text_end, void* co
const char* parsed_end = parse_number(text_start, text_end, &kernel_max);
if (parsed_end == text_start) {
cpuinfo_log_error("failed to parse file %s: \"%.*s\" is not an unsigned number",
KERNEL_MAX_FILENAME, (int) (text_end - text_start), text_start);
filename, (int) (text_end - text_start), text_start);
return false;
} else {
for (const char* char_ptr = parsed_end; char_ptr != text_end; char_ptr++) {
if (!is_whitespace(*char_ptr)) {
cpuinfo_log_warning("non-whitespace characters \"%.*s\" following number in file %s are ignored",
(int) (text_end - char_ptr), char_ptr, KERNEL_MAX_FILENAME);
(int) (text_end - char_ptr), char_ptr, filename);
break;
}
}
Expand Down Expand Up @@ -255,7 +255,7 @@ static bool max_processor_number_parser(uint32_t processor_list_start, uint32_t
uint32_t cpuinfo_linux_get_max_possible_processor(uint32_t max_processors_count) {
uint32_t max_possible_processor = 0;
if (!cpuinfo_linux_parse_cpulist(POSSIBLE_CPULIST_FILENAME, max_processor_number_parser, &max_possible_processor)) {
#if CPUINFO_ARCH_ARM || CPUINFO_ARCH_ARM64
#if CPUINFO_ARCH_ARM || CPUINFO_ARCH_ARM64 || CPUINFO_ARCH_RISCV32 || CPUINFO_ARCH_RISCV64
cpuinfo_log_error("failed to parse the list of possible processors in %s", POSSIBLE_CPULIST_FILENAME);
#else
cpuinfo_log_warning("failed to parse the list of possible processors in %s", POSSIBLE_CPULIST_FILENAME);
Expand All @@ -274,7 +274,7 @@ uint32_t cpuinfo_linux_get_max_possible_processor(uint32_t max_processors_count)
uint32_t cpuinfo_linux_get_max_present_processor(uint32_t max_processors_count) {
uint32_t max_present_processor = 0;
if (!cpuinfo_linux_parse_cpulist(PRESENT_CPULIST_FILENAME, max_processor_number_parser, &max_present_processor)) {
#if CPUINFO_ARCH_ARM || CPUINFO_ARCH_ARM64
#if CPUINFO_ARCH_ARM || CPUINFO_ARCH_ARM64 || CPUINFO_ARCH_RISCV32 || CPUINFO_ARCH_RISCV64
cpuinfo_log_error("failed to parse the list of present processors in %s", PRESENT_CPULIST_FILENAME);
#else
cpuinfo_log_warning("failed to parse the list of present processors in %s", PRESENT_CPULIST_FILENAME);
Expand Down
2 changes: 1 addition & 1 deletion src/linux/smallfile.c
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ bool cpuinfo_linux_parse_small_file(const char* filename, size_t buffer_size, cp
}
} while (bytes_read != 0);

status = callback(buffer, &buffer[buffer_position], context);
status = callback(filename, buffer, &buffer[buffer_position], context);

cleanup:
if (file != -1) {
Expand Down
28 changes: 28 additions & 0 deletions src/riscv/api.h
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

#include <cpuinfo.h>
#include <cpuinfo/common.h>
#include <cpuinfo/internal-api.h>

/* RISC-V Vendor IDs. */
enum cpuinfo_riscv_chipset_vendor {
Expand Down Expand Up @@ -40,3 +41,30 @@ CPUINFO_INTERNAL void cpuinfo_riscv_decode_vendor_uarch(
uint32_t imp_id,
enum cpuinfo_vendor vendor[restrict static 1],
enum cpuinfo_uarch uarch[restrict static 1]);

/**
* Decodes the cache hierarchy based on the provided inpu parameters,
* regardless of underlying operating system.
*
* @param[uarch]: The processor micro-architecture code.
* @param[l1i] - Reference to the l1i cpuinfo_cache to populate.
* @param[l1d]: - Reference to the l1d cpuinfo_cache to populate.
* @param[l2]: - Reference to the l2 cpuinfo_cache to populate.
* @return false if any error occurred, true otherwise
*/

CPUINFO_INTERNAL bool cpuinfo_riscv_decode_cache(
enum cpuinfo_uarch uarch,
struct cpuinfo_cache l1i[restrict static 1],
struct cpuinfo_cache l1d[restrict static 1],
struct cpuinfo_cache l2[restrict static 1]);

/**
* Extracts the maximum cache size from a RISC-V processor, independently
* of underlying operating system.
*
* @param[processor]: The RISC-V processor.
* @preturn: The maximum cache size.
*/
CPUINFO_INTERNAL uint32_t cpuinfo_riscv_compute_max_cache_size(
const struct cpuinfo_processor processor[restrict static 1]);
56 changes: 56 additions & 0 deletions src/riscv/cache.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
#include <inttypes.h>

#include <cpuinfo.h>
#include <cpuinfo/internal-api.h>
#include <cpuinfo/log.h>

bool cpuinfo_riscv_decode_cache(
enum cpuinfo_uarch uarch,
struct cpuinfo_cache l1i[restrict static 1],
struct cpuinfo_cache l1d[restrict static 1],
struct cpuinfo_cache l2[restrict static 1]) {
switch(uarch) {
// According to https://starfivetech.com/uploads/u74mc_core_complex_manual_21G1.pdf
case cpuinfo_uarch_u74_mc:
GlassOfWhiskey marked this conversation as resolved.
Show resolved Hide resolved
*l1i = (struct cpuinfo_cache) {
.size = 32 * 1024,
.associativity = 2,
.line_size = 64
};
*l1d = (struct cpuinfo_cache) {
.size = 32 * 1024,
.associativity = 4,
.line_size = 64
};
*l2 = (struct cpuinfo_cache) {
.size = 2 * 1024 * 1024,
.associativity = 16,
.line_size = 64
};
break;
default:
cpuinfo_log_warning("target uarch not recognized; cache data is not populated");
GlassOfWhiskey marked this conversation as resolved.
Show resolved Hide resolved
return false;
}
l1i->sets = l1i->size / (l1i->associativity * l1i->line_size);
l1i->partitions = 1;
l1d->sets = l1d->size / (l1d->associativity * l1d->line_size);
l1d->partitions = 1;
if (l2->size != 0) {
l2->sets = l2->size / (l2->associativity * l2->line_size);
l2->partitions = 1;
}

return true;
}

uint32_t cpuinfo_riscv_compute_max_cache_size(const struct cpuinfo_processor* processor) {
GlassOfWhiskey marked this conversation as resolved.
Show resolved Hide resolved
switch(processor->core->uarch) {
// According to https://starfivetech.com/uploads/u74mc_core_complex_manual_21G1.pdf
case cpuinfo_uarch_u74_mc:
return 2 * 1024 * 1024;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this always going to be the L2 cache size? Or the size of the largest cache of the set (L1i, L1d, L2)? If so, can we make this fully programmatic and write something like:

,,l2 = decode_cache(...); return l2.size?

This is just to ensure there's always one-source-of-truth of the data, in the function above.

default:
cpuinfo_log_warning("target uarch not recognized; max cache size is not populated");
return 0;
}
}
45 changes: 44 additions & 1 deletion src/riscv/linux/api.h
Original file line number Diff line number Diff line change
@@ -1,8 +1,26 @@
#pragma once

#include <inttypes.h>

#include <cpuinfo.h>
#include <cpuinfo/common.h>

#if CPUINFO_ARCH_RISCV32 || CPUINFO_ARCH_RISCV64
/* arch/riscv/include/uapi/asm/hwcap.h */
#define CPUINFO_RISCV_LINUX_FEATURE_I UINT32_C(0x00000100)
#define CPUINFO_RISCV_LINUX_FEATURE_M UINT32_C(0x00001000)
#define CPUINFO_RISCV_LINUX_FEATURE_A UINT32_C(0x00000001)
#define CPUINFO_RISCV_LINUX_FEATURE_F UINT32_C(0x00000020)
#define CPUINFO_RISCV_LINUX_FEATURE_D UINT32_C(0x00000008)
#define CPUINFO_RISCV_LINUX_FEATURE_C UINT32_C(0x00000004)
#define CPUINFO_RISCV_LINUX_FEATURE_V UINT32_C(0x00200000)
GlassOfWhiskey marked this conversation as resolved.
Show resolved Hide resolved
#endif

#define CPUINFO_RISCV_LINUX_VALID_ARCHITECTURE UINT32_C(0x10000000)
#define CPUINFO_RISCV_LINUX_VALID_IMPLEMENTER UINT32_C(0x20000000)
#define CPUINFO_RISCV_LINUX_VALID_PROCESSOR UINT32_C(0x40000000)
#define CPUINFO_RISCV_LINUX_VALID_FEATURES UINT32_C(0x80000000)

/**
* Definition of a RISC-V Linux processor. It is composed of the base processor
* definition in "include/cpuinfo.h" and flags specific to RISC-V Linux
Expand Down Expand Up @@ -41,6 +59,9 @@ struct cpuinfo_riscv_linux_processor {
* is the same for all logical processors on the same package.
*/
uint32_t package_leader_id;

/* RISC-V ISA extensions supported by this processor */
uint32_t features;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible currently to have a core complex where different processors have different feature sets? (E.g. Why is this a processor-specific set, as opposed to one riscv_isa structure for the whole package?)

};

/**
Expand All @@ -65,5 +86,27 @@ CPUINFO_INTERNAL void cpuinfo_riscv_linux_decode_vendor_uarch_from_hwprobe(
enum cpuinfo_vendor vendor[restrict static 1],
enum cpuinfo_uarch uarch[restrict static 1]);

/**
* Reads the value of hwcap from the `getauxval` function, or
* mocks a fake value for testing purposes
*
* @param[hwcap] - The hwcap flags to be populated
*/

CPUINFO_INTERNAL void cpuinfo_riscv_linux_hwcap_from_getauxval(
uint32_t hwcap[restrict static 1]);

/**
* Parses the output of the `/proc/cpuinfo` command to extract
* info about the RISC-V processors.
*
* @param[max_processors_count] - The maximum number of processors.
* @param processors - Reference to the processor list to populate.
* @return false if any error occurred, true otherwise.
*/
CPUINFO_INTERNAL bool cpuinfo_riscv_linux_parse_proc_cpuinfo(
uint32_t max_processors_count,
struct cpuinfo_riscv_linux_processor processors[restrict static max_processors_count]);

/* Used to determine which uarch is associated with the current thread. */
extern CPUINFO_INTERNAL const uint32_t* cpuinfo_linux_cpu_to_uarch_index_map;
extern CPUINFO_INTERNAL const uint32_t* cpuinfo_linux_cpu_to_uarch_index_map;
Loading