From mboxrd@z Thu Jan 1 00:00:00 1970 From: Suraj Jitindar Singh Subject: Re: [kvm-unit-tests PATCH v2] powerpc: Check whether TM is available before running other tests Date: Wed, 05 Oct 2016 18:22:14 +1100 Message-ID: <1475652134.2070.0.camel@gmail.com> References: <1475229293-11605-1-git-send-email-thuth@redhat.com> <1475542106.2034.7.camel@gmail.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-pa0-f65.google.com ([209.85.220.65]:33731 "EHLO mail-pa0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751664AbcJEHWX (ORCPT ); Wed, 5 Oct 2016 03:22:23 -0400 In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: On Tue, 2016-10-04 at 10:23 +0200, Thomas Huth wrote: > On 04.10.2016 02:48, Suraj Jitindar Singh wrote: > > > > On Fri, 2016-09-30 at 11:54 +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 is not available. > > > > > > Signed-off-by: Thomas Huth > > > --- > > >  v2: > > >  - Reworked the check for the "ibm,pa-features" and added a > > > comment > > >  - Use a dedicated variable "has_tm" instead of "i" in main() > > > > > >  Laurent, Suraj, Andrew, I did not add your Reviewed-by (thanks > > > for > > >  that!) from v1 here since I changed the code a little bit. So it > > >  would be great if you could have another quick look at this v2. > > Comments below > > > > > > > > >  powerpc/tm.c | 43 ++++++++++++++++++++++++++++++++++++++++++- > > >  1 file changed, 42 insertions(+), 1 deletion(-) > > > > > > diff --git a/powerpc/tm.c b/powerpc/tm.c > > > index 6ce750a..8344318 100644 > > > --- a/powerpc/tm.c > > > +++ b/powerpc/tm.c > > > @@ -10,6 +10,41 @@ > > >  #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); > > > + if (!prop) /* No features means TM is also not > > > available */ > > > + return; > > > + /* Sanity check for the property layout (first two bytes > > > are > > > header) */ > > > + assert(plen >= 8 && prop->data[1] == 0 && prop->data[0] > > > <= > > > plen - 2); > > Just curious as to why you're checking "prop->data[0] *<=* plen - > > 2" as > > isn't anything other than prop->data[0] *==* plen - 2 an error in > > the > > structure of ibm,pa-features and thus an error in the device-tree? > QEMU currently uses prop->data[0] == plen - 2 , but looking at the > LoPAPR specification, it clearly defines this property as > "prop-encoded-array: One or more attribute-descriptor(s)", so there > could be two or more attributes encoded in this property. While there > is > currently only attribute type 0 defined in the LoPAPR specification, > it > could be extended with other types in the future. So with the  "<=", > the > code is already prepared for this situation in the future. Sorry I do see that now, my misunderstanding. > > > > > > > > > + > > > + /* > > > +  * The "Transactional Memory Category Support" flags are > > > at > > > byte > > > +  * offset 22 and 23 of the attribute type 0, so when > > > adding > > > the > > > +  * two bytes for the header, we've got to look at offset > > > 24 > > > for > > > +  * the TM support bit. > > > +  */ > > > + if (plen >= 26 && prop->data[0] >= 24 && (prop->data[24] > > > & > > > 0x80) != 0) > > With the sanity checking you performed before isn't it sufficient > > to > > check "prop->data[0] >= 24" as this guarantees that "plen >= 26". > You're right, since the assert() already checked that > "data[0] <= plen - 2", and I also check that "data[0] >= 24", we > can automatically assume that "24 <= plen - 2", i.e. "plen >= 26". > I'll send a v3 with that check removed. Thanks > >  Thomas >