| Message ID | 20190403054817.3811-1-stewart@linux.ibm.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | cpufeatures: Add tm-suspend-hypervisor-assist and tm-suspend-xer-so-bug node | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch master (050d8165ab05b6d9cdf4bfee42d9776969c77029) |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot | success | Test snowpatch/job/snowpatch-skiboot on branch master |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot-dco | success | Signed-off-by present |
On Wed, 2019-04-03 at 16:48 +1100, Stewart Smith wrote: > tm-suspend-hypervisor-assist for P9 >=DD2.2 > And a tm-suspend-xer-so-bug node for P9 DD2.2 only. > > I also treat P9P as P9 DD2.3 and add a unit test for the cpufeatures > infrastructure. > > Fixes: https://github.com/open-power/skiboot/issues/233 > Suggested-by: Paul Mackerras <paulus@ozlabs.org> > Signed-off-by: Stewart Smith <stewart@linux.ibm.com> Tested-by: Michael Neuling <mikey@neuling.org> FWIW, tested in mambo by playing with the PVR and looking at what cpu features bits get set in the early kernel boot log. Checked 2.1, 2.2 and 2.3 did what was expected. Mikey > --- > core/cpufeatures.c | 56 ++++++++++++- > core/test/Makefile.check | 1 + > core/test/run-cpufeatures.c | 155 ++++++++++++++++++++++++++++++++++++ > 3 files changed, 209 insertions(+), 3 deletions(-) > create mode 100644 core/test/run-cpufeatures.c > > diff --git a/core/cpufeatures.c b/core/cpufeatures.c > index 070419d9cfb7..530dc77f73c4 100644 > --- a/core/cpufeatures.c > +++ b/core/cpufeatures.c > @@ -1,4 +1,4 @@ > -/* Copyright 2017-2018 IBM Corp. > +/* Copyright 2017-2019 IBM Corp. > * > * Licensed under the Apache License, Version 2.0 (the "License"); > * you may not use this file except in compliance with the License. > @@ -56,8 +56,12 @@ > #define CPU_P8_DD1 (1U << 0) > #define CPU_P8_DD2 (1U << 1) > #define CPU_P9_DD1 (1U << 2) > -#define CPU_P9_DD2 (1U << 3) > +#define CPU_P9_DD2_0_1 (1U << 3) // 2.01 or 2.1 > #define CPU_P9P (1U << 4) > +#define CPU_P9_DD2_2 (1U << 5) > +#define CPU_P9_DD2_3 (1U << 6) > + > +#define CPU_P9_DD2 (CPU_P9_DD2_0_1|CPU_P9_DD2_2|CPU_P9_DD2_3|CPU_P9P) > > #define CPU_P8 (CPU_P8_DD1|CPU_P8_DD2) > #define CPU_P9 (CPU_P9_DD1|CPU_P9_DD2|CPU_P9P) > @@ -721,6 +725,39 @@ static const struct cpu_feature cpu_features_table[] = { > HV_NONE, OS_NONE, > -1, -1, -1, > NULL, }, > + > + /* > + * Due to hardware bugs in POWER9, the hypervisor needs to assist > + * guests. > + * > + * Presence of this feature indicates presence of the bug. > + * > + * See linux kernel commit 4bb3c7a0208f > + * and linux Documentation/powerpc/transactional_memory.txt > + */ > + { "tm-suspend-hypervisor-assist", > + CPU_P9_DD2_2|CPU_P9_DD2_3|CPU_P9P, > + ISA_V3_0B, USABLE_HV|USABLE_OS, > + HV_CUSTOM, OS_NONE, > + -1, -1, -1, > + NULL, }, > + > + /* > + * Due to hardware bugs in POWER9, the hypervisor can hit > + * CPU bugs in the operations it needs to do for > + * tm-suspend-hypervisor-assist. > + * > + * Presence of this "feature" means processor is affected by the bug. > + * > + * See linux kernel commit 4bb3c7a0208f > + * and linux Documentation/powerpc/transactional_memory.txt > + */ > + { "tm-suspend-xer-so-bug", > + CPU_P9_DD2_2, > + ISA_V3_0B, USABLE_HV, > + HV_CUSTOM, OS_NONE, > + -1, -1, -1, > + NULL, }, > }; > > static void add_cpu_feature_nodeps(struct dt_node *features, > @@ -905,7 +942,20 @@ void dt_add_cpufeatures(struct dt_node *root) > if (is_power9n(version) && > (PVR_VERS_MAJ(version) == 2)) { > /* P9N DD2.x */ > - cpu_feature_cpu = CPU_P9_DD2; > + switch (PVR_VERS_MIN(version)) { > + case 0: > + case 1: > + cpu_feature_cpu = CPU_P9_DD2_0_1; > + break; > + case 2: > + cpu_feature_cpu = CPU_P9_DD2_2; > + break; > + case 3: > + cpu_feature_cpu = CPU_P9_DD2_3; > + break; > + default: > + assert(0); > + } > } else { > assert(0); > } > diff --git a/core/test/Makefile.check b/core/test/Makefile.check > index 0fb585e38d2d..8e59ef00e7b6 100644 > --- a/core/test/Makefile.check > +++ b/core/test/Makefile.check > @@ -1,6 +1,7 @@ > # -*-Makefile-*- > CORE_TEST := \ > core/test/run-bitmap \ > + core/test/run-cpufeatures \ > core/test/run-device \ > core/test/run-flash-subpartition \ > core/test/run-flash-firmware-versions \ > diff --git a/core/test/run-cpufeatures.c b/core/test/run-cpufeatures.c > new file mode 100644 > index 000000000000..9db21440d6eb > --- /dev/null > +++ b/core/test/run-cpufeatures.c > @@ -0,0 +1,155 @@ > +/* Copyright 2019 IBM Corp. > + * > + * Licensed under the Apache License, Version 2.0 (the "License"); > + * you may not use this file except in compliance with the License. > + * You may obtain a copy of the License at > + * > + * http://www.apache.org/licenses/LICENSE-2.0 > + * > + * Unless required by applicable law or agreed to in writing, software > + * distributed under the License is distributed on an "AS IS" BASIS, > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or > + * implied. > + * See the License for the specific language governing permissions and > + * limitations under the License. > + */ > + > +#include <skiboot.h> > +#include <stdio.h> > +#include <stdlib.h> > +#include <stdbool.h> > + > +/* Override this for testing. */ > +#define is_rodata(p) fake_is_rodata(p) > + > +char __rodata_start[16]; > +#define __rodata_end (__rodata_start + sizeof(__rodata_start)) > + > +static inline bool fake_is_rodata(const void *p) > +{ > + return ((char *)p >= __rodata_start && (char *)p < __rodata_end); > +} > + > +#define zalloc(bytes) calloc((bytes), 1) > + > +#include "../device.c" > +#include <assert.h> > +#include "../../test/dt_common.c" > + > +#define __TEST__ > + > +static inline unsigned long mfspr(unsigned int spr); > + > +#include <ccan/str/str.c> > + > +#include "../cpufeatures.c" > + > +static unsigned long fake_pvr = PVR_TYPE_P7; > + > +static inline unsigned long mfspr(unsigned int spr) > +{ > + assert(spr == SPR_PVR); > + return fake_pvr; > +} > + > +int main(void) > +{ > + struct dt_node *dt_root; > + > + dt_root = dt_new_root(""); > + dt_add_cpufeatures(dt_root); > + dump_dt(dt_root, 0, true); > + dt_free(dt_root); > + > + fake_pvr = (PVR_TYPE_P8E << 16) | 0x100; // P8E DD1.0 > + dt_root = dt_new_root(""); > + dt_add_cpufeatures(dt_root); > + dump_dt(dt_root, 0, false); > + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/mmu- > radix") == 0); > + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/tm- > suspend-hypervisor-assist") == 0); > + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/tm- > suspend-xer-so-bug") == 0); > + dt_free(dt_root); > + > + fake_pvr = (PVR_TYPE_P8E << 16) | 0x200; // P8E DD2.0 > + dt_root = dt_new_root(""); > + dt_add_cpufeatures(dt_root); > + dump_dt(dt_root, 0, false); > + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/mmu- > radix") == 0); > + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/tm- > suspend-hypervisor-assist") == 0); > + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/tm- > suspend-xer-so-bug") == 0); > + dt_free(dt_root); > + > + fake_pvr = (PVR_TYPE_P8 << 16) | 0x100; // P8 DD1.0 > + dt_root = dt_new_root(""); > + dt_add_cpufeatures(dt_root); > + dump_dt(dt_root, 0, false); > + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/mmu- > radix") == 0); > + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/tm- > suspend-hypervisor-assist") == 0); > + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/tm- > suspend-xer-so-bug") == 0); > + dt_free(dt_root); > + > + fake_pvr = (PVR_TYPE_P8 << 16) | 0x200; // P8 DD2.0 > + dt_root = dt_new_root(""); > + dt_add_cpufeatures(dt_root); > + dump_dt(dt_root, 0, false); > + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/mmu- > radix") == 0); > + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/tm- > suspend-hypervisor-assist") == 0); > + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/tm- > suspend-xer-so-bug") == 0); > + dt_free(dt_root); > + > + fake_pvr = (PVR_TYPE_P8NVL << 16) | 0x100; // P8NVL DD1.0 > + dt_root = dt_new_root(""); > + dt_add_cpufeatures(dt_root); > + dump_dt(dt_root, 0, false); > + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/mmu- > radix") == 0); > + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/tm- > suspend-hypervisor-assist") == 0); > + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/tm- > suspend-xer-so-bug") == 0); > + dt_free(dt_root); > + > + fake_pvr = (PVR_TYPE_P9 << 16) | 0x200; // P9 DD2.0 > + dt_root = dt_new_root(""); > + dt_add_cpufeatures(dt_root); > + dump_dt(dt_root, 0, false); > + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/mmu- > radix")); > + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/tm- > suspend-hypervisor-assist") == 0); > + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/tm- > suspend-xer-so-bug") == 0); > + dt_free(dt_root); > + > + fake_pvr = (PVR_TYPE_P9 << 16) | 0x201; // P9 DD2.1 > + dt_root = dt_new_root(""); > + dt_add_cpufeatures(dt_root); > + dump_dt(dt_root, 0, false); > + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/mmu- > radix")); > + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/tm- > suspend-hypervisor-assist") == 0); > + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/tm- > suspend-xer-so-bug") == 0); > + dt_free(dt_root); > + > + fake_pvr = (PVR_TYPE_P9 << 16) | 0x202; // P9 DD2.2 > + dt_root = dt_new_root(""); > + dt_add_cpufeatures(dt_root); > + dump_dt(dt_root, 0, false); > + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/mmu- > radix")); > + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/tm- > suspend-hypervisor-assist") != 0); > + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/tm- > suspend-xer-so-bug") != 0); > + dt_free(dt_root); > + > + fake_pvr = (PVR_TYPE_P9 << 16) | 0x203; // P9 DD2.3 > + dt_root = dt_new_root(""); > + dt_add_cpufeatures(dt_root); > + dump_dt(dt_root, 0, false); > + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/mmu- > radix")); > + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/tm- > suspend-hypervisor-assist") != 0); > + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/tm- > suspend-xer-so-bug") == 0); > + dt_free(dt_root); > + > + fake_pvr = (PVR_TYPE_P9P << 16) | 0x100; // P9P DD1.0 > + dt_root = dt_new_root(""); > + dt_add_cpufeatures(dt_root); > + dump_dt(dt_root, 0, false); > + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/mmu- > radix")); > + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/tm- > suspend-hypervisor-assist") != 0); > + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/tm- > suspend-xer-so-bug") == 0); > + dt_free(dt_root); > + > + exit(EXIT_SUCCESS); > +}
* Michael Neuling <mikey@neuling.org> [2019-04-04 17:30:38]: > On Wed, 2019-04-03 at 16:48 +1100, Stewart Smith wrote: > > tm-suspend-hypervisor-assist for P9 >=DD2.2 > > And a tm-suspend-xer-so-bug node for P9 DD2.2 only. > > > > I also treat P9P as P9 DD2.3 and add a unit test for the cpufeatures > > infrastructure. > > > > Fixes: https://github.com/open-power/skiboot/issues/233 > > Suggested-by: Paul Mackerras <paulus@ozlabs.org> > > Signed-off-by: Stewart Smith <stewart@linux.ibm.com> > > Tested-by: Michael Neuling <mikey@neuling.org> > > FWIW, tested in mambo by playing with the PVR and looking at what cpu features > bits get set in the early kernel boot log. Checked 2.1, 2.2 and 2.3 did what was > expected. Reviewed-by: Vaidyanathan Srinivasan <svaidy@linux.vnet.ibm.com> Tested-by: Vaidyanathan Srinivasan <svaidy@linux.vnet.ibm.com> The patch added the right flag as expected. I tested on DD2.2 and DD2.3 P9 hardware. P9 DD2.3: Has only the tm-suspend-hypervisor-assist flag # lsprop /proc/device-tree/cpus/ibm,powerpc-cpu-features/tm-suspend-hypervisor-assist/ isa 00000bb8 (3000) hv-support 00000000 phandle 0000015d (349) usable-privilege 00000006 name "tm-suspend-hypervisor-assist" P9 DD2.2: Has both the flags. # lsprop /proc/device-tree/cpus/ibm,powerpc-cpu-features/tm-suspend-hypervisor-assist/ isa 00000bb8 (3000) hv-support 00000000 phandle 0000015d (349) usable-privilege 00000006 name "tm-suspend-hypervisor-assist" # lsprop /proc/device-tree/cpus/ibm,powerpc-cpu-features/tm-suspend-xer-so-bug/ isa 00000bb8 (3000) hv-support 00000000 phandle 0000015e (350) usable-privilege 00000004 name "tm-suspend-xer-so-bug" --Vaidy
Stewart Smith <stewart@linux.ibm.com> writes: > tm-suspend-hypervisor-assist for P9 >=DD2.2 > And a tm-suspend-xer-so-bug node for P9 DD2.2 only. > > I also treat P9P as P9 DD2.3 and add a unit test for the cpufeatures > infrastructure. > > Fixes: https://github.com/open-power/skiboot/issues/233 > Suggested-by: Paul Mackerras <paulus@ozlabs.org> > Signed-off-by: Stewart Smith <stewart@linux.ibm.com> > --- > core/cpufeatures.c | 56 ++++++++++++- > core/test/Makefile.check | 1 + > core/test/run-cpufeatures.c | 155 ++++++++++++++++++++++++++++++++++++ > 3 files changed, 209 insertions(+), 3 deletions(-) > create mode 100644 core/test/run-cpufeatures.c > > diff --git a/core/cpufeatures.c b/core/cpufeatures.c > index 070419d9cfb7..530dc77f73c4 100644 > --- a/core/cpufeatures.c > +++ b/core/cpufeatures.c > @@ -1,4 +1,4 @@ > -/* Copyright 2017-2018 IBM Corp. > +/* Copyright 2017-2019 IBM Corp. > * > * Licensed under the Apache License, Version 2.0 (the "License"); > * you may not use this file except in compliance with the License. > @@ -56,8 +56,12 @@ > #define CPU_P8_DD1 (1U << 0) > #define CPU_P8_DD2 (1U << 1) > #define CPU_P9_DD1 (1U << 2) > -#define CPU_P9_DD2 (1U << 3) > +#define CPU_P9_DD2_0_1 (1U << 3) // 2.01 or 2.1 > #define CPU_P9P (1U << 4) > +#define CPU_P9_DD2_2 (1U << 5) > +#define CPU_P9_DD2_3 (1U << 6) > + > +#define CPU_P9_DD2 (CPU_P9_DD2_0_1|CPU_P9_DD2_2|CPU_P9_DD2_3|CPU_P9P) > > #define CPU_P8 (CPU_P8_DD1|CPU_P8_DD2) > #define CPU_P9 (CPU_P9_DD1|CPU_P9_DD2|CPU_P9P) > @@ -721,6 +725,39 @@ static const struct cpu_feature cpu_features_table[] = { > HV_NONE, OS_NONE, > -1, -1, -1, > NULL, }, > + > + /* > + * Due to hardware bugs in POWER9, the hypervisor needs to assist > + * guests. > + * > + * Presence of this feature indicates presence of the bug. > + * > + * See linux kernel commit 4bb3c7a0208f > + * and linux Documentation/powerpc/transactional_memory.txt > + */ > + { "tm-suspend-hypervisor-assist", > + CPU_P9_DD2_2|CPU_P9_DD2_3|CPU_P9P, > + ISA_V3_0B, USABLE_HV|USABLE_OS, After chatting with Nick quickly on Slack, he suggests dropping the USABLE_OS. Another option raised was to also use `errata-` as a prefix for these. Thoughts from the gallery?
Stewart Smith's on April 8, 2019 2:16 pm: > Stewart Smith <stewart@linux.ibm.com> writes: > >> @@ -721,6 +725,39 @@ static const struct cpu_feature cpu_features_table[] = { >> HV_NONE, OS_NONE, >> -1, -1, -1, >> NULL, }, >> + >> + /* >> + * Due to hardware bugs in POWER9, the hypervisor needs to assist >> + * guests. >> + * >> + * Presence of this feature indicates presence of the bug. >> + * >> + * See linux kernel commit 4bb3c7a0208f >> + * and linux Documentation/powerpc/transactional_memory.txt >> + */ >> + { "tm-suspend-hypervisor-assist", >> + CPU_P9_DD2_2|CPU_P9_DD2_3|CPU_P9P, >> + ISA_V3_0B, USABLE_HV|USABLE_OS, > > After chatting with Nick quickly on Slack, he suggests dropping the > USABLE_OS. > > Another option raised was to also use `errata-` as a prefix for > these. Thoughts from the gallery? Technically it makes no difference of course. I would like to add it as it gives more information to the name, and generally the rest of the features are positive (certain facility exists) rather than negative. Thanks, Nick
On 04/03/2019 11:18 AM, Stewart Smith wrote: > tm-suspend-hypervisor-assist for P9 >=DD2.2 > And a tm-suspend-xer-so-bug node for P9 DD2.2 only. .../... > > static void add_cpu_feature_nodeps(struct dt_node *features, > @@ -905,7 +942,20 @@ void dt_add_cpufeatures(struct dt_node *root) > if (is_power9n(version) && > (PVR_VERS_MAJ(version) == 2)) { > /* P9N DD2.x */ > - cpu_feature_cpu = CPU_P9_DD2; > + switch (PVR_VERS_MIN(version)) { > + case 0: > + case 1: > + cpu_feature_cpu = CPU_P9_DD2_0_1; > + break; > + case 2: > + cpu_feature_cpu = CPU_P9_DD2_2; > + break; > + case 3: > + cpu_feature_cpu = CPU_P9_DD2_3; > + break; > + default: > + assert(0); Better add warning here and use "cpu_feature_cpu=DD2_3"? -Vasant
Stewart Smith <stewart@linux.ibm.com> writes: > Stewart Smith <stewart@linux.ibm.com> writes: > >> tm-suspend-hypervisor-assist for P9 >=DD2.2 >> And a tm-suspend-xer-so-bug node for P9 DD2.2 only. >> >> I also treat P9P as P9 DD2.3 and add a unit test for the cpufeatures >> infrastructure. >> >> Fixes: https://github.com/open-power/skiboot/issues/233 >> Suggested-by: Paul Mackerras <paulus@ozlabs.org> >> Signed-off-by: Stewart Smith <stewart@linux.ibm.com> >> --- >> core/cpufeatures.c | 56 ++++++++++++- >> core/test/Makefile.check | 1 + >> core/test/run-cpufeatures.c | 155 ++++++++++++++++++++++++++++++++++++ >> 3 files changed, 209 insertions(+), 3 deletions(-) >> create mode 100644 core/test/run-cpufeatures.c >> >> diff --git a/core/cpufeatures.c b/core/cpufeatures.c >> index 070419d9cfb7..530dc77f73c4 100644 >> --- a/core/cpufeatures.c >> +++ b/core/cpufeatures.c >> @@ -721,6 +725,39 @@ static const struct cpu_feature cpu_features_table[] = { >> HV_NONE, OS_NONE, >> -1, -1, -1, >> NULL, }, >> + >> + /* >> + * Due to hardware bugs in POWER9, the hypervisor needs to assist >> + * guests. >> + * >> + * Presence of this feature indicates presence of the bug. >> + * >> + * See linux kernel commit 4bb3c7a0208f >> + * and linux Documentation/powerpc/transactional_memory.txt >> + */ >> + { "tm-suspend-hypervisor-assist", >> + CPU_P9_DD2_2|CPU_P9_DD2_3|CPU_P9P, >> + ISA_V3_0B, USABLE_HV|USABLE_OS, > > After chatting with Nick quickly on Slack, he suggests dropping the > USABLE_OS. > > Another option raised was to also use `errata-` as a prefix for > these. Thoughts from the gallery? +1 from me. cheers
Stewart Smith <stewart@linux.ibm.com> writes: > tm-suspend-hypervisor-assist for P9 >=DD2.2 > And a tm-suspend-xer-so-bug node for P9 DD2.2 only. > > I also treat P9P as P9 DD2.3 and add a unit test for the cpufeatures > infrastructure. > > Fixes: https://github.com/open-power/skiboot/issues/233 Merged to master as of 24268c7662068c276396220a68bb32d17715ebba with the USABLE_OS change as suggested by Nick. In future, a `errata-` prefix is probably a good idea, but since these names were already out there it'll have to be an errata that we don't have the errata prefix. This seems somewhat appropriate for TM.
On Mon, Apr 08, 2019 at 02:16:54PM +1000, Stewart Smith wrote: > Stewart Smith <stewart@linux.ibm.com> writes: > > > tm-suspend-hypervisor-assist for P9 >=DD2.2 > > And a tm-suspend-xer-so-bug node for P9 DD2.2 only. > > > > I also treat P9P as P9 DD2.3 and add a unit test for the cpufeatures > > infrastructure. > > + { "tm-suspend-hypervisor-assist", > > + CPU_P9_DD2_2|CPU_P9_DD2_3|CPU_P9P, > > + ISA_V3_0B, USABLE_HV|USABLE_OS, > > After chatting with Nick quickly on Slack, he suggests dropping the > USABLE_OS. That seems right. > Another option raised was to also use `errata-` as a prefix for > these. Thoughts from the gallery? I'd prefer not, because existing kernels are looking for the "tm-suspend-hypervisor-assist" and "tm-suspend-xer-so-bug" names. Paul.
Paul Mackerras <paulus@ozlabs.org> writes: > On Mon, Apr 08, 2019 at 02:16:54PM +1000, Stewart Smith wrote: >> Stewart Smith <stewart@linux.ibm.com> writes: >> >> > tm-suspend-hypervisor-assist for P9 >=DD2.2 >> > And a tm-suspend-xer-so-bug node for P9 DD2.2 only. >> > >> > I also treat P9P as P9 DD2.3 and add a unit test for the cpufeatures >> > infrastructure. > >> > + { "tm-suspend-hypervisor-assist", >> > + CPU_P9_DD2_2|CPU_P9_DD2_3|CPU_P9P, >> > + ISA_V3_0B, USABLE_HV|USABLE_OS, >> >> After chatting with Nick quickly on Slack, he suggests dropping the >> USABLE_OS. > > That seems right. > >> Another option raised was to also use `errata-` as a prefix for >> these. Thoughts from the gallery? > > I'd prefer not, because existing kernels are looking for the > "tm-suspend-hypervisor-assist" and "tm-suspend-xer-so-bug" names. Yeah, I think it might be something for future errata-type "features".
diff --git a/core/cpufeatures.c b/core/cpufeatures.c index 070419d9cfb7..530dc77f73c4 100644 --- a/core/cpufeatures.c +++ b/core/cpufeatures.c @@ -1,4 +1,4 @@ -/* Copyright 2017-2018 IBM Corp. +/* Copyright 2017-2019 IBM Corp. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -56,8 +56,12 @@ #define CPU_P8_DD1 (1U << 0) #define CPU_P8_DD2 (1U << 1) #define CPU_P9_DD1 (1U << 2) -#define CPU_P9_DD2 (1U << 3) +#define CPU_P9_DD2_0_1 (1U << 3) // 2.01 or 2.1 #define CPU_P9P (1U << 4) +#define CPU_P9_DD2_2 (1U << 5) +#define CPU_P9_DD2_3 (1U << 6) + +#define CPU_P9_DD2 (CPU_P9_DD2_0_1|CPU_P9_DD2_2|CPU_P9_DD2_3|CPU_P9P) #define CPU_P8 (CPU_P8_DD1|CPU_P8_DD2) #define CPU_P9 (CPU_P9_DD1|CPU_P9_DD2|CPU_P9P) @@ -721,6 +725,39 @@ static const struct cpu_feature cpu_features_table[] = { HV_NONE, OS_NONE, -1, -1, -1, NULL, }, + + /* + * Due to hardware bugs in POWER9, the hypervisor needs to assist + * guests. + * + * Presence of this feature indicates presence of the bug. + * + * See linux kernel commit 4bb3c7a0208f + * and linux Documentation/powerpc/transactional_memory.txt + */ + { "tm-suspend-hypervisor-assist", + CPU_P9_DD2_2|CPU_P9_DD2_3|CPU_P9P, + ISA_V3_0B, USABLE_HV|USABLE_OS, + HV_CUSTOM, OS_NONE, + -1, -1, -1, + NULL, }, + + /* + * Due to hardware bugs in POWER9, the hypervisor can hit + * CPU bugs in the operations it needs to do for + * tm-suspend-hypervisor-assist. + * + * Presence of this "feature" means processor is affected by the bug. + * + * See linux kernel commit 4bb3c7a0208f + * and linux Documentation/powerpc/transactional_memory.txt + */ + { "tm-suspend-xer-so-bug", + CPU_P9_DD2_2, + ISA_V3_0B, USABLE_HV, + HV_CUSTOM, OS_NONE, + -1, -1, -1, + NULL, }, }; static void add_cpu_feature_nodeps(struct dt_node *features, @@ -905,7 +942,20 @@ void dt_add_cpufeatures(struct dt_node *root) if (is_power9n(version) && (PVR_VERS_MAJ(version) == 2)) { /* P9N DD2.x */ - cpu_feature_cpu = CPU_P9_DD2; + switch (PVR_VERS_MIN(version)) { + case 0: + case 1: + cpu_feature_cpu = CPU_P9_DD2_0_1; + break; + case 2: + cpu_feature_cpu = CPU_P9_DD2_2; + break; + case 3: + cpu_feature_cpu = CPU_P9_DD2_3; + break; + default: + assert(0); + } } else { assert(0); } diff --git a/core/test/Makefile.check b/core/test/Makefile.check index 0fb585e38d2d..8e59ef00e7b6 100644 --- a/core/test/Makefile.check +++ b/core/test/Makefile.check @@ -1,6 +1,7 @@ # -*-Makefile-*- CORE_TEST := \ core/test/run-bitmap \ + core/test/run-cpufeatures \ core/test/run-device \ core/test/run-flash-subpartition \ core/test/run-flash-firmware-versions \ diff --git a/core/test/run-cpufeatures.c b/core/test/run-cpufeatures.c new file mode 100644 index 000000000000..9db21440d6eb --- /dev/null +++ b/core/test/run-cpufeatures.c @@ -0,0 +1,155 @@ +/* Copyright 2019 IBM Corp. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or + * implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include <skiboot.h> +#include <stdio.h> +#include <stdlib.h> +#include <stdbool.h> + +/* Override this for testing. */ +#define is_rodata(p) fake_is_rodata(p) + +char __rodata_start[16]; +#define __rodata_end (__rodata_start + sizeof(__rodata_start)) + +static inline bool fake_is_rodata(const void *p) +{ + return ((char *)p >= __rodata_start && (char *)p < __rodata_end); +} + +#define zalloc(bytes) calloc((bytes), 1) + +#include "../device.c" +#include <assert.h> +#include "../../test/dt_common.c" + +#define __TEST__ + +static inline unsigned long mfspr(unsigned int spr); + +#include <ccan/str/str.c> + +#include "../cpufeatures.c" + +static unsigned long fake_pvr = PVR_TYPE_P7; + +static inline unsigned long mfspr(unsigned int spr) +{ + assert(spr == SPR_PVR); + return fake_pvr; +} + +int main(void) +{ + struct dt_node *dt_root; + + dt_root = dt_new_root(""); + dt_add_cpufeatures(dt_root); + dump_dt(dt_root, 0, true); + dt_free(dt_root); + + fake_pvr = (PVR_TYPE_P8E << 16) | 0x100; // P8E DD1.0 + dt_root = dt_new_root(""); + dt_add_cpufeatures(dt_root); + dump_dt(dt_root, 0, false); + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/mmu-radix") == 0); + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/tm-suspend-hypervisor-assist") == 0); + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/tm-suspend-xer-so-bug") == 0); + dt_free(dt_root); + + fake_pvr = (PVR_TYPE_P8E << 16) | 0x200; // P8E DD2.0 + dt_root = dt_new_root(""); + dt_add_cpufeatures(dt_root); + dump_dt(dt_root, 0, false); + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/mmu-radix") == 0); + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/tm-suspend-hypervisor-assist") == 0); + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/tm-suspend-xer-so-bug") == 0); + dt_free(dt_root); + + fake_pvr = (PVR_TYPE_P8 << 16) | 0x100; // P8 DD1.0 + dt_root = dt_new_root(""); + dt_add_cpufeatures(dt_root); + dump_dt(dt_root, 0, false); + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/mmu-radix") == 0); + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/tm-suspend-hypervisor-assist") == 0); + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/tm-suspend-xer-so-bug") == 0); + dt_free(dt_root); + + fake_pvr = (PVR_TYPE_P8 << 16) | 0x200; // P8 DD2.0 + dt_root = dt_new_root(""); + dt_add_cpufeatures(dt_root); + dump_dt(dt_root, 0, false); + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/mmu-radix") == 0); + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/tm-suspend-hypervisor-assist") == 0); + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/tm-suspend-xer-so-bug") == 0); + dt_free(dt_root); + + fake_pvr = (PVR_TYPE_P8NVL << 16) | 0x100; // P8NVL DD1.0 + dt_root = dt_new_root(""); + dt_add_cpufeatures(dt_root); + dump_dt(dt_root, 0, false); + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/mmu-radix") == 0); + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/tm-suspend-hypervisor-assist") == 0); + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/tm-suspend-xer-so-bug") == 0); + dt_free(dt_root); + + fake_pvr = (PVR_TYPE_P9 << 16) | 0x200; // P9 DD2.0 + dt_root = dt_new_root(""); + dt_add_cpufeatures(dt_root); + dump_dt(dt_root, 0, false); + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/mmu-radix")); + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/tm-suspend-hypervisor-assist") == 0); + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/tm-suspend-xer-so-bug") == 0); + dt_free(dt_root); + + fake_pvr = (PVR_TYPE_P9 << 16) | 0x201; // P9 DD2.1 + dt_root = dt_new_root(""); + dt_add_cpufeatures(dt_root); + dump_dt(dt_root, 0, false); + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/mmu-radix")); + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/tm-suspend-hypervisor-assist") == 0); + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/tm-suspend-xer-so-bug") == 0); + dt_free(dt_root); + + fake_pvr = (PVR_TYPE_P9 << 16) | 0x202; // P9 DD2.2 + dt_root = dt_new_root(""); + dt_add_cpufeatures(dt_root); + dump_dt(dt_root, 0, false); + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/mmu-radix")); + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/tm-suspend-hypervisor-assist") != 0); + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/tm-suspend-xer-so-bug") != 0); + dt_free(dt_root); + + fake_pvr = (PVR_TYPE_P9 << 16) | 0x203; // P9 DD2.3 + dt_root = dt_new_root(""); + dt_add_cpufeatures(dt_root); + dump_dt(dt_root, 0, false); + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/mmu-radix")); + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/tm-suspend-hypervisor-assist") != 0); + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/tm-suspend-xer-so-bug") == 0); + dt_free(dt_root); + + fake_pvr = (PVR_TYPE_P9P << 16) | 0x100; // P9P DD1.0 + dt_root = dt_new_root(""); + dt_add_cpufeatures(dt_root); + dump_dt(dt_root, 0, false); + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/mmu-radix")); + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/tm-suspend-hypervisor-assist") != 0); + assert(dt_find_by_path(dt_root, "cpus/ibm,powerpc-cpu-features/tm-suspend-xer-so-bug") == 0); + dt_free(dt_root); + + exit(EXIT_SUCCESS); +}
tm-suspend-hypervisor-assist for P9 >=DD2.2 And a tm-suspend-xer-so-bug node for P9 DD2.2 only. I also treat P9P as P9 DD2.3 and add a unit test for the cpufeatures infrastructure. Fixes: https://github.com/open-power/skiboot/issues/233 Suggested-by: Paul Mackerras <paulus@ozlabs.org> Signed-off-by: Stewart Smith <stewart@linux.ibm.com> --- core/cpufeatures.c | 56 ++++++++++++- core/test/Makefile.check | 1 + core/test/run-cpufeatures.c | 155 ++++++++++++++++++++++++++++++++++++ 3 files changed, 209 insertions(+), 3 deletions(-) create mode 100644 core/test/run-cpufeatures.c