12:38 a.m.
Hi Andrea:
On Thu, Dec 14, 2023 at 02:08:45PM +0800, xianglai li wrote:
> From: lixianglai <lixianglai(a)loongson.cn > Add loongarch cpu support, Define new cpu type
'loongarch64'
> and implement it's driver functions.
> Signed-off-by: lixianglai <lixianglai(a)loongson.cn 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.
Ok, I'll correct it in the next version :)
> ---
> 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.
Ok, I think it is because I did not install cppi that the problem was
not detected during 'meson test'.
I will install cppi and conduct 'meson test' again for each patch.
> +++ 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.
Ok, I'll correct it in the next version.
> +++ 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?
Well, I think I will try to refer to riscv64 for cpu driver
implementation in the next version.
> +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.
Okay, so the hard coding here is a little bit inappropriate, and I feel
like I can do without the complexity,
I'm not sure, but I can try to simplify this.
> +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.
Ok, I will do a code check according to the suggestion.
> 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...
Ok, I'll fix it in the next version.
> +++ 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.
Ok, I'll fix it in the next version.
Thanks,
Xianglai.