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

[kvm-unit-tests PATCH v3] 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 is not available.

Reviewed-by: Laurent Vivier <lvivier@redhat.com>
Reviewed-by: Andrew Jones <drjones@redhat.com> 
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 v3:
 - Removed unnecessary check for "plen >= 26" in cpu_has_tm()

 v2:
 - Reworked the check for the "ibm,pa-features" and added a comment
 - Use a dedicated variable "has_tm" instead of "i" in main()

 powerpc/tm.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/powerpc/tm.c b/powerpc/tm.c
index 6ce750a..b611061 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);
+
+	/*
+	 * 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 (prop->data[0] >= 24 && (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)
 {
@@ -101,11 +136,17 @@ struct {
 
 int main(int argc, char **argv)
 {
-	bool all;
+	bool all, has_tm;
 	int i;
 
 	report_prefix_push("tm");
 
+	has_tm = all_cpus_have_tm();
+	report_xfail("TM available in 'ibm,pa-features' property",
+		     !has_tm, has_tm);
+	if (!has_tm)
+		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 v3] powerpc: Check whether TM is available before running other tests

[Laurent Vivier]

On 04/10/2016 11:07, 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.
> 
> Reviewed-by: Laurent Vivier <lvivier@redhat.com>
> Reviewed-by: Andrew Jones <drjones@redhat.com> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  v3:
>  - Removed unnecessary check for "plen >= 26" in cpu_has_tm()
> 
>  v2:
>  - Reworked the check for the "ibm,pa-features" and added a comment
>  - Use a dedicated variable "has_tm" instead of "i" in main()
> 
>  powerpc/tm.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 42 insertions(+), 1 deletion(-)
> 
> diff --git a/powerpc/tm.c b/powerpc/tm.c
> index 6ce750a..b611061 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);
> +
> +	/*
> +	 * 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 (prop->data[0] >= 24 && (prop->data[24] & 0x80) != 0)
> +		*(int *)ptr += 1;
> +}

in the future, if we think we can have more than the type 0, we should
scan the property with something like:

  off = 0
  while (off + 1 < plen) {
    if (prop->data[off + 1] != 0) {
      off += 2 + prop->data[off];
      continue;
    }
    ... check TM ...
  }

but I don't think we need that now...

Laurent

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

[Suraj Jitindar Singh]

On Tue, 2016-10-04 at 11:07 +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.
> 
> Reviewed-by: Laurent Vivier <lvivier@redhat.com>
> Reviewed-by: Andrew Jones <drjones@redhat.com> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
Thanks for sending a v3.

Reviewed-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> ---
>  v3:
>  - Removed unnecessary check for "plen >= 26" in cpu_has_tm()
> 
>  v2:
>  - Reworked the check for the "ibm,pa-features" and added a comment
>  - Use a dedicated variable "has_tm" instead of "i" in main()
> 
>  powerpc/tm.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 42 insertions(+), 1 deletion(-)
> 
> diff --git a/powerpc/tm.c b/powerpc/tm.c
> index 6ce750a..b611061 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);
> +
> +	/*
> +	 * 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 (prop->data[0] >= 24 && (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)
>  {
> @@ -101,11 +136,17 @@ struct {
>  
>  int main(int argc, char **argv)
>  {
> -	bool all;
> +	bool all, has_tm;
>  	int i;
>  
>  	report_prefix_push("tm");
>  
> +	has_tm = all_cpus_have_tm();
> +	report_xfail("TM available in 'ibm,pa-features' property",
> +		     !has_tm, has_tm);
> +	if (!has_tm)
> +		return report_summary();
> +
>  	all = argc == 1 || !strcmp(argv[1], "all");
>  
>  	for (i = 0; hctests[i].name != NULL; i++) {

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

[Thomas Huth]

On 05.10.2016 09:25, Suraj Jitindar Singh wrote:
> On Tue, 2016-10-04 at 11:07 +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.
>>
>> Reviewed-by: Laurent Vivier <lvivier@redhat.com>
>> Reviewed-by: Andrew Jones <drjones@redhat.com> 
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
> Thanks for sending a v3.
> 
> Reviewed-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
>> ---
>>  v3:
>>  - Removed unnecessary check for "plen >= 26" in cpu_has_tm()
>>
>>  v2:
>>  - Reworked the check for the "ibm,pa-features" and added a comment
>>  - Use a dedicated variable "has_tm" instead of "i" in main()
>>
>>  powerpc/tm.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 42 insertions(+), 1 deletion(-)

 Radim, Paolo,

I think this patch is now ready to go ... could you please pick it up?
(or shall I send a pull request for it?)

 Thanks,
  Thomas

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

[Paolo Bonzini]

On 12/10/2016 10:04, Thomas Huth wrote:
> On 05.10.2016 09:25, Suraj Jitindar Singh wrote:
>> On Tue, 2016-10-04 at 11:07 +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.
>>>
>>> Reviewed-by: Laurent Vivier <lvivier@redhat.com>
>>> Reviewed-by: Andrew Jones <drjones@redhat.com> 
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> Thanks for sending a v3.
>>
>> Reviewed-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
>>> ---
>>>  v3:
>>>  - Removed unnecessary check for "plen >= 26" in cpu_has_tm()
>>>
>>>  v2:
>>>  - Reworked the check for the "ibm,pa-features" and added a comment
>>>  - Use a dedicated variable "has_tm" instead of "i" in main()
>>>
>>>  powerpc/tm.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
>>>  1 file changed, 42 insertions(+), 1 deletion(-)
> 
>  Radim, Paolo,
> 
> I think this patch is now ready to go ... could you please pick it up?
> (or shall I send a pull request for it?)

Pull requests are always welcome, at least by me. :)

Paolo

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

[Radim Krčmář]

2016-10-12 10:19+0200, Paolo Bonzini:
> On 12/10/2016 10:04, Thomas Huth wrote:
>> On 05.10.2016 09:25, Suraj Jitindar Singh wrote:
>>> On Tue, 2016-10-04 at 11:07 +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.
>>>>
>>>> Reviewed-by: Laurent Vivier <lvivier@redhat.com>
>>>> Reviewed-by: Andrew Jones <drjones@redhat.com> 
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> Thanks for sending a v3.
>>>
>>> Reviewed-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
>>>> ---
>>>>  v3:
>>>>  - Removed unnecessary check for "plen >= 26" in cpu_has_tm()
>>>>
>>>>  v2:
>>>>  - Reworked the check for the "ibm,pa-features" and added a comment
>>>>  - Use a dedicated variable "has_tm" instead of "i" in main()
>>>>
>>>>  powerpc/tm.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
>>>>  1 file changed, 42 insertions(+), 1 deletion(-)
>> 
>>  Radim, Paolo,
>> 
>> I think this patch is now ready to go ... could you please pick it up?
>> (or shall I send a pull request for it?)
> 
> Pull requests are always welcome, at least by me. :)

Seconded.  I think cooperation will be smoother when using pull
requests ...  I didn't notice that the ball was on me this time.
Throwing it back for a pull request.