Re: [PATCH REPOST 3/3] powerpc/vphn: move endianness fixing to vphn_unpack_associativity()
From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date: 2014-11-26 23:39:37
On Mon, 2014-11-17 at 18:42 +0100, Greg Kurz wrote:
The first argument to vphn_unpack_associativity() is a const long *, but the parsing code expects __be64 values actually. This is inconsistent. We should either pass a const __be64 * or change vphn_unpack_associativity() so that it fixes endianness by itself. This patch does the latter, since the caller doesn't need to know about endianness and this allows to fix significant 64-bit values only. Please note that the previous code was able to cope with 32-bit fields being split accross two consecutives 64-bit values. Since PAPR+ doesn't say this cannot happen, the behaviour was kept. It requires extra checking to know when fixing is needed though.
While I agree with moving the endian fixing down, the patch makes me nervous. Note that I don't fully understand the format of what we are parsing here so I might be wrong but ...
#define VPHN_FIELD_UNUSED (0xffff)
#define VPHN_FIELD_MSB (0x8000)
#define VPHN_FIELD_MASK (~VPHN_FIELD_MSB)
- for (i = 1; i < VPHN_ASSOC_BUFSIZE; i++) {
- if (be16_to_cpup(field) == VPHN_FIELD_UNUSED)
+ for (i = 1, j = 0, k = 0; i < VPHN_ASSOC_BUFSIZE;) {
+ u16 field;
+
+ if (j % 4 == 0) {
+ fixed.packed[k] = cpu_to_be64(packed[k]);
+ k++;
+ }So we have essentially a bunch of 16-bit fields ... the above loads and swap a whole 4 of them at once. However that means not only we byteswap them individually, but we also flip the order of the fields. This is ok ?
+ field = be16_to_cpu(fixed.field[j]); + + if (field == VPHN_FIELD_UNUSED) /* All significant fields processed. */ break;
For example, we might have USED,USED,USED,UNUSED ... after the swap, we now have UNUSED,USED,USED,USED ... and we stop parsing in the above line on the first one. Or am I missing something ?
quoted hunk ↗ jump to hunk
- if (be16_to_cpup(field) & VPHN_FIELD_MSB) { + if (field & VPHN_FIELD_MSB) { /* Data is in the lower 15 bits of this field */ - unpacked[i] = cpu_to_be32( - be16_to_cpup(field) & VPHN_FIELD_MASK); - field++; + unpacked[i++] = cpu_to_be32(field & VPHN_FIELD_MASK); + j++; } else { /* Data is in the lower 15 bits of this field * concatenated with the next 16 bit field */ - unpacked[i] = *((__be32 *)field); - field += 2; + if (unlikely(j % 4 == 3)) { + /* The next field is to be copied from the next + * 64-bit input value. We must fix it now. + */ + fixed.packed[k] = cpu_to_be64(packed[k]); + k++; + } + + unpacked[i++] = *((__be32 *)&fixed.field[j]); + j += 2; } }@@ -1460,11 +1479,8 @@ static long hcall_vphn(unsigned long cpu, __be32 *associativity) long retbuf[PLPAR_HCALL9_BUFSIZE] = {0}; u64 flags = 1; int hwcpu = get_hard_smp_processor_id(cpu); - int i; rc = plpar_hcall9(H_HOME_NODE_ASSOCIATIVITY, retbuf, flags, hwcpu); - for (i = 0; i < VPHN_REGISTER_COUNT; i++) - retbuf[i] = cpu_to_be64(retbuf[i]); vphn_unpack_associativity(retbuf, associativity); return rc;