| Message ID | 20220505102705.3621584-1-nrb@linux.ibm.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | [kvm-unit-tests,v1] s390x: migration: don't run tests when facilities are not there | expand |
On Thu, 5 May 2022 12:27:05 +0200 Nico Boehr <nrb@linux.ibm.com> wrote: > In the new migration tests, there was no check whether guarded-storage and > vector facilities are there. This may lead to crashes, especially under TCG > where guarded-storage is not supported. > > Add checks for the respective facilities. Since it is possible neither > guarded-storage nor vector are there, add an additional report_pass() such that > least one PASS is reported. > > Signed-off-by: Nico Boehr <nrb@linux.ibm.com> Ooops, we missed this when reviewing the other patch I think this patch should actually be folded in the previous one, I'll do that unless there are objections > --- > s390x/migration.c | 154 +++++++++++++++++++++++++++------------------- > 1 file changed, 90 insertions(+), 64 deletions(-) > > diff --git a/s390x/migration.c b/s390x/migration.c > index 2f31fc53bf68..a45296374cd8 100644 > --- a/s390x/migration.c > +++ b/s390x/migration.c > @@ -11,6 +11,7 @@ > #include <asm/arch_def.h> > #include <asm/vector.h> > #include <asm/barrier.h> > +#include <asm/facility.h> > #include <gs.h> > #include <bitops.h> > #include <smp.h> > @@ -50,6 +51,16 @@ static void check_gs_regs(void) > report_prefix_pop(); > } > > +static bool have_vector_facility(void) > +{ > + return test_facility(129); > +} > + > +static bool have_guarded_storage_facility(void) > +{ > + return test_facility(133); > +} > + > static void test_func(void) > { > uint8_t expected_vec_contents[VEC_REGISTER_NUM][VEC_REGISTER_SIZE]; > @@ -58,74 +69,89 @@ static void test_func(void) > int i; > int vec_result = 0; > > - for (i = 0; i < VEC_REGISTER_NUM; i++) { > - vec_reg = &expected_vec_contents[i][0]; > - /* i+1 to avoid zero content */ > - memset(vec_reg, i + 1, VEC_REGISTER_SIZE); > + if (have_guarded_storage_facility()) { > + ctl_set_bit(2, CTL2_GUARDED_STORAGE); > + > + write_gs_regs(); > } > > - ctl_set_bit(0, CTL0_VECTOR); > - ctl_set_bit(2, CTL2_GUARDED_STORAGE); > - > - write_gs_regs(); > - > - /* > - * It is important loading the vector/floating point registers and > - * comparing their contents occurs in the same inline assembly block. > - * Otherwise, the compiler is allowed to re-use the registers for > - * something else in between. > - * For this very reason, this also runs on a second CPU, so all the > - * complex console stuff can be done in C on the first CPU and here we > - * just need to wait for it to set the flag. > - */ > - asm inline( > - " .machine z13\n" > - /* load vector registers: vlm handles at most 16 registers at a time */ > - " vlm 0,15, 0(%[expected_vec_reg])\n" > - " vlm 16,31, 256(%[expected_vec_reg])\n" > - /* inform CPU0 we are done, it will request migration */ > - " mvhi %[flag_thread_complete], 1\n" > - /* wait for migration to finish */ > - "0: clfhsi %[flag_migration_complete], 1\n" > - " jnz 0b\n" > - /* > - * store vector register contents in actual_vec_reg: vstm > - * handles at most 16 registers at a time > - */ > - " vstm 0,15, 0(%[actual_vec_reg])\n" > - " vstm 16,31, 256(%[actual_vec_reg])\n" > + if (have_vector_facility()) { > + for (i = 0; i < VEC_REGISTER_NUM; i++) { > + vec_reg = &expected_vec_contents[i][0]; > + /* i+1 to avoid zero content */ > + memset(vec_reg, i + 1, VEC_REGISTER_SIZE); > + } > + > + ctl_set_bit(0, CTL0_VECTOR); > + > /* > - * compare the contents in expected_vec_reg with actual_vec_reg: > - * clc handles at most 256 bytes at a time > + * It is important loading the vector/floating point registers and > + * comparing their contents occurs in the same inline assembly block. > + * Otherwise, the compiler is allowed to re-use the registers for > + * something else in between. > + * For this very reason, this also runs on a second CPU, so all the > + * complex console stuff can be done in C on the first CPU and here we > + * just need to wait for it to set the flag. > */ > - " clc 0(256, %[expected_vec_reg]), 0(%[actual_vec_reg])\n" > - " jnz 1f\n" > - " clc 256(256, %[expected_vec_reg]), 256(%[actual_vec_reg])\n" > - " jnz 1f\n" > - /* success */ > - " mvhi %[vec_result], 1\n" > - "1:" > - : > - : [expected_vec_reg] "a"(expected_vec_contents), > - [actual_vec_reg] "a"(actual_vec_contents), > - [flag_thread_complete] "Q"(flag_thread_complete), > - [flag_migration_complete] "Q"(flag_migration_complete), > - [vec_result] "Q"(vec_result) > - : "v0", "v1", "v2", "v3", "v4", "v5", "v6", "v7", "v8", "v9", > - "v10", "v11", "v12", "v13", "v14", "v15", "v16", "v17", "v18", > - "v19", "v20", "v21", "v22", "v23", "v24", "v25", "v26", "v27", > - "v28", "v29", "v30", "v31", "cc", "memory" > - ); > - > - report(vec_result, "vector contents match"); > - > - check_gs_regs(); > - > - report(stctg(0) & BIT(CTL0_VECTOR), "ctl0 vector bit set"); > - report(stctg(2) & BIT(CTL2_GUARDED_STORAGE), "ctl2 guarded-storage bit set"); > - > - ctl_clear_bit(0, CTL0_VECTOR); > - ctl_clear_bit(2, CTL2_GUARDED_STORAGE); > + asm inline( > + " .machine z13\n" > + /* load vector registers: vlm handles at most 16 registers at a time */ > + " vlm 0,15, 0(%[expected_vec_reg])\n" > + " vlm 16,31, 256(%[expected_vec_reg])\n" > + /* inform CPU0 we are done, it will request migration */ > + " mvhi %[flag_thread_complete], 1\n" > + /* wait for migration to finish */ > + "0: clfhsi %[flag_migration_complete], 1\n" > + " jnz 0b\n" > + /* > + * store vector register contents in actual_vec_reg: vstm > + * handles at most 16 registers at a time > + */ > + " vstm 0,15, 0(%[actual_vec_reg])\n" > + " vstm 16,31, 256(%[actual_vec_reg])\n" > + /* > + * compare the contents in expected_vec_reg with actual_vec_reg: > + * clc handles at most 256 bytes at a time > + */ > + " clc 0(256, %[expected_vec_reg]), 0(%[actual_vec_reg])\n" > + " jnz 1f\n" > + " clc 256(256, %[expected_vec_reg]), 256(%[actual_vec_reg])\n" > + " jnz 1f\n" > + /* success */ > + " mvhi %[vec_result], 1\n" > + "1:" > + : > + : [expected_vec_reg] "a"(expected_vec_contents), > + [actual_vec_reg] "a"(actual_vec_contents), > + [flag_thread_complete] "Q"(flag_thread_complete), > + [flag_migration_complete] "Q"(flag_migration_complete), > + [vec_result] "Q"(vec_result) > + : "v0", "v1", "v2", "v3", "v4", "v5", "v6", "v7", "v8", "v9", > + "v10", "v11", "v12", "v13", "v14", "v15", "v16", "v17", "v18", > + "v19", "v20", "v21", "v22", "v23", "v24", "v25", "v26", "v27", > + "v28", "v29", "v30", "v31", "cc", "memory" > + ); > + > + report(vec_result, "vector contents match"); > + > + report(stctg(0) & BIT(CTL0_VECTOR), "ctl0 vector bit set"); > + > + ctl_clear_bit(0, CTL0_VECTOR); > + } else { > + flag_thread_complete = 1; > + while(!flag_migration_complete) > + mb(); > + } > + > + report_pass("Migrated"); > + > + if (have_guarded_storage_facility()) { > + check_gs_regs(); > + > + report(stctg(2) & BIT(CTL2_GUARDED_STORAGE), "ctl2 guarded-storage bit set"); > + > + ctl_clear_bit(2, CTL2_GUARDED_STORAGE); > + } > > flag_thread_complete = 1; > }
On Thu, 2022-05-05 at 13:54 +0200, Claudio Imbrenda wrote: > I think this patch should actually be folded in the previous one, > I'll > do that unless there are objections Thanks. Fine for me.
diff --git a/s390x/migration.c b/s390x/migration.c index 2f31fc53bf68..a45296374cd8 100644 --- a/s390x/migration.c +++ b/s390x/migration.c @@ -11,6 +11,7 @@ #include <asm/arch_def.h> #include <asm/vector.h> #include <asm/barrier.h> +#include <asm/facility.h> #include <gs.h> #include <bitops.h> #include <smp.h> @@ -50,6 +51,16 @@ static void check_gs_regs(void) report_prefix_pop(); } +static bool have_vector_facility(void) +{ + return test_facility(129); +} + +static bool have_guarded_storage_facility(void) +{ + return test_facility(133); +} + static void test_func(void) { uint8_t expected_vec_contents[VEC_REGISTER_NUM][VEC_REGISTER_SIZE]; @@ -58,74 +69,89 @@ static void test_func(void) int i; int vec_result = 0; - for (i = 0; i < VEC_REGISTER_NUM; i++) { - vec_reg = &expected_vec_contents[i][0]; - /* i+1 to avoid zero content */ - memset(vec_reg, i + 1, VEC_REGISTER_SIZE); + if (have_guarded_storage_facility()) { + ctl_set_bit(2, CTL2_GUARDED_STORAGE); + + write_gs_regs(); } - ctl_set_bit(0, CTL0_VECTOR); - ctl_set_bit(2, CTL2_GUARDED_STORAGE); - - write_gs_regs(); - - /* - * It is important loading the vector/floating point registers and - * comparing their contents occurs in the same inline assembly block. - * Otherwise, the compiler is allowed to re-use the registers for - * something else in between. - * For this very reason, this also runs on a second CPU, so all the - * complex console stuff can be done in C on the first CPU and here we - * just need to wait for it to set the flag. - */ - asm inline( - " .machine z13\n" - /* load vector registers: vlm handles at most 16 registers at a time */ - " vlm 0,15, 0(%[expected_vec_reg])\n" - " vlm 16,31, 256(%[expected_vec_reg])\n" - /* inform CPU0 we are done, it will request migration */ - " mvhi %[flag_thread_complete], 1\n" - /* wait for migration to finish */ - "0: clfhsi %[flag_migration_complete], 1\n" - " jnz 0b\n" - /* - * store vector register contents in actual_vec_reg: vstm - * handles at most 16 registers at a time - */ - " vstm 0,15, 0(%[actual_vec_reg])\n" - " vstm 16,31, 256(%[actual_vec_reg])\n" + if (have_vector_facility()) { + for (i = 0; i < VEC_REGISTER_NUM; i++) { + vec_reg = &expected_vec_contents[i][0]; + /* i+1 to avoid zero content */ + memset(vec_reg, i + 1, VEC_REGISTER_SIZE); + } + + ctl_set_bit(0, CTL0_VECTOR); + /* - * compare the contents in expected_vec_reg with actual_vec_reg: - * clc handles at most 256 bytes at a time + * It is important loading the vector/floating point registers and + * comparing their contents occurs in the same inline assembly block. + * Otherwise, the compiler is allowed to re-use the registers for + * something else in between. + * For this very reason, this also runs on a second CPU, so all the + * complex console stuff can be done in C on the first CPU and here we + * just need to wait for it to set the flag. */ - " clc 0(256, %[expected_vec_reg]), 0(%[actual_vec_reg])\n" - " jnz 1f\n" - " clc 256(256, %[expected_vec_reg]), 256(%[actual_vec_reg])\n" - " jnz 1f\n" - /* success */ - " mvhi %[vec_result], 1\n" - "1:" - : - : [expected_vec_reg] "a"(expected_vec_contents), - [actual_vec_reg] "a"(actual_vec_contents), - [flag_thread_complete] "Q"(flag_thread_complete), - [flag_migration_complete] "Q"(flag_migration_complete), - [vec_result] "Q"(vec_result) - : "v0", "v1", "v2", "v3", "v4", "v5", "v6", "v7", "v8", "v9", - "v10", "v11", "v12", "v13", "v14", "v15", "v16", "v17", "v18", - "v19", "v20", "v21", "v22", "v23", "v24", "v25", "v26", "v27", - "v28", "v29", "v30", "v31", "cc", "memory" - ); - - report(vec_result, "vector contents match"); - - check_gs_regs(); - - report(stctg(0) & BIT(CTL0_VECTOR), "ctl0 vector bit set"); - report(stctg(2) & BIT(CTL2_GUARDED_STORAGE), "ctl2 guarded-storage bit set"); - - ctl_clear_bit(0, CTL0_VECTOR); - ctl_clear_bit(2, CTL2_GUARDED_STORAGE); + asm inline( + " .machine z13\n" + /* load vector registers: vlm handles at most 16 registers at a time */ + " vlm 0,15, 0(%[expected_vec_reg])\n" + " vlm 16,31, 256(%[expected_vec_reg])\n" + /* inform CPU0 we are done, it will request migration */ + " mvhi %[flag_thread_complete], 1\n" + /* wait for migration to finish */ + "0: clfhsi %[flag_migration_complete], 1\n" + " jnz 0b\n" + /* + * store vector register contents in actual_vec_reg: vstm + * handles at most 16 registers at a time + */ + " vstm 0,15, 0(%[actual_vec_reg])\n" + " vstm 16,31, 256(%[actual_vec_reg])\n" + /* + * compare the contents in expected_vec_reg with actual_vec_reg: + * clc handles at most 256 bytes at a time + */ + " clc 0(256, %[expected_vec_reg]), 0(%[actual_vec_reg])\n" + " jnz 1f\n" + " clc 256(256, %[expected_vec_reg]), 256(%[actual_vec_reg])\n" + " jnz 1f\n" + /* success */ + " mvhi %[vec_result], 1\n" + "1:" + : + : [expected_vec_reg] "a"(expected_vec_contents), + [actual_vec_reg] "a"(actual_vec_contents), + [flag_thread_complete] "Q"(flag_thread_complete), + [flag_migration_complete] "Q"(flag_migration_complete), + [vec_result] "Q"(vec_result) + : "v0", "v1", "v2", "v3", "v4", "v5", "v6", "v7", "v8", "v9", + "v10", "v11", "v12", "v13", "v14", "v15", "v16", "v17", "v18", + "v19", "v20", "v21", "v22", "v23", "v24", "v25", "v26", "v27", + "v28", "v29", "v30", "v31", "cc", "memory" + ); + + report(vec_result, "vector contents match"); + + report(stctg(0) & BIT(CTL0_VECTOR), "ctl0 vector bit set"); + + ctl_clear_bit(0, CTL0_VECTOR); + } else { + flag_thread_complete = 1; + while(!flag_migration_complete) + mb(); + } + + report_pass("Migrated"); + + if (have_guarded_storage_facility()) { + check_gs_regs(); + + report(stctg(2) & BIT(CTL2_GUARDED_STORAGE), "ctl2 guarded-storage bit set"); + + ctl_clear_bit(2, CTL2_GUARDED_STORAGE); + } flag_thread_complete = 1; }
In the new migration tests, there was no check whether guarded-storage and vector facilities are there. This may lead to crashes, especially under TCG where guarded-storage is not supported. Add checks for the respective facilities. Since it is possible neither guarded-storage nor vector are there, add an additional report_pass() such that least one PASS is reported. Signed-off-by: Nico Boehr <nrb@linux.ibm.com> --- s390x/migration.c | 154 +++++++++++++++++++++++++++------------------- 1 file changed, 90 insertions(+), 64 deletions(-)