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

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

[Thomas Huth]

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 <thuth@redhat.com>
---
 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 <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);
+	assert(prop != NULL);
+
+	if (plen >= 26 && prop->data[1] == 0 && (prop->data[24] & 0x80) != 0)
+		*(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++) {
-- 
1.8.3.1

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

[Laurent Vivier]

On 28/09/2016 12:18, 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 <thuth@redhat.com>
> ---
>  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 <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);
> +	assert(prop != NULL);
> +
> +	if (plen >= 26 && prop->data[1] == 0 && (prop->data[24] & 0x80) != 0)
> +		*(int *)ptr += 1;

Perhaps some comments can help here:
why do you check plen >= 26 and not >= 25?
why do you check prop->data[1]?
why don't you check prop->data[23] for the size of the attribute?

> +}
> +
> +/* 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();
> +

perhaps you can use a more explicit variable name for "i"?

>  	all = argc == 1 || !strcmp(argv[1], "all");
>  
>  	for (i = 0; hctests[i].name != NULL; i++) {
> 

Thanks,
Laurent

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

[Thomas Huth]

On 28.09.2016 14:13, Laurent Vivier wrote:
> 
> 
> On 28/09/2016 12:18, 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 <thuth@redhat.com>
>> ---
>>  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 <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);
>> +	assert(prop != NULL);
>> +
>> +	if (plen >= 26 && prop->data[1] == 0 && (prop->data[24] & 0x80) != 0)
>> +		*(int *)ptr += 1;
> 
> Perhaps some comments can help here:
> why do you check plen >= 26 and not >= 25?
> why do you check prop->data[1]?

Well, it's all in the (Lo-)PAPR spec, but I can add a comment there if
you like.

> why don't you check prop->data[23] for the size of the attribute?

I guess you mean  prop->data[0] ? ... sure, I can add that check, too.

>> +}
>> +
>> +/* 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();
>> +
> 
> perhaps you can use a more explicit variable name for "i"?

Sure, I can do that. I'll wait for some more review feedback, then I'll
send a v2.

 Thomas

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

[Laurent Vivier]

On 28/09/2016 14:23, Thomas Huth wrote:
> On 28.09.2016 14:13, Laurent Vivier wrote:
>>
>>
>> On 28/09/2016 12:18, 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 <thuth@redhat.com>
>>> ---
>>>  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 <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);
>>> +	assert(prop != NULL);
>>> +
>>> +	if (plen >= 26 && prop->data[1] == 0 && (prop->data[24] & 0x80) != 0)
>>> +		*(int *)ptr += 1;
>>
>> Perhaps some comments can help here:
>> why do you check plen >= 26 and not >= 25?
>> why do you check prop->data[1]?
> 
> Well, it's all in the (Lo-)PAPR spec, but I can add a comment there if
> you like.

Well, even with the spec, it's not really clear.

>> why don't you check prop->data[23] for the size of the attribute?
> 
> I guess you mean  prop->data[0] ? ... sure, I can add that check, too.

No in fact, I didn't understand correctly the spec. :)

So date[0] is the size, should be plen (so no need to add the check)
date[1] is the type, should be "0" ("attribute-specifier-type", you
check it)
and then an array( "attribute-specifier"), where byte 22 and 23 are
"Level of Transactional Memory Category Support". It's not clear if TM
bit is in byte 22 (data[24]) or in byte 23 (data[25]). How do you know?

> 
>>> +}
>>> +
>>> +/* 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();
>>> +
>>
>> perhaps you can use a more explicit variable name for "i"?
> 
> Sure, I can do that. I'll wait for some more review feedback, then I'll
> send a v2.
> 
>  Thomas
> 

Thanks,
Laurent

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

[Thomas Huth]

On 28.09.2016 14:41, Laurent Vivier wrote:
> 
> 
> On 28/09/2016 14:23, Thomas Huth wrote:
>> On 28.09.2016 14:13, Laurent Vivier wrote:
>>>
>>>
>>> On 28/09/2016 12:18, 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 <thuth@redhat.com>
>>>> ---
>>>>  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 <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);
>>>> +	assert(prop != NULL);
>>>> +
>>>> +	if (plen >= 26 && prop->data[1] == 0 && (prop->data[24] & 0x80) != 0)
>>>> +		*(int *)ptr += 1;
>>>
>>> Perhaps some comments can help here:
>>> why do you check plen >= 26 and not >= 25?
>>> why do you check prop->data[1]?
>>
>> Well, it's all in the (Lo-)PAPR spec, but I can add a comment there if
>> you like.
> 
> Well, even with the spec, it's not really clear.
> 
>>> why don't you check prop->data[23] for the size of the attribute?
>>
>> I guess you mean  prop->data[0] ? ... sure, I can add that check, too.
> 
> No in fact, I didn't understand correctly the spec. :)
> 
> So date[0] is the size, should be plen (so no need to add the check)
> date[1] is the type, should be "0" ("attribute-specifier-type", you
> check it)
> and then an array( "attribute-specifier"), where byte 22 and 23 are
> "Level of Transactional Memory Category Support". It's not clear if TM
> bit is in byte 22 (data[24]) or in byte 23 (data[25]). How do you know?

That's the way how it is encoded in QEMU currently, and the byte the
Linux kernel looks at. So I think it's right.

 Thomas

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

[Laurent Vivier]

On 28/09/2016 15:10, Thomas Huth wrote:
> On 28.09.2016 14:41, Laurent Vivier wrote:
>>
>>
>> On 28/09/2016 14:23, Thomas Huth wrote:
>>> On 28.09.2016 14:13, Laurent Vivier wrote:
>>>>
>>>>
>>>> On 28/09/2016 12:18, 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 <thuth@redhat.com>
>>>>> ---
>>>>>  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 <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);
>>>>> +	assert(prop != NULL);
>>>>> +
>>>>> +	if (plen >= 26 && prop->data[1] == 0 && (prop->data[24] & 0x80) != 0)
>>>>> +		*(int *)ptr += 1;
>>>>
>>>> Perhaps some comments can help here:
>>>> why do you check plen >= 26 and not >= 25?
>>>> why do you check prop->data[1]?
>>>
>>> Well, it's all in the (Lo-)PAPR spec, but I can add a comment there if
>>> you like.
>>
>> Well, even with the spec, it's not really clear.
>>
>>>> why don't you check prop->data[23] for the size of the attribute?
>>>
>>> I guess you mean  prop->data[0] ? ... sure, I can add that check, too.
>>
>> No in fact, I didn't understand correctly the spec. :)
>>
>> So date[0] is the size, should be plen (so no need to add the check)
>> date[1] is the type, should be "0" ("attribute-specifier-type", you
>> check it)
>> and then an array( "attribute-specifier"), where byte 22 and 23 are
>> "Level of Transactional Memory Category Support". It's not clear if TM
>> bit is in byte 22 (data[24]) or in byte 23 (data[25]). How do you know?
> 
> That's the way how it is encoded in QEMU currently, and the byte the
> Linux kernel looks at. So I think it's right.

thanks, I think it could be good to have a comment.

Reviewed-by: Laurent Vivier <lvivier@redhat.com>

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

[Andrew Jones]

On Wed, Sep 28, 2016 at 12:18:39PM +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 <thuth@redhat.com>
> ---
>  powerpc/tm.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)

Reviewed-by: Andrew Jones <drjones@redhat.com>

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

[Suraj Jitindar Singh]

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 <thuth@redhat.com>
> ---
>  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 <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);
> +	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 <sjitindarsingh@gmail.com>