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@xxxxxxxxxx>
> > > ---
> > > 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
>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
