Re: [Qemu-devel] [QEMU-PPC] [PATCH v3] powerpc/spapr: Add host threads parameter to ibm, get_system_parameter

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5164 bytes --]

On Mon, Jul 01, 2019 at 04:19:46PM +1000, Suraj Jitindar Singh wrote:
> The ibm,get_system_parameter rtas call is used by the guest to retrieve
> data relating to certain parameters of the system. The SPLPAR
> characteristics option (token 20) is used to determin characteristics of
> the environment in which the lpar will run.
> 
> It may be useful for a guest to know the number of physical host threads
> present on the underlying system where it is being run. Add the
> characteristic "HostThrs" to the SPLPAR Characteristics
> ibm,get_system_parameter rtas call to expose this information to a
> guest and provide an implementation which determines this information
> based on the number of interrupt servers present in the device tree.
> 
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>

Hrm, as I said on our call, I have some misgivings about this.

Starting with the most general: this again publishes host information
to the guest without filtering, which has caused us problems before
(e.g. security issues with publishing the host serial and model
information).  Now, I can't immediately see what harm a guest could do
with the host # threads (especially since it could in theory deduce it
from the PVR, I think) but it still makes me uneasy.

Secondly, the "HostThrs" tag doesn't seem to be documented in PAPR as
something that this system-parameter will include.  I don't much like
the idea of adding ad-hoc bits of information here without some
thought going into designing and specifying it first.

> 
> ---
> 
> V1 -> V2:
> - Take into account that the core may be operating in split core mode
>   meaning a single core may be split into multiple subcores.
> V2 -> V3:
> - Add curly braces for single line if statements
> ---
>  hw/ppc/spapr_rtas.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 62 insertions(+)
> 
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index 5bc1a93271..1bab71c90c 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -229,6 +229,58 @@ static inline int sysparm_st(target_ulong addr, target_ulong len,
>      return RTAS_OUT_SUCCESS;
>  }
>  
> +#define CPUS_PATH       "/proc/device-tree/cpus/"
> +#define SUBCORE_PATH    "/sys/devices/system/cpu/subcores_per_core"
> +
> +static int rtas_get_num_host_threads(void)
> +{
> +    int num_threads = -1;
> +    unsigned long len;
> +    const char *entry;
> +    char *buf;
> +    GDir *dir;
> +
> +    if (!kvm_enabled()) {
> +        return 1;
> +    }
> +
> +    /* Read interrupt servers to determine number of threads per core */
> +    dir = g_dir_open(CPUS_PATH, 0, NULL);
> +    if (!dir) {
> +        return -1;
> +    }
> +
> +    while ((entry = g_dir_read_name(dir))) {
> +        if (!strncmp(entry, "PowerPC,POWER", strlen("PowerPC,POWER"))) {
> +            char *path;
> +
> +            path = g_strconcat(CPUS_PATH, entry, "/ibm,ppc-interrupt-server#s",
> +                               NULL);
> +            if (g_file_get_contents(path, &buf, &len, NULL)) {
> +                num_threads = len / sizeof(int);
> +                g_free(buf);
> +            }
> +
> +            g_free(path);
> +            break;
> +        }
> +    }
> +
> +    g_dir_close(dir);
> +
> +    /* Check if split core mode in use */
> +    if (g_file_get_contents(SUBCORE_PATH, &buf, &len, NULL)) {
> +        int subcores = g_ascii_strtoll(buf, NULL, 10);
> +
> +        if (subcores) {
> +            num_threads /= subcores;
> +        }
> +        g_free(buf);
> +    }

Finally, all the logic above is built on the assumption of a ppc host
- and not just that but an IBM POWER host...

> +    return num_threads;
> +}
> +
>  static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu,
>                                            SpaprMachineState *spapr,
>                                            uint32_t token, uint32_t nargs,
> @@ -250,6 +302,16 @@ static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu,
>                                            current_machine->ram_size / MiB,
>                                            smp_cpus,
>                                            max_cpus);
> +        int num_host_threads = rtas_get_num_host_threads();
> +
> +        if (num_host_threads > 0) {

... this sort of implements a fallback in other cases (KVM PR with a
non-IBM host, TCG, but the boundary conditions are not really well defined.

> +            char *hostthr_val, *old = param_val;
> +
> +            hostthr_val = g_strdup_printf(",HostThrs=%d", num_host_threads);
> +            param_val = g_strconcat(param_val, hostthr_val, NULL);
> +            g_free(hostthr_val);
> +            g_free(old);
> +        }
>          ret = sysparm_st(buffer, length, param_val, strlen(param_val) + 1);
>          g_free(param_val);
>          break;

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]