From mboxrd@z Thu Jan 1 00:00:00 1970 From: Suraj Jitindar Singh Subject: Re: [kvm-unit-tests PATCH V2 3/4] lib/powerpc: Add function to start secondary threads Date: Tue, 16 Aug 2016 15:10:01 +1000 Message-ID: <1471324201.2108.4.camel@gmail.com> References: <1470794377-14427-1-git-send-email-sjitindarsingh@gmail.com> <1470794377-14427-3-git-send-email-sjitindarsingh@gmail.com> <20160812170750.lqd7cbjovejyhyvl@kamzik.localdomain> <1471226326.4052.24.camel@gmail.com> <20160815062701.xe2zh77lydta5qzx@hawk.localdomain> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit Cc: kvm@vger.kernel.org, pbonzini@redhat.com, rkrcmar@redhat.com, kvm-ppc@vger.kernel.org, lvivier@redhat.com, thuth@redhat.com To: Andrew Jones Return-path: Received: from mail-pa0-f68.google.com ([209.85.220.68]:36852 "EHLO mail-pa0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750752AbcHPFKJ (ORCPT ); Tue, 16 Aug 2016 01:10:09 -0400 In-Reply-To: <20160815062701.xe2zh77lydta5qzx@hawk.localdomain> Sender: kvm-owner@vger.kernel.org List-ID: On Mon, 2016-08-15 at 08:27 +0200, Andrew Jones wrote: > On Mon, Aug 15, 2016 at 11:58:46AM +1000, Suraj Jitindar Singh wrote: > > > > On Fri, 2016-08-12 at 19:07 +0200, Andrew Jones wrote: > > > > > > On Wed, Aug 10, 2016 at 11:59:36AM +1000, 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 > > > > --- > > > >  lib/powerpc/asm/smp.h   |  15 +++++++ > > > >  lib/powerpc/smp.c       | 115 > > > > ++++++++++++++++++++++++++++++++++++++++++++++++ > > > >  lib/ppc64/asm/smp.h     |   1 + > > > >  powerpc/Makefile.common |   1 + > > > >  4 files changed, 132 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..a4f3e7f > > > > --- /dev/null > > > > +++ b/lib/powerpc/asm/smp.h > > > > @@ -0,0 +1,15 @@ > > > > +#ifndef _ASMPOWERPC_SMP_H_ > > > > +#define _ASMPOWERPC_SMP_H_ > > > > + > > > > +#include > > > > +#include > > > nit: no need to include stdint.h, libcflat.h already does, so > > > most > > > files neglect it. > > Will remove > > > > > > > > > > > > > > > > > > + > > > > +typedef void (*secondary_entry_fn)(void); > > > > + > > > > +extern void halt(void); > > > > + > > > > +bool start_thread(int cpu_id, secondary_entry_fn entry, > > > > uint32_t > > > > r3); > > > > +bool start_cpu(int cpu_node, secondary_entry_fn entry, > > > > uint32_t > > > > r3); > > > > +bool start_all_cpus(secondary_entry_fn entry, uint32_t r3); > > > nit: kvm-unit-tests likes to use 'extern' on function > > > declarations > > Ok, I'll add this > > > > > > > > > > > > > > > > > > + > > > > +#endif /* _ASMPOWERPC_SMP_H_ */ > > > > diff --git a/lib/powerpc/smp.c b/lib/powerpc/smp.c > > > > new file mode 100644 > > > > index 0000000..8968907 > > > > --- /dev/null > > > > +++ b/lib/powerpc/smp.c > > > > @@ -0,0 +1,115 @@ > > > > +/* > > > > + * Secondary cpu support > > > > + * > > > > + * Copyright 2016 Suraj Jitindar Singh, IBM. > > > > + * > > > > + * This work is licensed under the terms of the GNU LGPL, > > > > version > > > > 2. > > > > + */ > > > > + > > > > +#include > > > same nit as above > > > > > > > > > > > > > > > +#include > > > > +#include > > > nit: only need libfdt/libfdt.h, as that includes fdt.h > > > > > > > > > > > > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > + > > > > +struct secondary_entry_data { > > > > + secondary_entry_fn entry; > > > > + uint64_t r3; > > > > + bool init_failed; > > > > +}; > > > > + > > > > +/* > > > > + * Start stopped thread cpu_id at entry > > > > + * Returns: 1 on success or cpu not in stopped state > > > > + * 0 on failure to start stopped cpu > > > Returns: TRUE  ... > > >          FALSE ... > > Same thing, but ok > > > > > > > > > > > > > > > > > > + * > > > > + * Note: This function returns 1 on success in starting a > > > > stopped > > > > cpu or if the > > > returns true > > > > > > > > > > > > > > > + *  given cpu was not in the stopped state. Thus this > > > > can > > > > be called on a > > > > + *  list of cpus and all the stopped ones will be > > > > started > > > > while false > > > > + *  won't be returned if some cpus in that list were > > > > already running. Thus > > > > + *  the user should check that cpus passed to this > > > > function > > > > are already in > > > > + *  the stopped state if they want to guarantee that a > > > > return value of > > > > + *  true corresponds to the given cpu now executing at > > > > entry. This > > > > + *  function checks again however as calling cpu-start > > > > on a > > > > not stopped > > > > + *  cpu results in undefined behaviour. > > > > + */ > > > > +bool 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); > > > > + > > > > + ret = rtas_call(query_token, 1, 2, outputs, cpu_id); > > > > + if (ret) { > > > > + printf("query-cpu-stopped-state failed for cpu > > > > %d\n", cpu_id); > > > > + return false; > > > > + } > > > > + > > > > + if (!outputs[0]) { /* cpu in stopped state */ > > > > + ret = rtas_call(start_token, 3, 1, NULL, > > > > cpu_id, > > > > entry, r3); > > > > + if (ret) { > > > > + printf("failed to start cpu %d\n", > > > > cpu_id); > > > > + return false; > > > > + } > > > > + } > > > > + > > > > + return true; > > > > +} > > > > + > > > > +/* > > > > + * Start all stopped threads (vcpus) on cpu_node > > > > + * Returns: 1 on success > > > > + * 0 on failure > > > TRUE/FALSE, but I'd actually change the return type to struct > > > start_threads; > > > > > >  struct start_threads { > > >     int nr_threads; > > >     int nr_started; > > >  }; > > > > > > Then... > > > > > > > > > > > > > > > + */ > > > > +bool start_cpu(int cpu_node, secondary_entry_fn entry, > > > > uint32_t > > > > r3) > > > > +{ > > > > + const struct fdt_property *prop; > > > > + int len, nr_cpu, cpu; > > > > + u32 *cpus; > > > > + bool ret = true; > > > > + > > > > + /* 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_cpu = len >> 2; /* Divide by 4 since 4 bytes per > > > > cpu */ > > > > + cpus = (u32 *)prop->data; /* Array of valid cpu > > > > numbers */ > > > > + > > > > + for (cpu = 0; cpu < nr_cpu && ret; cpu++) > > > > + ret = start_thread(fdt32_to_cpu(cpus[cpu]), > > > > entry, > > > > r3); > > > > + return ret; > > > ...change this to > > > > > >  nr_threads = len >> 2; /* Divide by 4 since 4 bytes per cpu */ > > >  cpus = (u32 *)prop->data; /* Array of valid cpu numbers */ > > > > > >  for (cpu = 0; cpu < nr_threads; cpu++) > > >      nr_started += start_thread(fdt32_to_cpu(cpus[cpu]), entry, > > > r3); > > I guess this fits the more generic use case. Although I can't > > really > > see a scenario where the caller wants to start all cpus and > > continue > > starting them when one fails, that is if one fails to start you > > probably might as well return an error code immediately. > > > > > > > > >  return (struct start_threads){ nr_threads, nr_started }; > > > > > > and... > > >      > > > > > > > > > > > > +} > > > > + > > > > +static void start_each_secondary(int fdtnode, u32 regval > > > > __unused, > > > > void *info) > > > > +{ > > > > + struct secondary_entry_data *datap = info; > > > > + > > > > + datap->init_failed |= !start_cpu(fdtnode, datap- > > > > >entry, > > > > datap->r3); > > > > +} > > > ...change init_failed to nr_started > > > > > >  start_threads = start_cpu(fdtnode, datap->entry, datap->r3); > > >  nr_cpus += start_threads.nr_threads; // nr_cpus is global like > > > ARM > > > has > > >  datap->nr_started += start_threads.nr_started; > > > > > > and below just check that datap->nr_started == nr_cpus. > > nr_cpus is set during setup so it would be possible to just have > > the > > above return nr_started and then check this accumulated value > > against > > nr_cpu below. > > > > > > > > > > > > > > > > > > + > > > > +/* > > > > + * Start all stopped cpus on the guest at entry with register > > > > 3 > > > > set to r3 > > > > + * Returns: 1 on success > > > > + * 0 on failure > > > TRUE/FALSE > > Ok > > > > > > > > > > > > > > > > > > + */ > > > > +bool start_all_cpus(secondary_entry_fn entry, uint32_t r3) > > > > +{ > > > > + struct secondary_entry_data data = { > > > > + entry, > > > > + r3, > > > > + false > > > > + }; > > > > + int ret; > > > > + > > > > + ret = dt_for_each_cpu_node(&start_each_secondary, > > > > (void *) > > > > &data); > > > assert(ret == 0) > > Sounds good > > > > > > > > > > > > > > > > > > + > > > > + return !(ret || data.init_failed); > > > See comment above about setting nr_cpus, and then just confirming > > > they all > > > started here instead. > > I think I'll change the above so that start_cpu returns the number > > of > > cpus started (we already know the total number of cpus so the > > struct is > > unnecessary), we come in with one cpu already started so I'll check > > that nr_started == nr_cpu - 1. > I completely forgot that I wrote code setting up nr_cpus... After > reading this patch, I actually assumed I hadn't, because I didn't > recall addressing threads. So is the nr_cpus in setup correct? nr_cpus in setup is done as just that, the number of cpus, not threads. Which I didn't realise until I read the code closer. > Without accounting for threads, then it can't be, can it? Maybe > this patch series should start by fixing that, and also bumping > NR_CPUS in lib/powerpc/asm/setup.h up to whatever makes more sense. Depending on what we want nr_cpus to be. I suggest we keep it as the number of cpus and I implement this as you initially suggested using struct start_threads to keep track of the total number of threads and the number successfully started, checking that the number started is one less than the total number. This is how I plan on implementing it currently for the next revision. > > > > > > > > > > > > > > > > > > > > > +} > > > > 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) > > Thanks for the review > Thanks, > drew