| Message ID | 1475229293-11605-1-git-send-email-thuth@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
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++) { > -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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> -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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++) { -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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++) {
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(-)