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