On Thu, Dec 14, 2023 at 02:08:45PM +0800, xianglai li wrote:
> From: lixianglai <lixianglai@xxxxxxxxxxx>
>
> Add loongarch cpu support, Define new cpu type 'loongarch64'
> and implement it's driver functions.
>
> Signed-off-by: lixianglai <lixianglai@xxxxxxxxxxx>
We usually prefer the full name to be used both for git authorship
and DCO purposes. I believe this would be "Xianglai Li" in your case.
> ---
> po/POTFILES | 1 +
> src/conf/schemas/basictypes.rng | 1 +
> src/cpu/cpu.c | 2 +
> src/cpu/cpu.h | 2 +
> src/cpu/cpu_loongarch.c | 742 ++++++++++++++++++++++++++++++++
> src/cpu/cpu_loongarch.h | 25 ++
> src/cpu/cpu_loongarch_data.h | 37 ++
> src/cpu/meson.build | 1 +
> src/qemu/qemu_capabilities.c | 1 +
> src/qemu/qemu_domain.c | 4 +
> src/util/virarch.c | 2 +
> src/util/virarch.h | 4 +
> 12 files changed, 822 insertions(+)
> create mode 100644 src/cpu/cpu_loongarch.c
> create mode 100644 src/cpu/cpu_loongarch.h
> create mode 100644 src/cpu/cpu_loongarch_data.h
This patch breaks the test suite:
$ make -C .../libvirt/build/build-aux sc_preprocessor_indentation
make: Entering directory '.../libvirt/build/build-aux'
cppi: .../libvirt/src/cpu/cpu_loongarch.h: line 23: not properly indented
cppi: .../libvirt/src/cpu/cpu_loongarch_data.h: line 23: not properly indented
cppi: .../libvirt/src/cpu/cpu_loongarch_data.h: line 31: not properly indented
incorrect preprocessor indentation
make: *** [.../libvirt/build-aux/syntax-check.mk:500:
sc_preprocessor_indentation] Error 1
make: Leaving directory '.../libvirt/build/build-aux'
Should be an easy enough fix.
Please make sure that 'meson test' runs successfully after every
single patch in the series, and that you have optional test tools
such as cppi installed.
> +++ b/src/conf/schemas/basictypes.rng
> @@ -470,6 +470,7 @@
> <value>x86_64</value>
> <value>xtensa</value>
> <value>xtensaeb</value>
> + <value>loongarch64</value>
This list is sorted alphabetically; please ensure that it remains
that way after your changes. Not all lists in libvirt are sorted
alphabetically, but generally speaking if you see one that is you
should keep it that way.
> +++ b/src/cpu/cpu_loongarch.c
> @@ -0,0 +1,742 @@
> +static const virArch archs[] = { VIR_ARCH_LOONGARCH64 };
> +
> +typedef struct _virCPULoongArchVendor virCPULoongArchVendor;
> +struct _virCPULoongArchVendor {
> + char *name;
> +};
> +
> +typedef struct _virCPULoongArchModel virCPULoongArchModel;
> +struct _virCPULoongArchModel {
> + char *name;
> + const virCPULoongArchVendor *vendor;
> + virCPULoongArchData data;
> +};
> +
> +typedef struct _virCPULoongArchMap virCPULoongArchMap;
> +struct _virCPULoongArchMap {
> + size_t nvendors;
> + virCPULoongArchVendor **vendors;
> + size_t nmodels;
> + virCPULoongArchModel **models;
> +};
This CPU driver appears to be directly modeled after the ppc64
driver. I wonder if all the complexity is necessary at this point in
time? Wouldn't it perhaps be better to start with a very bare-bone
CPU driver, modeled after the riscv64 one, and then grow from there
as the demand for more advanced features becomes apparent?
> +static int
> +virCPULoongArchGetHostPRID(void)
> +{
> + return 0x14c010;
> +}
Hardcoding the host CPU's PRID...
> +static int
> +virCPULoongArchGetHost(virCPUDef *cpu,
> + virDomainCapsCPUModels *models)
> +{
> + virCPUData *cpuData = NULL;
> + virCPULoongArchData *data;
> + int ret = -1;
> +
> + if (!(cpuData = virCPUDataNew(archs[0])))
> + goto cleanup;
> +
> + data = &cpuData->data.loongarch;
> + data->prid = g_new0(virCPULoongArchPrid, 1);
> + if (!data->prid)
> + goto cleanup;
> +
> +
> + data->len = 1;
> +
> + data->prid[0].value = virCPULoongArchGetHostPRID();
> + data->prid[0].mask = 0xffff00ul;
... and corresponding mask is definitely not acceptable. You'll need
to implement a function that fetches the value dynamically by using
whatever mechanism is appropriate, and of course ensure that such
code is only ever run on a loongarch64 host.
But again, do we really need that complexity right now? The riscv64
driver doesn't have any of that and is usable for many purposes.
> +static virCPUDef *
> +virCPULoongArchDriverBaseline(virCPUDef **cpus,
> + unsigned int ncpus,
> + virDomainCapsCPUModels *models ATTRIBUTE_UNUSED,
> + const char **features ATTRIBUTE_UNUSED,
> + bool migratable ATTRIBUTE_UNUSED)
The function arguments are not aligned properly here. There are
several other instances of this. Please make sure that things are
aligned correctly throughout.
> diff --git a/src/cpu/meson.build b/src/cpu/meson.build
> index 55396903b9..254d6b4545 100644
> --- a/src/cpu/meson.build
> +++ b/src/cpu/meson.build
> @@ -6,6 +6,7 @@ cpu_sources = [
> 'cpu_riscv64.c',
> 'cpu_s390.c',
> 'cpu_x86.c',
> + 'cpu_loongarch.c'
> ]
This is another list that needs to remain sorted...
> +++ b/src/util/virarch.h
> @@ -69,6 +69,8 @@ typedef enum {
> VIR_ARCH_XTENSA, /* XTensa 32 LE https://en.wikipedia.org/wiki/Xtensa#Processor_Cores */
> VIR_ARCH_XTENSAEB, /* XTensa 32 BE https://en.wikipedia.org/wiki/Xtensa#Processor_Cores */
>
> + VIR_ARCH_LOONGARCH64, /* LoongArch 64 LE */
> +
> VIR_ARCH_LAST,
> } virArch;
... as is this one and those that are derived from it, including
preferredMachines.
--
Andrea Bolognani / Red Hat / Virtualization
_______________________________________________
Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
