Re: [libvirt PATCH V2 1/4] Add loongarch cpu support

On Wed, Jan 10, 2024 at 11:07:46AM +0800, Xianglai Li wrote: > From: xianglai li<lixianglai(a)loongson.cn> Please consider adjusting your git configuration so that the authorship information matches your Signed-off-by and the email's From header.

> Add loongarch cpu support, Define new cpu type 'loongarch64' > and implement it's driver functions. > > Signed-off-by: "Xianglai Li&quot;&lt;lixianglai(a)loongson.cn&gt; > --- > src/cpu/cpu.c | 2 + > src/cpu/cpu_loongarch.c | 80 ++++++++++++++++++++++++++++++++++++ > src/cpu/cpu_loongarch.h | 25 +++++++++++ > src/cpu/meson.build | 1 + > src/qemu/qemu_capabilities.c | 2 + > src/qemu/qemu_domain.c | 4 ++ > src/util/virarch.c | 2 + > src/util/virarch.h | 4 ++ > 8 files changed, 120 insertions(+) > create mode 100644 src/cpu/cpu_loongarch.c > create mode 100644 src/cpu/cpu_loongarch.h > > diff --git a/src/cpu/cpu_loongarch.c b/src/cpu/cpu_loongarch.c > new file mode 100644 > index 0000000000..48f9fef5ea > --- /dev/null > +++ b/src/cpu/cpu_loongarch.c > @@ -0,0 +1,80 @@ > +/* > + * cpu_loongarch.c: CPU driver for 64-bit LOONGARCH CPUs > + * > + * Copyright (C) 2024 Loongson Technology. > + * > + * 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 <fcntl.h> > +#include <unistd.h> > +#include <stdint.h> > +#include <stdio.h> > +#include <sys/time.h> > +#include <sys/types.h> > +#include <sys/stat.h> > +#include "virlog.h" > +#include "viralloc.h" > +#include "cpu.h" > +#include "virstring.h" > +#include "cpu_map.h" > +#include "virbuffer.h" Most of these includes are unnecessary. You only really need <config.h>, "virlog.h" and "cpu.h".

> +#define VIR_FROM_THIS VIR_FROM_CPU > + > +VIR_LOG_INIT("cpu.cpu_loongarch"); > + > +static const virArch archs[] = { VIR_ARCH_LOONGARCH64 }; > + > +static virCPUCompareResult > +virCPULoongArchCompare(virCPUDef *host G_GNUC_UNUSED, > + virCPUDef *cpu G_GNUC_UNUSED, > + bool failIncompatible G_GNUC_UNUSED) > +{ > + return VIR_CPU_COMPARE_IDENTICAL; > +} > + > +static int > +virCPULoongArchUpdate(virCPUDef *guest, > + const virCPUDef *host ATTRIBUTE_UNUSED, G_GNUC_UNUSED

> + bool relative G_GNUC_UNUSED) > +{ > + /* > + * - host-passthrough doesn't even get here > + * - host-model is used for host CPU running in a compatibility mode and > + * it needs to remain unchanged > + * - custom doesn't support any optional features, there's nothing to > + * update > + */ This comment is lifted directly from the ppc64 CPU driver and it feels out of place here, especially the part about "compatibility mode", which is extremely ppc64 specific. I'd say just drop it.

> + if (guest->mode == VIR_CPU_MODE_CUSTOM) > + guest->match = VIR_CPU_MATCH_EXACT; This doesn't seem to be needed for a basic CPU driver. If you drop it, this will end up looking exactly the way the RISC-V CPU driver originally did, and that worked just fine until it was later improved. So I'd say just drop it. We can add more features to the CPU driver later.

> diff --git a/src/util/virarch.h b/src/util/virarch.h > index 747f77c48e..c033e5c68d 100644 > --- a/src/util/virarch.h > +++ b/src/util/virarch.h > @@ -36,6 +36,8 @@ typedef enum { > VIR_ARCH_ITANIUM, /* Itanium 64 LEhttps://en.wikipedia.org/wiki/Itanium */ > VIR_ARCH_LM32, /* MilkyMist 32 BEhttps://en.wikipedia.org/wiki/Milkymist */ > > + VIR_ARCH_LOONGARCH64, /* LoongArch 64 LE */ > + > VIR_ARCH_M68K, /* m68k 32 BEhttps://en.wikipedia.org/wiki/Motorola_68000_family */ > VIR_ARCH_MICROBLAZE, /* Microblaze 32 BEhttps://en.wikipedia.org/wiki/MicroBlaze */ > VIR_ARCH_MICROBLAZEEL, /* Microblaze 32 LEhttps://en.wikipedia.org/wiki/MicroBlaze */ The entries here are grouped five by five, so as you add loongarch64 in the middle you're going to have to reorganize all the ones coming after it. Same for the virarch.c part and preferredMachines in the QEMU driver.

There doesn't seem to be a Wikipedia entry for loongarch64 itself, but perhaps it would be appropriate to link to https://en.wikipedia.org/wiki/Loongson#LoongArch instead. There are similar examples in the file already.

> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c > index 896aa8394f..0cea0b323a 100644 > --- a/src/qemu/qemu_domain.c > +++ b/src/qemu/qemu_domain.c > @@ -4222,6 +4222,10 @@ qemuDomainDefAddDefaultDevices(virQEMUDriver *driver, > addPCIRoot = true; > break; > > + case VIR_ARCH_LOONGARCH64: > + addPCIeRoot = true; > + break; > + > case VIR_ARCH_ARMV7B: > case VIR_ARCH_CRIS: > case VIR_ARCH_ITANIUM: In this commit you're just adding the architecture, and it's good practice to touch the individual drivers as little as possible while doing so. So while you're forced to add a new case to the switch to keep things building, leave out the actual logic for now and make it a no-action entry, such as the existing ARMV7B, CRIS, etc. In a later patch, when you actually implement loongarch64 support in the QEMU driver, you can add this logic. When you do, please also set addDefaultUSB = false; addDefaultMemballoon = false; The default behavior, which is to add these devices automatically for all new domains, mainly exists to ensure backwards compatibility in the context of x86, and we've moved away from it for architectures that have been introduced more recently, such as aarch64 and RISC-V.

> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c > index 83119e871a..f2339d6013 100644 > --- a/src/qemu/qemu_capabilities.c > +++ b/src/qemu/qemu_capabilities.c > @@ -2665,6 +2665,8 @@ static const char *preferredMachines[] = > NULL, /* VIR_ARCH_ITANIUM (doesn't exist in QEMU any more) */ > "lm32-evr", /* VIR_ARCH_LM32 */ > > + "virt", /* VIR_ARCH_LOONGARCH64 */ In this case too, it would be slightly nicer if you set this to NULL as part of this patch and changed it to the actual value in the patch where QEMU support is implemented.