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@xxxxxxxxxx>
>>> ---
>>> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
