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

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

[Suraj Jitindar Singh]

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>
---
 hw/ppc/spapr_rtas.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index 5bc1a93271..a33d87794c 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -229,6 +229,40 @@ static inline int sysparm_st(target_ulong addr, target_ulong len,
     return RTAS_OUT_SUCCESS;
 }
 
+static int rtas_get_num_host_threads(void)
+{
+    const char *entry, *name = "/proc/device-tree/cpus/";
+    int num_threads = -1;
+    GDir *dir;
+
+    if (!kvm_enabled())
+        return 1;
+
+    dir = g_dir_open(name, 0, NULL);
+    if (!dir)
+        return -1;
+
+    while ((entry = g_dir_read_name(dir))) {
+        if (!strncmp(entry, "PowerPC,POWER", strlen("PowerPC,POWER"))) {
+            unsigned long len;
+            char *path, *buf;
+
+            path = g_strconcat(name, 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);
+    return num_threads;
+}
+
 static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu,
                                           SpaprMachineState *spapr,
                                           uint32_t token, uint32_t nargs,
@@ -250,6 +284,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) {
+            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;
-- 
2.13.6

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

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190624013921.11944-1-sjitindarsingh@gmail.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [Qemu-devel] [QEMU-PPC] [PATCH] powerpc/spapr: Add host threads parameter to ibm, get_system_parameter
Type: series
Message-id: 20190624013921.11944-1-sjitindarsingh@gmail.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

From https://github.com/patchew-project/qemu
 * [new tag]               patchew/20190624013921.11944-1-sjitindarsingh@gmail.com -> patchew/20190624013921.11944-1-sjitindarsingh@gmail.com
Switched to a new branch 'test'
285da11abd powerpc/spapr: Add host threads parameter to ibm, get_system_parameter

=== OUTPUT BEGIN ===
ERROR: braces {} are necessary for all arms of this statement
#36: FILE: hw/ppc/spapr_rtas.c:238:
+    if (!kvm_enabled())
[...]

ERROR: braces {} are necessary for all arms of this statement
#40: FILE: hw/ppc/spapr_rtas.c:242:
+    if (!dir)
[...]

total: 2 errors, 0 warnings, 56 lines checked

Commit 285da11abde5 (powerpc/spapr: Add host threads parameter to ibm, get_system_parameter) has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20190624013921.11944-1-sjitindarsingh@gmail.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

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

[Greg Kurz]

On Mon, 24 Jun 2019 11:39:21 +1000
Suraj Jitindar Singh <sjitindarsingh@gmail.com> 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.
> 

Shouldn't this also take split core into account, ie. divide the
result by "/sys/devices/system/cpu/subcores_per_core" like the 
ppc64_cpu command from powerpc-utils does ?

> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> ---
>  hw/ppc/spapr_rtas.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index 5bc1a93271..a33d87794c 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -229,6 +229,40 @@ static inline int sysparm_st(target_ulong addr, target_ulong len,
>      return RTAS_OUT_SUCCESS;
>  }
>  
> +static int rtas_get_num_host_threads(void)
> +{
> +    const char *entry, *name = "/proc/device-tree/cpus/";
> +    int num_threads = -1;
> +    GDir *dir;
> +
> +    if (!kvm_enabled())
> +        return 1;
> +
> +    dir = g_dir_open(name, 0, NULL);
> +    if (!dir)
> +        return -1;
> +
> +    while ((entry = g_dir_read_name(dir))) {
> +        if (!strncmp(entry, "PowerPC,POWER", strlen("PowerPC,POWER"))) {
> +            unsigned long len;
> +            char *path, *buf;
> +
> +            path = g_strconcat(name, 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);
> +    return num_threads;
> +}
> +
>  static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu,
>                                            SpaprMachineState *spapr,
>                                            uint32_t token, uint32_t nargs,
> @@ -250,6 +284,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) {
> +            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;

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

[Suraj Jitindar Singh]

On Mon, 2019-06-24 at 10:37 +0200, Greg Kurz wrote:
> On Mon, 24 Jun 2019 11:39:21 +1000
> Suraj Jitindar Singh <sjitindarsingh@gmail.com> 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.
> > 
> 
> Shouldn't this also take split core into account, ie. divide the
> result by "/sys/devices/system/cpu/subcores_per_core" like the 
> ppc64_cpu command from powerpc-utils does ?

That makes sense, I'll modify the code to account for that.

Thanks,
Suraj

> 
> > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> > ---
> >  hw/ppc/spapr_rtas.c | 44
> > ++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 44 insertions(+)
> > 
> > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> > index 5bc1a93271..a33d87794c 100644
> > --- a/hw/ppc/spapr_rtas.c
> > +++ b/hw/ppc/spapr_rtas.c
> > @@ -229,6 +229,40 @@ static inline int sysparm_st(target_ulong
> > addr, target_ulong len,
> >      return RTAS_OUT_SUCCESS;
> >  }
> >  
> > +static int rtas_get_num_host_threads(void)
> > +{
> > +    const char *entry, *name = "/proc/device-tree/cpus/";
> > +    int num_threads = -1;
> > +    GDir *dir;
> > +
> > +    if (!kvm_enabled())
> > +        return 1;
> > +
> > +    dir = g_dir_open(name, 0, NULL);
> > +    if (!dir)
> > +        return -1;
> > +
> > +    while ((entry = g_dir_read_name(dir))) {
> > +        if (!strncmp(entry, "PowerPC,POWER",
> > strlen("PowerPC,POWER"))) {
> > +            unsigned long len;
> > +            char *path, *buf;
> > +
> > +            path = g_strconcat(name, 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);
> > +    return num_threads;
> > +}
> > +
> >  static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu,
> >                                            SpaprMachineState
> > *spapr,
> >                                            uint32_t token, uint32_t
> > nargs,
> > @@ -250,6 +284,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) {
> > +            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;
> 
>