Re: [kvm-unit-tests PATCH v2] powerpc: Check whether TM is available before running other tests

[Suraj Jitindar Singh <sjitindarsingh@gmail.com>]

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 <thuth@redhat.com>
> > > ---
> > >  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 <asm/processor.h>
> > >  #include <asm/handlers.h>
> > >  #include <asm/smp.h>
> > > +#include <asm/setup.h>
> > > +#include <devicetree.h>
> > > +
> > > +/* 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
>