qemu-devel
[Top][All Lists]
Advanced

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

Re: [Qemu-devel] [PATCH v2 1/1] target-ppc: Implement rtas_get_sysparm(P


From: Alexey Kardashevskiy
Subject: Re: [Qemu-devel] [PATCH v2 1/1] target-ppc: Implement rtas_get_sysparm(PROCESSOR_MODULE_INFO)
Date: Mon, 9 Nov 2015 12:57:48 +1100
User-agent: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0

On 11/05/2015 10:06 AM, Sukadev Bhattiprolu wrote:
Implement RTAS_SYSPARM_PROCESSOR_MODULE_INFO parameter to rtas_get_sysparm()
call in qemu. This call returns the processor module (socket), chip and core
information as specified in section 7.3.16.18 of PAPR v2.7.

We walk the /proc/device-tree to determine the number of chips, cores and
modules in the _host_ system and return this info to the guest application
that makes the rtas_get_sysparm() call.

We currently hard code the number of module_types to 1, but we should determine
that dynamically somehow later.

Thanks to input from Nishanth Aravamudan and Alexey Kardashevsk.

"iy" is missing :)



Signed-off-by: Sukadev Bhattiprolu <address@hidden>
---
Changelog[v2]:
         [Alexey Kardashevsk] Use existing interfaces like ldl_be_p(),
                 stw_be_phys(), g_hash_table_new_full(), error_report() rather
                 than re-inventing; fix indentation, function prottypes etc;
                 Drop the fts() interface (which doesn't seem to be available
                 on mingw32/mingw64) and use opendir() to walk specific
                 directories in the directory tree.
---
  hw/ppc/Makefile.objs        |   1 +
  hw/ppc/spapr_rtas.c         |  35 +++++++
  hw/ppc/spapr_rtas_modinfo.c | 230 ++++++++++++++++++++++++++++++++++++++++++++
  hw/ppc/spapr_rtas_modinfo.h |  12 +++
  include/hw/ppc/spapr.h      |   1 +
  5 files changed, 279 insertions(+)
  create mode 100644 hw/ppc/spapr_rtas_modinfo.c
  create mode 100644 hw/ppc/spapr_rtas_modinfo.h

diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
index c1ffc77..57c6b02 100644
--- a/hw/ppc/Makefile.objs
+++ b/hw/ppc/Makefile.objs
@@ -4,6 +4,7 @@ obj-y += ppc.o ppc_booke.o
  obj-$(CONFIG_PSERIES) += spapr.o spapr_vio.o spapr_events.o
  obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
  obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
+obj-$(CONFIG_PSERIES) += spapr_rtas_modinfo.o
  ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
  obj-y += spapr_pci_vfio.o
  endif
diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index 34b12a3..41fd8a6 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -34,6 +34,8 @@
  #include "hw/ppc/spapr.h"
  #include "hw/ppc/spapr_vio.h"
  #include "qapi-event.h"
+
+#include "spapr_rtas_modinfo.h"
  #include "hw/boards.h"

  #include <libfdt.h>
@@ -240,6 +242,39 @@ static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu,
      target_ulong ret = RTAS_OUT_SUCCESS;

      switch (parameter) {
+    case RTAS_SYSPARM_PROCESSOR_MODULE_INFO: {
+        int i;
+        int offset = 0;
+        int size;

Nit - could be one line.


+        struct rtas_module_info modinfo;
+
+        if (rtas_get_module_info(&modinfo)) {
+            break;


@ret will be still 0 in this case, set @ret to RTAS_OUT_HW_ERROR here.

Also, rtas_ibm_set_system_parameter() must return RTAS_OUT_NOT_AUTHORIZED on RTAS_SYSPARM_PROCESSOR_MODULE_INFO, as PAPR says.


+        }
+
+        size = sizeof(modinfo);
+        size += (modinfo.module_types - 1) * sizeof(struct rtas_socket_info);
+
+        stw_be_phys(&address_space_memory, buffer+offset, size);
+        offset += 2;
+
+        stw_be_phys(&address_space_memory, buffer+offset, 
modinfo.module_types);
+        offset += 2;
+
+        for (i = 0; i < modinfo.module_types; i++) {
+            stw_be_phys(&address_space_memory, buffer+offset,
+                                    modinfo.si[i].sockets);


checkpatch.pl does not warn on this but new lines start under opening brace in the previous line.

In terms on vim, it would be:
set expandtab
set tabstop=4
set shiftwidth=4
set cino=:0,(0



+            offset += 2;
+            stw_be_phys(&address_space_memory, buffer+offset,
+                                    modinfo.si[i].chips);
+            offset += 2;
+            stw_be_phys(&address_space_memory, buffer+offset,
+                                    modinfo.si[i].cores_per_chip);
+            offset += 2;
+        }
+        break;
+    }
+
      case RTAS_SYSPARM_SPLPAR_CHARACTERISTICS: {
          char *param_val = g_strdup_printf("MaxEntCap=%d,"
                                            "DesMem=%llu,"
diff --git a/hw/ppc/spapr_rtas_modinfo.c b/hw/ppc/spapr_rtas_modinfo.c
new file mode 100644
index 0000000..068fc2c
--- /dev/null
+++ b/hw/ppc/spapr_rtas_modinfo.c
@@ -0,0 +1,230 @@
+/*
+ * QEMU PowerPC pSeries Logical Partition (aka sPAPR) hardware System Emulator
+ *
+ * Hypercall based emulated RTAS


This is a description of hw/ppc/spapr_rtas.c, not of the new file.

Not sure the new code deserves a separate file, I'd either:
- add it into hw/ppc/spapr_rtas.c OR
- move rtas_ibm_get_system_parameter() + rtas_ibm_set_system_parameter() to a separate file (let's call it hw/ppc/spapr_rtas_syspar.c) and add this new parameter there as there will be new parameters in the future anyway.

But I'll leave to the maintainer (David, hi :) ).



+ *
+ * Copyright (c) 2015 Sukadev Bhattiprolu, IBM Corporation.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ *
+ */
+#include <stdio.h>
+#include <dirent.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <string.h>
+#include <errno.h>
+
+#include <glib.h>
+#include "spapr_rtas_modinfo.h"
+#include "qemu/error-report.h"
+#include "qemu/bswap.h"
+
+static int file_read_buf(char *file_name, char *buf, int len)
+{
+    int rc;
+    FILE *fp;
+
+    fp = fopen(file_name, "r");
+    if (!fp) {
+        error_report("%s: Error opening %s\n", __func__, file_name);


error_report() does not need "\n", remote it here and below.

Another thing - you passed __func__ here but you did not in other places, please make this consistent and pass __func__ everywhere OR make error messages more descriptive, the latter seems to be preferable way in QEMU, either way this should be easily grep'able, for example - "Unable to open %s, error %d" is too generic.


+        return -1;
+    }
+
+    rc = fread(buf, 1, len, fp);
+    fclose(fp);
+
+    if (rc != len) {
+        return -1;
+    }
+
+    return 0;
+}
+
+/*
+ * Read an identifier from the file @path and add the identifier
+ * to the hash table @gt unless its already in the table.
+ */
+static int hash_table_add_contents(GHashTable *gt, char *path)

Move this helper before count_modules_chips(). I thought for a sec that count_cores() uses it too :)


+{
+    int idx;
+    char buf[16];
+    int *key;
+
+    if (file_read_buf(path, buf, sizeof(int))) {
+        return -1;
+    }
+
+    idx = ldl_be_p(buf);
+
+    if (g_hash_table_contains(gt, &idx)) {
+        return 0;
+    }
+
+    key = g_malloc(sizeof(int));


I kind of hoped that g_direct_hash() (and probably GINT_TO_POINTER() but I do not see kvm-all.c using it) will let you avoid g_malloc()'ing the keys.



+
+    *key = idx;
+    if (!g_hash_table_insert(gt, key, NULL)) {
+        error_report("Unable to insert key %d into chips hash table\n", idx);
+        g_free(key);
+        return -1;
+    }
+
+    return 0;
+}
+
+/*
+ * Each core in the system is represented by a directory with the
+ * prefix 'PowerPC,POWER' in the directory /proc/device-tree/cpus/.
+ * Process that directory and count the number of cores in the system.
+ */
+static int count_cores(int *num_cores)
+{
+    int rc;
+    DIR *dir;
+    struct dirent *ent;
+    const char *cpus_dir = "/proc/device-tree/cpus";
+    const char *ppc_prefix = "PowerPC,POWER";
+
+    dir = opendir(cpus_dir);
+    if (!dir) {
+        error_report("Unable to open %s, error %d\n", cpus_dir, errno);
+        return -1;
+    }
+
+    *num_cores = 0;
+    while (1) {
+        errno = 0;
+        ent = readdir(dir);
+        if (!ent) {

rc = -errno; ....


+            break;
+        }
+
+        if (!strncmp(ppc_prefix, ent->d_name, strlen(ppc_prefix))) {
+            (*num_cores)++;
+        }
+    }
+
+    rc = 0;
+    if (errno) {
+        rc = -1;
+    }


... and remove these 4 lines above?



+
+    closedir(dir);
+    return rc;
+}
+
+/*
+ * Each module's (aka socket's) id is contained in the 'ibm,hw-module-id'
+ * file in the "xscom" directory (/proc/device-tree/xscom*). Similarly each
+ * chip's id is contained in the 'ibm,chip-id' file in the xscom directory.
+ *
+ * A module can contain more than one chip and a chip can contain more
+ * than one core. So there are likely to be duplicates in the module
+ * and chip identifiers (i.e more than one xscom directory can contain
+ * the same module/chip id).
+ *
+ * Search the xscom directories and count the number of _UNIQUE_ module
+ * and chip identifiers in the system.
+ */
+static int count_modules_chips(int *num_modules, int *num_chips)
+{
+    int rc;

int rc = -1;


+    DIR *dir;
+    struct dirent *ent;
+    char path[PATH_MAX];
+    const char *xscom_dir = "/proc/device-tree";
+    const char *xscom_prefix = "xscom@";
+    GHashTable *chips_tbl, *modules_tbl;
+
+    dir = opendir(xscom_dir);
+    if (!dir) {
+        error_report("Unable to open %s, error %d\n", xscom_dir, errno);
+        return -1;
+    }
+
+    modules_tbl = g_hash_table_new_full(g_int_hash, g_int_equal, g_free, NULL);
+    chips_tbl = g_hash_table_new_full(g_int_hash, g_int_equal, g_free, NULL);
+
+    rc = -1;


Remove this.

+    while (1) {
+        errno = 0;
+        ent = readdir(dir);
+        if (!ent) {

Add this:
rc = -errno;
goto cleanup;

+            break;
+        }
+
+        if (strncmp(xscom_prefix, ent->d_name, strlen(xscom_prefix))) {
+            continue;
+        }
+
+        sprintf(path, "%s/%s/ibm,chip-id", xscom_dir, ent->d_name);
+        if (hash_table_add_contents(chips_tbl, path)) {
+            goto cleanup;
+        }
+
+        sprintf(path, "%s/%s/ibm,hw-module-id", xscom_dir, ent->d_name);
+        if (hash_table_add_contents(modules_tbl, path)) {
+            goto cleanup;
+        }
+    }
+
+    if (errno) {
+        goto cleanup;
+    }

Do not need these 3 lines.


+
+    *num_modules = g_hash_table_size(modules_tbl);
+    *num_chips = g_hash_table_size(chips_tbl);
+    rc = 0;


Remove this.

count_modules_chips() and count_cores() do pretty similar things, it would improve the code if error handling was done similar ways.


+
+cleanup:
+    g_hash_table_remove_all(modules_tbl);
+    g_hash_table_destroy(modules_tbl);
+
+    g_hash_table_remove_all(chips_tbl);
+    g_hash_table_destroy(chips_tbl);
+
+    closedir(dir);
+
+    return rc;
+}
+
+int rtas_get_module_info(struct rtas_module_info *modinfo)
+{
+    int cores;
+    int chips;
+    int modules;

Nit - could be one line.


+
+    memset(modinfo, 0, sizeof(struct rtas_module_info));
+
+    if (count_modules_chips(&modules, &chips) || count_cores(&cores)) {
+        return -1;
+    }
+
+    /*
+     * TODO: Hard code number of module_types for now till
+     *       we figure out how to determine it dynamically.
+     */
+    modinfo->module_types = 1;
+    modinfo->si[0].sockets = modules;


A "module" here is what modinfo describes so I'd expect @modules to be "1" in the existing code and count_modules_chips() to be named count_sockets_chips() and return @sockets, not @modules.

When exactly does a socket become a module? The SPAPR spec uses "sockets" here.


+    modinfo->si[0].chips = chips;
+    modinfo->si[0].cores_per_chip = cores / chips;


What if no "ibm,chip-id" was found and chips == 0?


+
+    return 0;
+}
diff --git a/hw/ppc/spapr_rtas_modinfo.h b/hw/ppc/spapr_rtas_modinfo.h
new file mode 100644
index 0000000..356a2b5
--- /dev/null
+++ b/hw/ppc/spapr_rtas_modinfo.h


This file is missing a license and
#ifndef SPAPR_RTAS_MODINFO_H
#define SPAPR_RTAS_MODINFO_H
#endif

But I'd rather put the content to include/hw/ppc/spapr.h and avoid creating new files.


@@ -0,0 +1,12 @@
+
+struct rtas_socket_info {
+    unsigned short sockets;
+    unsigned short chips;
+    unsigned short cores_per_chip;
+};
+struct rtas_module_info {
+    unsigned short module_types;
+    struct rtas_socket_info si[1];
+};


Will the number of rtas_socket_info be always a constant or (sure, in the future) it may take different values?

Normally when you invent API like rtas_get_module_info(), you tell the helper how much memory is allocated in the rtas_module_info struct (in our case it is ARRAY_SIZE(rtas_module_info.si) which is fine) and then the helper signals somehow how many module types it has found (which is missing here).


+
+extern int rtas_get_module_info(struct rtas_module_info *topo);
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 5baa906..cfe7fa2 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -463,6 +463,7 @@ int spapr_allocate_irq_block(int num, bool lsi, bool msi);
  /* RTAS ibm,get-system-parameter token values */
  #define RTAS_SYSPARM_SPLPAR_CHARACTERISTICS      20
  #define RTAS_SYSPARM_DIAGNOSTICS_RUN_MODE        42
+#define RTAS_SYSPARM_PROCESSOR_MODULE_INFO       43
  #define RTAS_SYSPARM_UUID                        48

  /* RTAS indicator/sensor types



--
Alexey



reply via email to

[Prev in Thread] Current Thread [Next in Thread]