From mboxrd@z Thu Jan 1 00:00:00 1970 From: Suraj Jitindar Singh Date: Thu, 29 Sep 2016 00:20:51 +0000 Subject: Re: [kvm-unit-tests PATCH] powerpc: Check whether TM is available before running other tests Message-Id: <1475108451.2405.6.camel@gmail.com> List-Id: References: <1475057919-29017-1-git-send-email-thuth@redhat.com> In-Reply-To: <1475057919-29017-1-git-send-email-thuth@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: Radim =?UTF-8?Q?Kr=C4=8Dm=C3=A1=C5=99?= , kvm-ppc@vger.kernel.org, Laurent Vivier , Drew Jones On Wed, 2016-09-28 at 12:18 +0200, Thomas Huth wrote: > Transactional memory is currently only supported on KVM-HV, and > not yet on KVM-PR. So it's better to check the device tree first > and fail gracefully if it's not available. > > Signed-off-by: Thomas Huth > --- >  powerpc/tm.c | 31 +++++++++++++++++++++++++++++++ >  1 file changed, 31 insertions(+) > > diff --git a/powerpc/tm.c b/powerpc/tm.c > index 6ce750a..83d9d3d 100644 > --- a/powerpc/tm.c > +++ b/powerpc/tm.c > @@ -10,6 +10,32 @@ >  #include >  #include >  #include > +#include > +#include > + > +/* Check "ibm,pa-features" property of a CPU node for the TM flag */ > +static void cpu_has_tm(int fdtnode, u32 regval __unused, void *ptr) > +{ > + const struct fdt_property *prop; > + int plen; > + > + prop = fdt_get_property(dt_fdt(), fdtnode, "ibm,pa- > features", &plen); > + assert(prop != NULL); > + > + if (plen >= 26 && prop->data[1] = 0 && (prop->data[24] & > 0x80) != 0) If you're checking prop->data[1] = 0 to verify device tree consistency then maybe it's worth checking that plen = prop->data[0] as well. Although in the current state these are hard coded into qemu and unlikely to ever be wrong it's probably nice to check. It might also be worth printing some error message if these consistency checks fail as it's probably an indicator of a larger problem somewhere else than anything actually wrong with the test. > + *(int *)ptr += 1; > +} > + > +/* Check whether all CPU nodes have the TM flag */ > +static bool all_cpus_have_tm(void) > +{ > + int ret; > + int available = 0; > + > + ret = dt_for_each_cpu_node(cpu_has_tm, &available); > + > + return ret = 0 && available = nr_cpus; > +} >   >  static int h_cede(void) >  { > @@ -106,6 +132,11 @@ int main(int argc, char **argv) >   >   report_prefix_push("tm"); >   > + i = all_cpus_have_tm(); > + report_xfail("TM available in 'ibm,pa-features' property", > !i, i); > + if (!i) > + return report_summary(); > + >   all = argc = 1 || !strcmp(argv[1], "all"); >   >   for (i = 0; hctests[i].name != NULL; i++) { Thanks for adding this check, didn't cross my mind when implementing the test. In its current state or with changes: Reviewed-by: Suraj Jitindar Singh From mboxrd@z Thu Jan 1 00:00:00 1970 From: Suraj Jitindar Singh Subject: Re: [kvm-unit-tests PATCH] powerpc: Check whether TM is available before running other tests Date: Thu, 29 Sep 2016 10:20:51 +1000 Message-ID: <1475108451.2405.6.camel@gmail.com> References: <1475057919-29017-1-git-send-email-thuth@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit Cc: Radim =?UTF-8?Q?Kr=C4=8Dm=C3=A1=C5=99?= , kvm-ppc@vger.kernel.org, Laurent Vivier , Drew Jones To: Thomas Huth , kvm@vger.kernel.org Return-path: Received: from mail-pf0-f194.google.com ([209.85.192.194]:34206 "EHLO mail-pf0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750831AbcI2AU6 (ORCPT ); Wed, 28 Sep 2016 20:20:58 -0400 In-Reply-To: <1475057919-29017-1-git-send-email-thuth@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Wed, 2016-09-28 at 12:18 +0200, Thomas Huth wrote: > Transactional memory is currently only supported on KVM-HV, and > not yet on KVM-PR. So it's better to check the device tree first > and fail gracefully if it's not available. > > Signed-off-by: Thomas Huth > --- >  powerpc/tm.c | 31 +++++++++++++++++++++++++++++++ >  1 file changed, 31 insertions(+) > > diff --git a/powerpc/tm.c b/powerpc/tm.c > index 6ce750a..83d9d3d 100644 > --- a/powerpc/tm.c > +++ b/powerpc/tm.c > @@ -10,6 +10,32 @@ >  #include >  #include >  #include > +#include > +#include > + > +/* Check "ibm,pa-features" property of a CPU node for the TM flag */ > +static void cpu_has_tm(int fdtnode, u32 regval __unused, void *ptr) > +{ > + const struct fdt_property *prop; > + int plen; > + > + prop = fdt_get_property(dt_fdt(), fdtnode, "ibm,pa- > features", &plen); > + assert(prop != NULL); > + > + if (plen >= 26 && prop->data[1] == 0 && (prop->data[24] & > 0x80) != 0) If you're checking prop->data[1] == 0 to verify device tree consistency then maybe it's worth checking that plen == prop->data[0] as well. Although in the current state these are hard coded into qemu and unlikely to ever be wrong it's probably nice to check. It might also be worth printing some error message if these consistency checks fail as it's probably an indicator of a larger problem somewhere else than anything actually wrong with the test. > + *(int *)ptr += 1; > +} > + > +/* Check whether all CPU nodes have the TM flag */ > +static bool all_cpus_have_tm(void) > +{ > + int ret; > + int available = 0; > + > + ret = dt_for_each_cpu_node(cpu_has_tm, &available); > + > + return ret == 0 && available == nr_cpus; > +} >   >  static int h_cede(void) >  { > @@ -106,6 +132,11 @@ int main(int argc, char **argv) >   >   report_prefix_push("tm"); >   > + i = all_cpus_have_tm(); > + report_xfail("TM available in 'ibm,pa-features' property", > !i, i); > + if (!i) > + return report_summary(); > + >   all = argc == 1 || !strcmp(argv[1], "all"); >   >   for (i = 0; hctests[i].name != NULL; i++) { Thanks for adding this check, didn't cross my mind when implementing the test. In its current state or with changes: Reviewed-by: Suraj Jitindar Singh