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

[Alexey Kardashevskiy <aik@ozlabs.ru>]

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 <sukadev@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