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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Andrea:

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.


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.



_______________________________________________
Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux