* [kvm-unit-tests PATCH v2] powerpc: Check whether TM is available before running other tests
@ 2016-09-30 9:54 ` Thomas Huth
0 siblings, 0 replies; 12+ messages in thread
From: Thomas Huth @ 2016-09-30 9:54 UTC (permalink / raw)
To: kvm
Cc: Radim Krčmář,
kvm-ppc, Laurent Vivier, Drew Jones, Suraj Jitindar Singh
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.
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);
+
+ /*
+ * 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)
+ *(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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [kvm-unit-tests PATCH v2] powerpc: Check whether TM is available before running other tests
@ 2016-09-30 9:54 ` Thomas Huth
0 siblings, 0 replies; 12+ messages in thread
From: Thomas Huth @ 2016-09-30 9:54 UTC (permalink / raw)
To: kvm
Cc: Radim Krčmář,
kvm-ppc, Laurent Vivier, Drew Jones, Suraj Jitindar Singh
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.
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);
+
+ /*
+ * 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)
+ *(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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [kvm-unit-tests PATCH v2] powerpc: Check whether TM is available before running other tests
2016-09-30 9:54 ` Thomas Huth
@ 2016-09-30 10:01 ` Laurent Vivier
-1 siblings, 0 replies; 12+ messages in thread
From: Laurent Vivier @ 2016-09-30 10:01 UTC (permalink / raw)
To: Thomas Huth, kvm
Cc: Radim Krčmář, kvm-ppc, Drew Jones, Suraj Jitindar Singh
On 30/09/2016 11:54, 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.
Reviewed-by: Laurent Vivier <lvivier@redhat.com>
>
> 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);
> +
> + /*
> + * 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)
> + *(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++) {
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [kvm-unit-tests PATCH v2] powerpc: Check whether TM is available before running other tests
@ 2016-09-30 10:01 ` Laurent Vivier
0 siblings, 0 replies; 12+ messages in thread
From: Laurent Vivier @ 2016-09-30 10:01 UTC (permalink / raw)
To: Thomas Huth, kvm
Cc: Radim Krčmář, kvm-ppc, Drew Jones, Suraj Jitindar Singh
On 30/09/2016 11:54, 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.
Reviewed-by: Laurent Vivier <lvivier@redhat.com>
>
> 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);
> +
> + /*
> + * 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)
> + *(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++) {
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [kvm-unit-tests PATCH v2] powerpc: Check whether TM is available before running other tests
2016-09-30 9:54 ` Thomas Huth
@ 2016-09-30 11:32 ` Andrew Jones
-1 siblings, 0 replies; 12+ messages in thread
From: Andrew Jones @ 2016-09-30 11:32 UTC (permalink / raw)
To: Thomas Huth
Cc: kvm, Radim Krčmář,
kvm-ppc, Laurent Vivier, Suraj Jitindar Singh
On Fri, Sep 30, 2016 at 11:54:53AM +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.
>
> powerpc/tm.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 42 insertions(+), 1 deletion(-)
>
Reviewed-by: Andrew Jones <drjones@redhat.com>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [kvm-unit-tests PATCH v2] powerpc: Check whether TM is available before running other tests
@ 2016-09-30 11:32 ` Andrew Jones
0 siblings, 0 replies; 12+ messages in thread
From: Andrew Jones @ 2016-09-30 11:32 UTC (permalink / raw)
To: Thomas Huth
Cc: kvm, Radim Krčmář,
kvm-ppc, Laurent Vivier, Suraj Jitindar Singh
On Fri, Sep 30, 2016 at 11:54:53AM +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.
>
> powerpc/tm.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 42 insertions(+), 1 deletion(-)
>
Reviewed-by: Andrew Jones <drjones@redhat.com>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [kvm-unit-tests PATCH v2] powerpc: Check whether TM is available before running other tests
2016-09-30 9:54 ` Thomas Huth
@ 2016-10-04 0:48 ` Suraj Jitindar Singh
-1 siblings, 0 replies; 12+ messages in thread
From: Suraj Jitindar Singh @ 2016-10-04 0:48 UTC (permalink / raw)
To: Thomas Huth, kvm
Cc: Radim Krčmář, kvm-ppc, Laurent Vivier, Drew Jones
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++) {
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [kvm-unit-tests PATCH v2] powerpc: Check whether TM is available before running other tests
@ 2016-10-04 0:48 ` Suraj Jitindar Singh
0 siblings, 0 replies; 12+ messages in thread
From: Suraj Jitindar Singh @ 2016-10-04 0:48 UTC (permalink / raw)
To: Thomas Huth, kvm
Cc: Radim Krčmář, kvm-ppc, Laurent Vivier, Drew Jones
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++) {
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [kvm-unit-tests PATCH v2] powerpc: Check whether TM is available before running other tests
2016-10-04 0:48 ` Suraj Jitindar Singh
@ 2016-10-04 8:23 ` Thomas Huth
-1 siblings, 0 replies; 12+ messages in thread
From: Thomas Huth @ 2016-10-04 8:23 UTC (permalink / raw)
To: Suraj Jitindar Singh, kvm
Cc: Radim Krčmář, kvm-ppc, Laurent Vivier, Drew Jones
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [kvm-unit-tests PATCH v2] powerpc: Check whether TM is available before running other tests
@ 2016-10-04 8:23 ` Thomas Huth
0 siblings, 0 replies; 12+ messages in thread
From: Thomas Huth @ 2016-10-04 8:23 UTC (permalink / raw)
To: Suraj Jitindar Singh, kvm
Cc: Radim Krčmář, kvm-ppc, Laurent Vivier, Drew Jones
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [kvm-unit-tests PATCH v2] powerpc: Check whether TM is available before running other tests
2016-10-04 8:23 ` Thomas Huth
@ 2016-10-05 7:22 ` Suraj Jitindar Singh
-1 siblings, 0 replies; 12+ messages in thread
From: Suraj Jitindar Singh @ 2016-10-05 7:22 UTC (permalink / raw)
To: Thomas Huth, kvm
Cc: Radim Krčmář, kvm-ppc, Laurent Vivier, Drew Jones
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@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.
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
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [kvm-unit-tests PATCH v2] powerpc: Check whether TM is available before running other tests
@ 2016-10-05 7:22 ` Suraj Jitindar Singh
0 siblings, 0 replies; 12+ messages in thread
From: Suraj Jitindar Singh @ 2016-10-05 7:22 UTC (permalink / raw)
To: Thomas Huth, kvm
Cc: Radim Krčmář, kvm-ppc, Laurent Vivier, Drew Jones
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@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.
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
>
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2016-10-05 7:22 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-30 9:54 [kvm-unit-tests PATCH v2] powerpc: Check whether TM is available before running other tests Thomas Huth
2016-09-30 9:54 ` Thomas Huth
2016-09-30 10:01 ` Laurent Vivier
2016-09-30 10:01 ` Laurent Vivier
2016-09-30 11:32 ` Andrew Jones
2016-09-30 11:32 ` Andrew Jones
2016-10-04 0:48 ` Suraj Jitindar Singh
2016-10-04 0:48 ` Suraj Jitindar Singh
2016-10-04 8:23 ` Thomas Huth
2016-10-04 8:23 ` Thomas Huth
2016-10-05 7:22 ` Suraj Jitindar Singh
2016-10-05 7:22 ` Suraj Jitindar Singh
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.