VPHN parsing fixes
cover VPHN parsing fixes
The following commit fixed an endianness issue in the VPHN code:
commit 5c9fb1899400096c6818181c525897a31d57e488
Author: Greg Kurz <[email protected]> Date: Wed Oct 15 12:42:58 2014 +0200
powerpc/vphn: NUMA node code expects big-endian
It was discussed at the time that we should patch the parsing code instead of boldly fixing all the values returned by the hypervisor. It is the goal of this series.
I have an extra question: PAPR+ says that H_HOME_NODE_ASSOCIATIVITY is supposed
to populate registers R4 to R9 with 16-bit or 32-bit values. This means that we
could theorically get 24 associativity domain numbers. According to this commentthe code is limited to 12 though:
/*
* 6 64-bit registers unpacked into 12 32-bit associativity values. To form
* the complete property we have to add the length in the first cell. */
#define VPHN_ASSOC_BUFSIZE (VPHN_REGISTER_COUNT*sizeof(u64)/sizeof(u32) + 1)
I could find no justification for the fact that we don't expect the registers to hold 16-bit relevant numbers only. Have I missed something ?
---
Greg Kurz (3):
powerpc/vphn: clarify the H_HOME_NODE_ASSOCIATIVITY API powerpc/vphn: simplify the parsing code
powerpc/vphn: move endianness fixing to vphn_unpack_associativity()
arch/powerpc/mm/numa.c | 61 ++++++++++++++++++++++++++++++------------------ 1 file changed, 38 insertions(+), 23 deletions(-)
--
Greg
PATCH 1/3 powerpc/vphn: clarify the H_HOME_NODE_ASSOCIATIVITY API
The number of values returned by the H_HOME_NODE_ASSOCIATIVITY h_call deserves to be explicitly defined, for a better understanding of the code.
--- arch/powerpc/mm/numa.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
arch/powerpc/mm/numa.c +6 -2
@@ -1401,11 +1401,15 @@ static int update_cpu_associativity_changes_mask(void)return cpumask_weight (changes); } +/* The H_HOME_NODE_ASSOCIATIVITY h_call returns 6 64-bit registers. +*/ +#define VPHN_REGISTER_COUNT 6 +/* * 6 64-bit registers unpacked into 12 32-bit associativity values. To form * the complete property we have to add the length in the first cell. */ -#define VPHN_ASSOC_BUFSIZE (6*sizeof(u64)/sizeof(u32) + 1) +#define VPHN_ASSOC_BUFSIZE (VPHN_REGISTER_COUNT*sizeof(u64)/sizeof(u32) + 1)/* * Convert the associativity domain numbers returned from the hypervisor
@@ -1463,7 +1467,7 @@ static long hcall_vphn(unsigned long cpu, __be32 *associativity)int i; rc =plpar_hcall9 (H_HOME_NODE_ASSOCIATIVITY , retbuf, flags, hwcpu); -for (i =0 ; i <6 ; i++) +for (i =0 ; i <VPHN_REGISTER_COUNT ; i++) retbuf[i] =cpu_to_be64 (retbuf[i]);vphn_unpack_associativity (retbuf, associativity);
PATCH 2/3 powerpc/vphn: simplify the parsing code
In reply to Greg KurzAccording to PAPR+ 14.11.6.1 H_HOME_NODE_ASSOCIATIVITY, the hypervisor is supposed to pack significant fields first and fill the remaining unused fields with "all ones". It means that the first unused field can be viewed as an end-of-list marker.
The "ibm,associativity" property in the DT isn't padded with ones and no code in arch/powerpc/mm/numa.c seems to expect the associativity array to be padded either.
This patch simply ends the parsing when we reach the first unused field.
--- arch/powerpc/mm/numa.c | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-)
arch/powerpc/mm/numa.c +8 -12
@@ -1417,7 +1417,7 @@ static int update_cpu_associativity_changes_mask(void)*/ static int vphn_unpack_associativity (const long *packed,__be32 *unpacked) { -int i, nr_assoc_doms =0 ; +int i;const __be16 *field = (const __be16 *) packed;#define VPHN_FIELD_UNUSED (0xffff)
@@ -1425,33 +1425,29 @@ static int vphn_unpack_associativity(const long *packed, __be32 *unpacked)#define VPHN_FIELD_MASK (~VPHN_FIELD_MSB)for (i =1 ; i <VPHN_ASSOC_BUFSIZE ; i++) { -if (be16_to_cpup (field) ==VPHN_FIELD_UNUSED ) { -/* All significant fields processed, and remaining -* fields contain the reserved value of all 1's. -* Just store them. +if (be16_to_cpup (field) ==VPHN_FIELD_UNUSED ) +/* All significant fields processed. */ - unpacked[i] = *((__be32 *)field); - field +=2 ; - }else if (be16_to_cpup (field) &VPHN_FIELD_MSB ) { +break ; + +if (be16_to_cpup (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++; - nr_assoc_doms++; }else {/* Data is in the lower 15 bits of this field * concatenated with the next 16 bit field */ unpacked[i] = *((__be32 *)field); field +=2 ; - nr_assoc_doms++; } }/* The first cell contains the length of the property */ - unpacked[0 ] =cpu_to_be32 (nr_assoc_doms); + unpacked[0 ] =cpu_to_be32 (i -1 ); -return nr_assoc_doms; +return i -1 ; }/*
PATCH 3/3 powerpc/vphn: move endianness fixing to vphn_unpack_associativity()
In reply to Greg KurzThe 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.
--- arch/powerpc/mm/numa.c | 42 +++++++++++++++++++++++++++++------------- 1 file changed, 29 insertions(+), 13 deletions(-)
arch/powerpc/mm/numa.c +29 -13
@@ -1417,30 +1417,49 @@ static int update_cpu_associativity_changes_mask(void)*/ static int vphn_unpack_associativity (const long *packed,__be32 *unpacked) { -int i; -const __be16 *field = (const __be16 *) packed; +int i, j, k; +union { +__be64 packed[VPHN_REGISTER_COUNT ]; +__be16 field[VPHN_REGISTER_COUNT *4 ]; + } fixed;#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++; + } + + field =be16_to_cpu (fixed.field[j]); + +if (field ==VPHN_FIELD_UNUSED )/* All significant fields processed. */ break ; -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;