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

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

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?
> +
> +	/*
> +	 * 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". If
you were to change the above to "prop->data[0] == plen - 2" then either
one of the two checks could be kept as sufficient to ensure the other.
> +		*(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++) {