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 <suka...@linux.vnet.ibm.com>
---
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
