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

[Thomas Huth <thuth@redhat.com>]

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.

>> +
>> +	/*
>> +	 * 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.

 Thomas