From mboxrd@z Thu Jan 1 00:00:00 1970 From: Suraj Jitindar Singh Subject: Re: [kvm-unit-tests PATCH V4 3/5] lib/powerpc: Add function to start secondary threads Date: Thu, 18 Aug 2016 13:59:10 +1000 Message-ID: <1471492750.2138.5.camel@gmail.com> References: <1471416538-14088-1-git-send-email-sjitindarsingh@gmail.com> <1471416538-14088-3-git-send-email-sjitindarsingh@gmail.com> <568b1de9-c1dc-050b-3acf-b86159cbb3f4@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit Cc: pbonzini@redhat.com, rkrcmar@redhat.com, kvm-ppc@vger.kernel.org, lvivier@redhat.com, drjones@redhat.com To: Thomas Huth , kvm@vger.kernel.org Return-path: Received: from mail-pa0-f66.google.com ([209.85.220.66]:35150 "EHLO mail-pa0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752435AbcHRD7V (ORCPT ); Wed, 17 Aug 2016 23:59:21 -0400 In-Reply-To: <568b1de9-c1dc-050b-3acf-b86159cbb3f4@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Wed, 2016-08-17 at 09:44 +0200, Thomas Huth wrote: > On 17.08.2016 08:48, Suraj Jitindar Singh wrote: > > > > Add the lib/powerpc/smp.c file and associated header files as a > > place > > to implement generic smp functionality for inclusion in tests. > > > > Add functions start_all_cpus(), start_cpu() and start_thread() to > > start > > all stopped threads of all cpus, all stopped threads of a single > > cpu or a > > single stopped thread of a guest at a given execution location, > > respectively. > > > > Signed-off-by: Suraj Jitindar Singh > > Reviewed-by: Andrew Jones > > > > --- > > > > Change Log: > > > > V2 -> V3: > > - start_thread now returns int to reflect error, success or > > failure to > >   start thread > > - start_cpu returns number of threads on cpu and number > > successfully > >   started > > - start_all_cpus checks if number of threads started == total > > number of > >   threads - 1 > > V3 -> V4: > > - Style changes > > --- > >  lib/powerpc/asm/smp.h   |  22 +++++++++++ > >  lib/powerpc/smp.c       | 102 > > ++++++++++++++++++++++++++++++++++++++++++++++++ > >  lib/ppc64/asm/smp.h     |   1 + > >  powerpc/Makefile.common |   1 + > >  4 files changed, 126 insertions(+) > >  create mode 100644 lib/powerpc/asm/smp.h > >  create mode 100644 lib/powerpc/smp.c > >  create mode 100644 lib/ppc64/asm/smp.h > > > > diff --git a/lib/powerpc/asm/smp.h b/lib/powerpc/asm/smp.h > > new file mode 100644 > > index 0000000..21940b4 > > --- /dev/null > > +++ b/lib/powerpc/asm/smp.h > > @@ -0,0 +1,22 @@ > > +#ifndef _ASMPOWERPC_SMP_H_ > > +#define _ASMPOWERPC_SMP_H_ > > + > > +#include > > + > > +extern int nr_threads; > > + > > +struct start_threads { > > + int nr_threads; > > + int nr_started; > > +}; > > + > > +typedef void (*secondary_entry_fn)(void); > > + > > +extern void halt(void); > > + > > +extern int start_thread(int cpu_id, secondary_entry_fn entry, > > uint32_t r3); > > +extern struct start_threads start_cpu(int cpu_node, > > secondary_entry_fn entry, > > +       uint32_t r3); > > +extern bool start_all_cpus(secondary_entry_fn entry, uint32_t r3); > > + > > +#endif /* _ASMPOWERPC_SMP_H_ */ > > diff --git a/lib/powerpc/smp.c b/lib/powerpc/smp.c > > new file mode 100644 > > index 0000000..caa65b9 > > --- /dev/null > > +++ b/lib/powerpc/smp.c > > @@ -0,0 +1,102 @@ > > +/* > > + * Secondary cpu support > > + * > > + * Copyright 2016 Suraj Jitindar Singh, IBM. > > + * > > + * This work is licensed under the terms of the GNU LGPL, version > > 2. > > + */ > > + > > +#include > > +#include > > +#include > > +#include > > + > > +int nr_threads; > > + > > +struct secondary_entry_data { > > + secondary_entry_fn entry; > > + uint64_t r3; > > + int nr_started; > > +}; > > + > > +/* > > + * Start stopped thread cpu_id at entry > > + * Returns: <0 on failure to start stopped cpu > > + * 0  on success > > + * >0 on cpu not in stopped state > > + */ > > +int start_thread(int cpu_id, secondary_entry_fn entry, uint32_t > > r3) > > +{ > > + int query_token, start_token, outputs[1], ret; > > + > > + query_token = rtas_token("query-cpu-stopped-state"); > > + start_token = rtas_token("start-cpu"); > > + assert(query_token != RTAS_UNKNOWN_SERVICE && > > + start_token != RTAS_UNKNOWN_SERVICE); > One TAB too much before start_token ? AFAIK the kernel coding style doesn't address how to indent a broken line. I personally prefer a double indent or aligning it to where the argument list starts on the line above if possible so when this is in a large block of code it's obvious that this is a broken line rather than the next level of code indentation. Since this statement is followed by a blank line it's unlikely to lead to confusion - I'll go with the single indent. > > > > > + ret = rtas_call(query_token, 1, 2, outputs, cpu_id); > > + if (ret) /* rtas query call failed */ > The comment above is somewhat redundant - the printf statement below > explains the code quite well already. > Also, according to the kernel coding style (which we use for > kvm-unit-tests), I think you should use curly braces for both > branches > of a conditional statement if they are required at least for one of > the > branches (see chapter 3, right before chapter 3.1). Yep > > > > > + printf("query-cpu-stopped-state failed for cpu > > %d\n", cpu_id); > > + else if (!outputs[0]) { /* cpu in stopped state */ > > + ret = rtas_call(start_token, 3, 1, NULL, cpu_id, > > entry, r3); > > + if (ret) /* rtas start-cpu call failed */ > > + printf("failed to start cpu %d\n", > > cpu_id); > > + } else /* cpu not in stopped state */ > > + ret = outputs[0]; > That branch should also get some curly braces. Ditto > > > > > + > > + return ret; > > +} > > + > > +/* > > + * Start all stopped threads (vcpus) on cpu_node > > + * Returns: Number of stopped cpus which were successfully started > > + */ > > +struct start_threads start_cpu(int cpu_node, secondary_entry_fn > > entry, > > +        uint32_t r3) > > +{ > > + int len, i, nr_threads, nr_started = 0; > > + const struct fdt_property *prop; > > + u32 *threads; > > + > > + /* Get the id array of threads on this cpu_node */ > > + prop = fdt_get_property(dt_fdt(), cpu_node, > > + "ibm,ppc-interrupt-server#s", &len); > > + assert(prop); > > + > > + nr_threads = len >> 2; /* Divide by 4 since 4 bytes per > > thread */ > > + threads = (u32 *)prop->data; /* Array of valid ids */ > > + > > + for (i = 0; i < nr_threads; i++) { > > + if (!start_thread(fdt32_to_cpu(threads[i]), entry, > > r3)) > > + nr_started++; > > + } > > + > > + return (struct start_threads) {nr_threads, nr_started}; > Add maybe a space around each curly brace? Ok > > > > > +} > > + > > +static void start_each_secondary(int fdtnode, u32 regval __unused, > > void *info) > > +{ > > + struct secondary_entry_data *datap = info; > > + struct start_threads ret = start_cpu(fdtnode, datap- > > >entry, datap->r3); > > + > > + nr_threads += ret.nr_threads; > > + datap->nr_started += ret.nr_started; > > +} > > + > > +/* > > + * Start all stopped cpus on the guest at entry with register 3 > > set to r3 > > + * We expect that we come in with only one thread currently > > started > > + * Returns: TRUE on success > > + * FALSE on failure > > + */ > > +bool start_all_cpus(secondary_entry_fn entry, uint32_t r3) > > +{ > > + struct secondary_entry_data data = { entry, r3, 0 > > }; > > + int ret; > > + > > + ret = dt_for_each_cpu_node(start_each_secondary, &data); > > + assert(ret == 0); > > + > > + /* We expect that we come in with one thread already > > started */ > > + return data.nr_started == nr_threads - 1; > > +} > > diff --git a/lib/ppc64/asm/smp.h b/lib/ppc64/asm/smp.h > > new file mode 100644 > > index 0000000..67ced75 > > --- /dev/null > > +++ b/lib/ppc64/asm/smp.h > > @@ -0,0 +1 @@ > > +#include "../../powerpc/asm/smp.h" > > diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common > > index 404194b..677030a 100644 > > --- a/powerpc/Makefile.common > > +++ b/powerpc/Makefile.common > > @@ -37,6 +37,7 @@ cflatobjs += lib/powerpc/setup.o > >  cflatobjs += lib/powerpc/rtas.o > >  cflatobjs += lib/powerpc/processor.o > >  cflatobjs += lib/powerpc/handlers.o > > +cflatobjs += lib/powerpc/smp.o > >   > >  FLATLIBS = $(libcflat) $(LIBFDT_archive) > >  %.elf: CFLAGS += $(arch_CFLAGS) > Just some cosmetic nits ... it would be nice if you'd address them in > case you respin, but I'm also fine with the patch in the current > shape > already, so: > > Reviewed-by: Thomas Huth > Thanks From mboxrd@z Thu Jan 1 00:00:00 1970 From: Suraj Jitindar Singh Date: Thu, 18 Aug 2016 03:59:10 +0000 Subject: Re: [kvm-unit-tests PATCH V4 3/5] lib/powerpc: Add function to start secondary threads Message-Id: <1471492750.2138.5.camel@gmail.com> List-Id: References: <1471416538-14088-1-git-send-email-sjitindarsingh@gmail.com> <1471416538-14088-3-git-send-email-sjitindarsingh@gmail.com> <568b1de9-c1dc-050b-3acf-b86159cbb3f4@redhat.com> In-Reply-To: <568b1de9-c1dc-050b-3acf-b86159cbb3f4@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: Thomas Huth , kvm@vger.kernel.org Cc: pbonzini@redhat.com, rkrcmar@redhat.com, kvm-ppc@vger.kernel.org, lvivier@redhat.com, drjones@redhat.com On Wed, 2016-08-17 at 09:44 +0200, Thomas Huth wrote: > On 17.08.2016 08:48, Suraj Jitindar Singh wrote: > > > > Add the lib/powerpc/smp.c file and associated header files as a > > place > > to implement generic smp functionality for inclusion in tests. > > > > Add functions start_all_cpus(), start_cpu() and start_thread() to > > start > > all stopped threads of all cpus, all stopped threads of a single > > cpu or a > > single stopped thread of a guest at a given execution location, > > respectively. > > > > Signed-off-by: Suraj Jitindar Singh > > Reviewed-by: Andrew Jones > > > > --- > > > > Change Log: > > > > V2 -> V3: > > - start_thread now returns int to reflect error, success or > > failure to > >   start thread > > - start_cpu returns number of threads on cpu and number > > successfully > >   started > > - start_all_cpus checks if number of threads started = total > > number of > >   threads - 1 > > V3 -> V4: > > - Style changes > > --- > >  lib/powerpc/asm/smp.h   |  22 +++++++++++ > >  lib/powerpc/smp.c       | 102 > > ++++++++++++++++++++++++++++++++++++++++++++++++ > >  lib/ppc64/asm/smp.h     |   1 + > >  powerpc/Makefile.common |   1 + > >  4 files changed, 126 insertions(+) > >  create mode 100644 lib/powerpc/asm/smp.h > >  create mode 100644 lib/powerpc/smp.c > >  create mode 100644 lib/ppc64/asm/smp.h > > > > diff --git a/lib/powerpc/asm/smp.h b/lib/powerpc/asm/smp.h > > new file mode 100644 > > index 0000000..21940b4 > > --- /dev/null > > +++ b/lib/powerpc/asm/smp.h > > @@ -0,0 +1,22 @@ > > +#ifndef _ASMPOWERPC_SMP_H_ > > +#define _ASMPOWERPC_SMP_H_ > > + > > +#include > > + > > +extern int nr_threads; > > + > > +struct start_threads { > > + int nr_threads; > > + int nr_started; > > +}; > > + > > +typedef void (*secondary_entry_fn)(void); > > + > > +extern void halt(void); > > + > > +extern int start_thread(int cpu_id, secondary_entry_fn entry, > > uint32_t r3); > > +extern struct start_threads start_cpu(int cpu_node, > > secondary_entry_fn entry, > > +       uint32_t r3); > > +extern bool start_all_cpus(secondary_entry_fn entry, uint32_t r3); > > + > > +#endif /* _ASMPOWERPC_SMP_H_ */ > > diff --git a/lib/powerpc/smp.c b/lib/powerpc/smp.c > > new file mode 100644 > > index 0000000..caa65b9 > > --- /dev/null > > +++ b/lib/powerpc/smp.c > > @@ -0,0 +1,102 @@ > > +/* > > + * Secondary cpu support > > + * > > + * Copyright 2016 Suraj Jitindar Singh, IBM. > > + * > > + * This work is licensed under the terms of the GNU LGPL, version > > 2. > > + */ > > + > > +#include > > +#include > > +#include > > +#include > > + > > +int nr_threads; > > + > > +struct secondary_entry_data { > > + secondary_entry_fn entry; > > + uint64_t r3; > > + int nr_started; > > +}; > > + > > +/* > > + * Start stopped thread cpu_id at entry > > + * Returns: <0 on failure to start stopped cpu > > + * 0  on success > > + * >0 on cpu not in stopped state > > + */ > > +int start_thread(int cpu_id, secondary_entry_fn entry, uint32_t > > r3) > > +{ > > + int query_token, start_token, outputs[1], ret; > > + > > + query_token = rtas_token("query-cpu-stopped-state"); > > + start_token = rtas_token("start-cpu"); > > + assert(query_token != RTAS_UNKNOWN_SERVICE && > > + start_token != RTAS_UNKNOWN_SERVICE); > One TAB too much before start_token ? AFAIK the kernel coding style doesn't address how to indent a broken line. I personally prefer a double indent or aligning it to where the argument list starts on the line above if possible so when this is in a large block of code it's obvious that this is a broken line rather than the next level of code indentation. Since this statement is followed by a blank line it's unlikely to lead to confusion - I'll go with the single indent. > > > > > + ret = rtas_call(query_token, 1, 2, outputs, cpu_id); > > + if (ret) /* rtas query call failed */ > The comment above is somewhat redundant - the printf statement below > explains the code quite well already. > Also, according to the kernel coding style (which we use for > kvm-unit-tests), I think you should use curly braces for both > branches > of a conditional statement if they are required at least for one of > the > branches (see chapter 3, right before chapter 3.1). Yep > > > > > + printf("query-cpu-stopped-state failed for cpu > > %d\n", cpu_id); > > + else if (!outputs[0]) { /* cpu in stopped state */ > > + ret = rtas_call(start_token, 3, 1, NULL, cpu_id, > > entry, r3); > > + if (ret) /* rtas start-cpu call failed */ > > + printf("failed to start cpu %d\n", > > cpu_id); > > + } else /* cpu not in stopped state */ > > + ret = outputs[0]; > That branch should also get some curly braces. Ditto > > > > > + > > + return ret; > > +} > > + > > +/* > > + * Start all stopped threads (vcpus) on cpu_node > > + * Returns: Number of stopped cpus which were successfully started > > + */ > > +struct start_threads start_cpu(int cpu_node, secondary_entry_fn > > entry, > > +        uint32_t r3) > > +{ > > + int len, i, nr_threads, nr_started = 0; > > + const struct fdt_property *prop; > > + u32 *threads; > > + > > + /* Get the id array of threads on this cpu_node */ > > + prop = fdt_get_property(dt_fdt(), cpu_node, > > + "ibm,ppc-interrupt-server#s", &len); > > + assert(prop); > > + > > + nr_threads = len >> 2; /* Divide by 4 since 4 bytes per > > thread */ > > + threads = (u32 *)prop->data; /* Array of valid ids */ > > + > > + for (i = 0; i < nr_threads; i++) { > > + if (!start_thread(fdt32_to_cpu(threads[i]), entry, > > r3)) > > + nr_started++; > > + } > > + > > + return (struct start_threads) {nr_threads, nr_started}; > Add maybe a space around each curly brace? Ok > > > > > +} > > + > > +static void start_each_secondary(int fdtnode, u32 regval __unused, > > void *info) > > +{ > > + struct secondary_entry_data *datap = info; > > + struct start_threads ret = start_cpu(fdtnode, datap- > > >entry, datap->r3); > > + > > + nr_threads += ret.nr_threads; > > + datap->nr_started += ret.nr_started; > > +} > > + > > +/* > > + * Start all stopped cpus on the guest at entry with register 3 > > set to r3 > > + * We expect that we come in with only one thread currently > > started > > + * Returns: TRUE on success > > + * FALSE on failure > > + */ > > +bool start_all_cpus(secondary_entry_fn entry, uint32_t r3) > > +{ > > + struct secondary_entry_data data = { entry, r3, 0 > > }; > > + int ret; > > + > > + ret = dt_for_each_cpu_node(start_each_secondary, &data); > > + assert(ret = 0); > > + > > + /* We expect that we come in with one thread already > > started */ > > + return data.nr_started = nr_threads - 1; > > +} > > diff --git a/lib/ppc64/asm/smp.h b/lib/ppc64/asm/smp.h > > new file mode 100644 > > index 0000000..67ced75 > > --- /dev/null > > +++ b/lib/ppc64/asm/smp.h > > @@ -0,0 +1 @@ > > +#include "../../powerpc/asm/smp.h" > > diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common > > index 404194b..677030a 100644 > > --- a/powerpc/Makefile.common > > +++ b/powerpc/Makefile.common > > @@ -37,6 +37,7 @@ cflatobjs += lib/powerpc/setup.o > >  cflatobjs += lib/powerpc/rtas.o > >  cflatobjs += lib/powerpc/processor.o > >  cflatobjs += lib/powerpc/handlers.o > > +cflatobjs += lib/powerpc/smp.o > >   > >  FLATLIBS = $(libcflat) $(LIBFDT_archive) > >  %.elf: CFLAGS += $(arch_CFLAGS) > Just some cosmetic nits ... it would be nice if you'd address them in > case you respin, but I'm also fine with the patch in the current > shape > already, so: > > Reviewed-by: Thomas Huth > Thanks