Alexey Kardashevskiy address@hidden wrote:
| 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 :)
Argh. Sorry :-)
Agree with your other comments. Couple of questions/comments below.
|
| 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.
Agree.
|
| 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 :) ).
Will discuss this on the other thread from David (maybe move to
target-ppc/kvm.c?)
|
|
<snip>
| >+
| >+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.
will change to "Error opening device tree file %s" and drop the
__func__.
|
|
| >+ 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 :)
Ok.
|
|
| >+{
| >+ 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.
I think g_direct_hash() and GINT_TO_POINTER will work. Will try them
out.
|
|
|
| >+
| >+ *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; ....
Yes. Thanks,
|
|
| >+ 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?
Ok.
|
|
|
| >+
| >+ 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.
Agree.
|
|
| >+
| >+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.
I am trying to get the terminology too :-) Is socket a slot where a
module is attached?
|
|
| >+
| >+ 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.
Ok.
|
|
| >@@ -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?
TBH, I am not sure. One thing though, array size of rtas_socket_info
depends on number of module _types_. Like Nishanth Aravamudan mentioned
offline, we are not sure if both a DCM and SCM types can be on a system
at once (or even if they can be added dynamically).