From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33420) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zvbia-0005Mf-6c for qemu-devel@nongnu.org; Sun, 08 Nov 2015 20:58:02 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZvbiV-0000k9-IA for qemu-devel@nongnu.org; Sun, 08 Nov 2015 20:58:00 -0500 Received: from mail-pa0-x236.google.com ([2607:f8b0:400e:c03::236]:34977) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZvbiV-0000jv-6e for qemu-devel@nongnu.org; Sun, 08 Nov 2015 20:57:55 -0500 Received: by pasz6 with SMTP id z6so186957694pas.2 for ; Sun, 08 Nov 2015 17:57:54 -0800 (PST) References: <1446678366-15082-1-git-send-email-sukadev@linux.vnet.ibm.com> From: Alexey Kardashevskiy Message-ID: <563FFD9C.7070407@ozlabs.ru> Date: Mon, 9 Nov 2015 12:57:48 +1100 MIME-Version: 1.0 In-Reply-To: <1446678366-15082-1-git-send-email-sukadev@linux.vnet.ibm.com> Content-Type: text/plain; charset=koi8-r; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 1/1] target-ppc: Implement rtas_get_sysparm(PROCESSOR_MODULE_INFO) List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sukadev Bhattiprolu , agraf@suse.de, benh@au1.ibm.com, david@gibson.dropbear.id.au, paulus@au1.ibm.com, stewart@linux.vnet.ibm.com Cc: nacc@linux.vnet.ibm.com, qemu-ppc@nongnu.org, qemu-devel@nongnu.org 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 > --- > 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 > @@ -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 > +#include > +#include > +#include > +#include > +#include > + > +#include > +#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