On 9/28/22 10:54, Yu Gu wrote:
> This patch provides basic support for the RISC-V architecture, so
> libvirt can run in RISC-V machine.
>
> Signed-off-by: Yu Gu <guyu2876@xxxxxxxxx>
> ---
> po/POTFILES | 1 +
> src/cpu/cpu.c | 2 +
> src/cpu/cpu.h | 2 +
> src/cpu/cpu_riscv64.c | 118 +++++++++++++++++++++++++++++++
> src/cpu/cpu_riscv64.h | 28 ++++++++
> src/cpu/cpu_riscv64_data.h | 40 +++++++++++
> src/cpu/meson.build | 1 +
> src/cpu_map/index.xml | 4 ++
> src/cpu_map/meson.build | 1 +
> src/cpu_map/riscv64_vendors.xml | 3 +
> src/util/virarch.c | 2 +
> src/util/virhostcpu.c | 2 +-
> src/util/virsysinfo.c | 121 ++++++++++++++++++++++++++++++++
> 13 files changed, 324 insertions(+), 1 deletion(-)
> create mode 100644 src/cpu/cpu_riscv64.c
> create mode 100644 src/cpu/cpu_riscv64.h
> create mode 100644 src/cpu/cpu_riscv64_data.h
> create mode 100644 src/cpu_map/riscv64_vendors.xml
Hey, this is just an RFC, right? Because there are couple of issues with
this patch. And I don't have RISCV anywhere handy, so I'd appreciate if
you'd add a sysinfotest test case.
And for your next version, please make sure the code compiles with:
meson -Dexpensive_tests=enabled _build && ninja -C _build/ test
>
> diff --git a/po/POTFILES b/po/POTFILES
> index 169e2a41dc..a52795e7c1 100644
> --- a/po/POTFILES
> +++ b/po/POTFILES
> @@ -72,6 +72,7 @@ src/cpu/cpu_map.c
> src/cpu/cpu_ppc64.c
> src/cpu/cpu_s390.c
> src/cpu/cpu_x86.c
> +src/cpu/cpu_riscv64.c
> src/datatypes.c
> src/driver.c
> src/esx/esx_driver.c
> diff --git a/src/cpu/cpu.c b/src/cpu/cpu.c
> index d97ef5e873..8fdc42e719 100644
> --- a/src/cpu/cpu.c
> +++ b/src/cpu/cpu.c
> @@ -27,6 +27,7 @@
> #include "cpu_ppc64.h"
> #include "cpu_s390.h"
> #include "cpu_arm.h"
> +#include "cpu_riscv64.h"
> #include "capabilities.h"
>
>
> @@ -39,6 +40,7 @@ static struct cpuArchDriver *drivers[] = {
> &cpuDriverPPC64,
> &cpuDriverS390,
> &cpuDriverArm,
> + &cpuDriverRISCV64,
> };
>
>
> diff --git a/src/cpu/cpu.h b/src/cpu/cpu.h
> index 41a62ce486..6e0a06fce4 100644
> --- a/src/cpu/cpu.h
> +++ b/src/cpu/cpu.h
> @@ -27,6 +27,7 @@
> #include "cpu_x86_data.h"
> #include "cpu_ppc64_data.h"
> #include "cpu_arm_data.h"
> +#include "cpu_riscv64_data.h"
>
>
> typedef struct _virCPUData virCPUData;
> @@ -36,6 +37,7 @@ struct _virCPUData {
> virCPUx86Data x86;
> virCPUppc64Data ppc64;
> virCPUarmData arm;
> + virCPUriscv64Data riscv64;
> /* generic driver needs no data */
> } data;
> };
> diff --git a/src/cpu/cpu_riscv64.c b/src/cpu/cpu_riscv64.c
> new file mode 100644
> index 0000000000..21f7178cc2
> --- /dev/null
> +++ b/src/cpu/cpu_riscv64.c
> @@ -0,0 +1,118 @@
> +/*
> + * cpu_riscv64.c: CPU driver for riscv64(x) CPUs
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library. If not, see
> + * <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <config.h>
> +
> +#include "cpu.h"
> +
> +
> +#define VIR_FROM_THIS VIR_FROM_CPU
> +
> +static const virArch archs[] = { VIR_ARCH_RISCV64 };
> +
> +static virCPUCompareResult
> +virCPUriscv64Compare(virCPUDef *host G_GNUC_UNUSED,
> + virCPUDef *cpu G_GNUC_UNUSED,
> + bool failMessages G_GNUC_UNUSED)
Alignment. Here and in couple of other places.
> +{
> + /* riscv64 relies on QEMU to perform all runability checking. Return
> + * VIR_CPU_COMPARE_IDENTICAL to bypass Libvirt checking.
> + */
> + return VIR_CPU_COMPARE_IDENTICAL;
> +}
> +
> +static int
> +virCPUriscv64Update(virCPUDef *guest,
> + const virCPUDef *host,
> + bool relative)
> +{
> + g_autoptr(virCPUDef) updated = NULL;
> + size_t i;
> +
> + if (!relative)
> + return 0;
> +
> + if (guest->mode == VIR_CPU_MODE_CUSTOM) {
> + if (guest->match == VIR_CPU_MATCH_MINIMUM) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("match mode %s not supported"),
> + virCPUMatchTypeToString(guest->match));
> + } else {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("optional CPU features are not supported"));
> + }
> + return -1;
> + }
> +
> + if (!host) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("unknown host CPU model"));
> + return -1;
> + }
> +
> + if (!(updated = virCPUDefCopyWithoutModel(guest)))
> + return -1;
> +
> + updated->mode = VIR_CPU_MODE_CUSTOM;
> + if (virCPUDefCopyModel(updated, host, true) < 0)
> + return -1;
> +
> + for (i = 0; i < guest->nfeatures; i++) {
> + if (virCPUDefUpdateFeature(updated,
> + guest->features[i].name,
> + guest->features[i].policy) < 0)
> + return -1;
> + }
> +
> + virCPUDefStealModel(guest, updated, false);
> + guest->mode = VIR_CPU_MODE_CUSTOM;
> + guest->match = VIR_CPU_MATCH_EXACT;
> +
> + return 0;
> +}
> +
> +
> +static int
> +virCPUriscv64ValidateFeatures(virCPUDef *cpu)
> +{
> + size_t i;
> +
> + for (i = 0; i < cpu->nfeatures; i++) {
> + if (cpu->features[i].policy == VIR_CPU_FEATURE_OPTIONAL) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("only cpu feature policies 'require' and "
> + "'disable' are supported for %s"),
Error messages are exempt from the 80chars rule. In other words, It's
desired to put the whole message onto a single line. The reason is
simple: easier git grep. I know you'll find a lot of old code which
still breaks error messages, but we change that whenever the old code is
changed and surely we want to already have that for any new code.
> + cpu->features[i].name);
> + return -1;
> + }
> + }
> +
> + return 0;
> +}
> +
> +struct cpuArchDriver cpuDriverRISCV64 = {
> + .name = "riscv64",
> + .arch = archs,
> + .narch = G_N_ELEMENTS(archs),
> + .compare = virCPUriscv64Compare,
> + .decode = NULL,
> + .encode = NULL,
> + .baseline = NULL,
> + .update = virCPUriscv64Update,
> + .validateFeatures = virCPUriscv64ValidateFeatures,
> +};
> diff --git a/src/cpu/cpu_riscv64.h b/src/cpu/cpu_riscv64.h
> new file mode 100644
> index 0000000000..67528415fe
> --- /dev/null
> +++ b/src/cpu/cpu_riscv64.h
> @@ -0,0 +1,28 @@
> +/*
> + * cpu_riscv64.h: CPU driver for 64-bit RISC-V CPUs
> + *
> + * Copyright (C) Copyright (C) IBM Corporation, 2010
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library. If not, see
> + * <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef __VIR_CPU_RISCV64_H__
> +# define __VIR_CPU_RISCV64_H__
We use #pragma once nowadays.
> +
> +# include "cpu.h"
> +
> +extern struct cpuArchDriver cpuDriverRISCV64;
> +
> +#endif
> diff --git a/src/cpu/cpu_riscv64_data.h b/src/cpu/cpu_riscv64_data.h
> new file mode 100644
> index 0000000000..819b9e8fde
> --- /dev/null
> +++ b/src/cpu/cpu_riscv64_data.h
> @@ -0,0 +1,40 @@
> +/*
> + * cpu_riscv64_data.h: 64-bit riscv64 CPU specific data
> + *
> + * Copyright (C) 2012 IBM Corporation.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; If not, see
> + * <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef __VIR_CPU_RISCV64_DATA_H__
> +# define __VIR_CPU_RISCV64_DATA_H__
> +
> +# include <stdint.h>
> +
> +typedef struct _virCPUriscv64Prid virCPUriscv64Prid;
> +struct _virCPUriscv64Prid {
> + uint32_t value;
> + uint32_t mask;
> +};
> +
> +# define VIR_CPU_riscv64_DATA_INIT { 0 }
> +
> +typedef struct _virCPUriscv64Data virCPUriscv64Data;
> +struct _virCPUriscv64Data {
> + size_t len;
> + virCPUriscv64Prid *prid;
> +};
> +
> +#endif
I wonder what's the purpose for this file, because nothing it declares
is ever used.
> diff --git a/src/cpu/meson.build b/src/cpu/meson.build
> index b4ad95e46d..eba1d45743 100644
> --- a/src/cpu/meson.build
> +++ b/src/cpu/meson.build
> @@ -5,6 +5,7 @@ cpu_sources = [
> 'cpu_ppc64.c',
> 'cpu_s390.c',
> 'cpu_x86.c',
> + 'cpu_riscv64.c',
> ]
>
> cpu_lib = static_library(
> diff --git a/src/cpu_map/index.xml b/src/cpu_map/index.xml
> index d533a28865..f2c4b1c62a 100644
> --- a/src/cpu_map/index.xml
> +++ b/src/cpu_map/index.xml
> @@ -89,6 +89,10 @@
> <include filename='ppc64_POWERPC_e6500.xml'/>
> </arch>
>
> + <arch name='riscv64'>
> + <include filename='riscv64_vendors.xml'/>
> + </arch>
> +
> <arch name='arm'>
> <include filename='arm_vendors.xml'/>
> <include filename='arm_features.xml'/>
> diff --git a/src/cpu_map/meson.build b/src/cpu_map/meson.build
> index 99264289e2..1a02df8268 100644
> --- a/src/cpu_map/meson.build
> +++ b/src/cpu_map/meson.build
> @@ -19,6 +19,7 @@ cpumap_data = [
> 'ppc64_POWERPC_e5500.xml',
> 'ppc64_POWERPC_e6500.xml',
> 'ppc64_vendors.xml',
> + 'riscv64_vendors.xml',
> 'x86_486.xml',
> 'x86_athlon.xml',
> 'x86_Broadwell-IBRS.xml',
> diff --git a/src/cpu_map/riscv64_vendors.xml b/src/cpu_map/riscv64_vendors.xml
> new file mode 100644
> index 0000000000..478a23a467
> --- /dev/null
> +++ b/src/cpu_map/riscv64_vendors.xml
> @@ -0,0 +1,3 @@
> +<cpus>
> + <vendor name='RISC-V'/>
> +</cpus>
> \ No newline at end of file
> diff --git a/src/util/virarch.c b/src/util/virarch.c
> index 2134dd6a9d..3d14ecd193 100644
> --- a/src/util/virarch.c
> +++ b/src/util/virarch.c
> @@ -190,6 +190,8 @@ virArch virArchFromHost(void)
> return VIR_ARCH_ALPHA;
> case PROCESSOR_ARCHITECTURE_PPC:
> return VIR_ARCH_PPC;
> + case PROCESSOR_ARCHITECTURE_RISCV:
> + return VIR_ARCH_RISCV64;
> case PROCESSOR_ARCHITECTURE_SHX:
> return VIR_ARCH_SH4;
> case PROCESSOR_ARCHITECTURE_ARM:
> diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c
> index c1e8dc8078..08c2290f00 100644
> --- a/src/util/virhostcpu.c
> +++ b/src/util/virhostcpu.c
> @@ -544,7 +544,7 @@ virHostCPUParseFrequency(FILE *cpuinfo,
> char line[1024];
>
> /* No sensible way to retrieve CPU frequency */
> - if (ARCH_IS_ARM(arch))
> + if (ARCH_IS_ARM(arch) || ARCH_IS_RISCV(arch))
> return 0;
>
> if (ARCH_IS_X86(arch))
> diff --git a/src/util/virsysinfo.c b/src/util/virsysinfo.c
> index 376d5d4816..e281d928c7 100644
> --- a/src/util/virsysinfo.c
> +++ b/src/util/virsysinfo.c
> @@ -623,6 +623,125 @@ virSysinfoReadS390(void)
> return g_steal_pointer(&ret);
> }
>
> +#if 0
> +static int
> +virSysinfoParseRISCVSystem(const char *base, virSysinfoSystemDef **sysdef)
> +{
> + int ret = -1;
> + virSysinfoSystemDef *def;
> +
> + def = g_new0(virSysinfoSystemDef, 1);
> +
> +#if 0
> + if (!virSysinfoParseS390Line(base, "Manufacturer", &def->manufacturer))
At this point, maube the function should be renamed? It feels very weird
that a RISCV function calls an S390 function.
> + goto cleanup;
> +
> + if (!virSysinfoParseS390Line(base, "Type", &def->family))
Ditto. And I believe this is going to be uncommented so that you can
detect the manufacturer for the sysinfotest, right?
> + goto cleanup;
> +#endif
> + def->manufacturer = g_strndup("Virt-RISC-V", sizeof("Virt RISC-V"));
> +
> + if (!def->manufacturer && !def->product && !def->version &&
> + !def->serial && !def->uuid && !def->sku && !def->family) {
> + g_clear_pointer(&def, virSysinfoSystemDefFree);
> + }
> +
> + *sysdef = g_steal_pointer(&def);
> + ret = 0;
> + cleanup:
> + virSysinfoSystemDefFree(def);
> + return ret;
> +}
> +#endif
> +
> +static int
> +virSysinfoParseRISCVProcessor(const char *base, virSysinfoDef *ret)
> +{
> + const char *tmp_base;
> + char *manufacturer = NULL;
> + char *procline = NULL;
> + char *ncpu = NULL;
> + int result = -1;
> + virSysinfoProcessorDef *processor;
> +
> + if (!(tmp_base = virSysinfoParseS390Line(base, "uarch", &manufacturer)))
> + goto error;
> +
> + /* Find processor N: line and gather the processor manufacturer,
> + version, serial number, and family */
> + while ((tmp_base = strstr(tmp_base, "processor "))
> + && (tmp_base = virSysinfoParseS390Line(tmp_base, "processor ",
> + &procline))) {
> + VIR_EXPAND_N(ret->processor, ret->nprocessor, 1);
> + processor = &ret->processor[ret->nprocessor - 1];
> + processor->processor_manufacturer = g_strdup(manufacturer);
> +
> + VIR_FREE(procline);
> + }
> +
> + /* now, for each processor found, extract the frequency information */
> + tmp_base = base;
> +
> + while ((tmp_base = strstr(tmp_base, "hart")) &&
> + (tmp_base = virSysinfoParseS390Line(tmp_base, "hart", &ncpu))) {
> + unsigned int n;
> + char *mhz = NULL;
> +
> + if (virStrToLong_uip(ncpu, NULL, 10, &n) < 0)
> + goto error;
> +
> + if (n >= ret->nprocessor) {
> + VIR_DEBUG("CPU number '%u' out of range", n);
> + goto cleanup;
> + }
> +
> + VIR_FREE(ncpu);
> + }
> +
> + cleanup:
> + result = 0;
> +
> + error:
> + VIR_FREE(manufacturer);
> + VIR_FREE(procline);
> + VIR_FREE(ncpu);
> + return result;
> +}
> +
> +virSysinfoDef *
> +virSysinfoReadRISCV(void)
You need to add a declaration for this function into
src/util/virsysinfopriv.h.
Michal
