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@xxxxxxxxxx>
>> ---
>> 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" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
