On Thu, Dec 14, 2023 at 02:08:45PM +0800, xianglai li wrote: > From: lixianglai <lixiang...@loongson.cn> > > Add loongarch cpu support, Define new cpu type 'loongarch64' > and implement it's driver functions. > > Signed-off-by: lixianglai <lixiang...@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. > --- > 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@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org