| Message ID | 20221201084642.3747014-2-nrb@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | s390x: test storage keys during migration | expand |
On Thu, 1 Dec 2022 09:46:40 +0100 Nico Boehr <nrb@linux.ibm.com> wrote: > Upcoming changes will add a test which is very similar to the existing > skey migration test. To reduce code duplication, move the common > functions to a library which can be re-used by both tests. > > Signed-off-by: Nico Boehr <nrb@linux.ibm.com> a few nits, otherwise looks pretty straightforward > --- > lib/s390x/skey.c | 92 ++++++++++++++++++++++++++++++++++++++++++ > lib/s390x/skey.h | 32 +++++++++++++++ > s390x/Makefile | 1 + > s390x/migration-skey.c | 44 +++----------------- > 4 files changed, 131 insertions(+), 38 deletions(-) > create mode 100644 lib/s390x/skey.c > create mode 100644 lib/s390x/skey.h > > diff --git a/lib/s390x/skey.c b/lib/s390x/skey.c > new file mode 100644 > index 000000000000..100f0949a244 > --- /dev/null > +++ b/lib/s390x/skey.c > @@ -0,0 +1,92 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Storage key migration test library > + * > + * Copyright IBM Corp. 2022 > + * > + * Authors: > + * Nico Boehr <nrb@linux.ibm.com> > + */ > + > +#include <libcflat.h> > +#include <asm/facility.h> > +#include <asm/mem.h> > +#include <skey.h> > + > +/* > + * Set storage keys on pagebuf. surely you should explain better what the function does (e.g. how are you setting the keys and why) > + * pagebuf must point to page_count consecutive pages. > + */ > +void skey_set_keys(uint8_t *pagebuf, unsigned long page_count) this name does not make clear what the function is doing. at first one would think that it sets the same key for all pages. maybe something like set_storage_keys_test_pattern or skey_set_test_pattern or something like that > +{ > + unsigned char key_to_set; > + unsigned long i; > + > + for (i = 0; i < page_count; i++) { > + /* > + * Storage keys are 7 bit, lowest bit is always returned as zero > + * by iske. > + * This loop will set all 7 bits which means we set fetch > + * protection as well as reference and change indication for > + * some keys. > + */ > + key_to_set = i * 2; > + set_storage_key(pagebuf + i * PAGE_SIZE, key_to_set, 1); why not just i * 2 instead of using key_to_set ? > + } > +} > + > +/* > + * Verify storage keys on pagebuf. > + * Storage keys must have been set by skey_set_keys on pagebuf before. > + * > + * If storage keys match the expected result, will return a skey_verify_result > + * with verify_failed false. All other fields are then invalid. > + * If there is a mismatch, returned struct will have verify_failed true and will > + * be filled with the details on the first mismatch encountered. > + */ > +struct skey_verify_result skey_verify_keys(uint8_t *pagebuf, unsigned long page_count) and here then adjust the function name accordingly > +{ > + union skey expected_key, actual_key; > + struct skey_verify_result result = { > + .verify_failed = true > + }; > + uint8_t *cur_page; > + unsigned long i; > + > + for (i = 0; i < page_count; i++) { > + cur_page = pagebuf + i * PAGE_SIZE; > + actual_key.val = get_storage_key(cur_page); > + expected_key.val = i * 2; > + > + /* > + * The PoP neither gives a guarantee that the reference bit is > + * accurate nor that it won't be cleared by hardware. Hence we > + * don't rely on it and just clear the bits to avoid compare > + * errors. > + */ > + actual_key.str.rf = 0; > + expected_key.str.rf = 0; > + > + if (actual_key.val != expected_key.val) { > + result.expected_key.val = expected_key.val; > + result.actual_key.val = actual_key.val; > + result.page_mismatch_idx = i; > + result.page_mismatch_addr = (unsigned long)cur_page; > + return result; > + } > + } > + > + result.verify_failed = false; > + return result; > +} > + > +void skey_report_verify(struct skey_verify_result * const result) > +{ > + if (result->verify_failed) > + report_fail("page skey mismatch: first page idx = %lu, addr = 0x%lx, " > + "expected_key = 0x%x, actual_key = 0x%x", > + result->page_mismatch_idx, result->page_mismatch_addr, > + result->expected_key.val, result->actual_key.val); > + else > + report_pass("skeys match"); > +} > diff --git a/lib/s390x/skey.h b/lib/s390x/skey.h > new file mode 100644 > index 000000000000..a0f8caa1270b > --- /dev/null > +++ b/lib/s390x/skey.h > @@ -0,0 +1,32 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Storage key migration test library > + * > + * Copyright IBM Corp. 2022 > + * > + * Authors: > + * Nico Boehr <nrb@linux.ibm.com> > + */ > +#ifndef S390X_SKEY_H > +#define S390X_SKEY_H > + > +#include <libcflat.h> > +#include <asm/facility.h> > +#include <asm/page.h> > +#include <asm/mem.h> > + > +struct skey_verify_result { > + bool verify_failed; > + union skey expected_key; > + union skey actual_key; > + unsigned long page_mismatch_idx; > + unsigned long page_mismatch_addr; > +}; > + > +void skey_set_keys(uint8_t *pagebuf, unsigned long page_count); > + > +struct skey_verify_result skey_verify_keys(uint8_t *pagebuf, unsigned long page_count); > + > +void skey_report_verify(struct skey_verify_result * const result); > + > +#endif /* S390X_SKEY_H */ > diff --git a/s390x/Makefile b/s390x/Makefile > index bf1504f9d58c..d097b7071dfb 100644 > --- a/s390x/Makefile > +++ b/s390x/Makefile > @@ -99,6 +99,7 @@ cflatobjs += lib/s390x/malloc_io.o > cflatobjs += lib/s390x/uv.o > cflatobjs += lib/s390x/sie.o > cflatobjs += lib/s390x/fault.o > +cflatobjs += lib/s390x/skey.o > > OBJDIRS += lib/s390x > > diff --git a/s390x/migration-skey.c b/s390x/migration-skey.c > index b7bd82581abe..fed6fc1ed0f8 100644 > --- a/s390x/migration-skey.c > +++ b/s390x/migration-skey.c > @@ -10,55 +10,23 @@ > > #include <libcflat.h> > #include <asm/facility.h> > -#include <asm/page.h> > -#include <asm/mem.h> > -#include <asm/interrupt.h> > #include <hardware.h> > +#include <skey.h> > > #define NUM_PAGES 128 > -static uint8_t pagebuf[NUM_PAGES][PAGE_SIZE] __attribute__((aligned(PAGE_SIZE))); > +static uint8_t pagebuf[NUM_PAGES * PAGE_SIZE] __attribute__((aligned(PAGE_SIZE))); > > static void test_migration(void) > { > - union skey expected_key, actual_key; > - int i, key_to_set, key_mismatches = 0; > + struct skey_verify_result result; > > - for (i = 0; i < NUM_PAGES; i++) { > - /* > - * Storage keys are 7 bit, lowest bit is always returned as zero > - * by iske. > - * This loop will set all 7 bits which means we set fetch > - * protection as well as reference and change indication for > - * some keys. > - */ > - key_to_set = i * 2; ah I see, you have simply moved this code :) > - set_storage_key(pagebuf[i], key_to_set, 1); > - } > + skey_set_keys(pagebuf, NUM_PAGES); > > puts("Please migrate me, then press return\n"); > (void)getchar(); > > - for (i = 0; i < NUM_PAGES; i++) { > - actual_key.val = get_storage_key(pagebuf[i]); > - expected_key.val = i * 2; > - > - /* > - * The PoP neither gives a guarantee that the reference bit is > - * accurate nor that it won't be cleared by hardware. Hence we > - * don't rely on it and just clear the bits to avoid compare > - * errors. > - */ > - actual_key.str.rf = 0; > - expected_key.str.rf = 0; > - > - /* don't log anything when key matches to avoid spamming the log */ > - if (actual_key.val != expected_key.val) { > - key_mismatches++; > - report_fail("page %d expected_key=0x%x actual_key=0x%x", i, expected_key.val, actual_key.val); > - } > - } > - > - report(!key_mismatches, "skeys after migration match"); > + result = skey_verify_keys(pagebuf, NUM_PAGES); > + skey_report_verify(&result); > } > > int main(void)
Quoting Claudio Imbrenda (2022-12-01 14:16:50) > On Thu, 1 Dec 2022 09:46:40 +0100 > Nico Boehr <nrb@linux.ibm.com> wrote: [...] > > diff --git a/lib/s390x/skey.c b/lib/s390x/skey.c > > new file mode 100644 > > index 000000000000..100f0949a244 [...] > > +/* > > + * Set storage keys on pagebuf. > > surely you should explain better what the function does (e.g. how are > you setting the keys and why) Well there is the comment below which explains why the * 2 is needed, so what about this paragraph (after merging the commits as discussed before): * Each page's storage key is generated by taking the page's index in pagebuf, * XOR-ing that with the given seed and then multipling the result with two. (But really that's also easy to see from the code below, so I am not sure if this really adds value.) > > + * pagebuf must point to page_count consecutive pages. > > + */ > > +void skey_set_keys(uint8_t *pagebuf, unsigned long page_count) > > this name does not make clear what the function is doing. at first one > would think that it sets the same key for all pages. > > maybe something like set_storage_keys_test_pattern or > skey_set_test_pattern or something like that Oh that's a nice suggestion, thanks. > > > +{ > > + unsigned char key_to_set; > > + unsigned long i; > > + > > + for (i = 0; i < page_count; i++) { > > + /* > > + * Storage keys are 7 bit, lowest bit is always returned as zero > > + * by iske. > > + * This loop will set all 7 bits which means we set fetch > > + * protection as well as reference and change indication for > > + * some keys. > > + */ > > + key_to_set = i * 2; > > + set_storage_key(pagebuf + i * PAGE_SIZE, key_to_set, 1); > > why not just i * 2 instead of using key_to_set ? Well you answered that yourself :) In patch 2, the key_to_set expression becomes a bit more complex, so the extra variable makes sense to me.
On Thu, 01 Dec 2022 16:55:42 +0100 Nico Boehr <nrb@linux.ibm.com> wrote: > Quoting Claudio Imbrenda (2022-12-01 14:16:50) > > On Thu, 1 Dec 2022 09:46:40 +0100 > > Nico Boehr <nrb@linux.ibm.com> wrote: > [...] > > > diff --git a/lib/s390x/skey.c b/lib/s390x/skey.c > > > new file mode 100644 > > > index 000000000000..100f0949a244 > [...] > > > +/* > > > + * Set storage keys on pagebuf. ... according to a pattern > > > > surely you should explain better what the function does (e.g. how are > > you setting the keys and why) > > Well there is the comment below which explains why the * 2 is needed, so what > about this paragraph (after merging the commits as discussed before): > > * Each page's storage key is generated by taking the page's index in pagebuf, > * XOR-ing that with the given seed and then multipling the result with two. looks good > > (But really that's also easy to see from the code below, so I am not sure if > this really adds value.) if you want to add documentation, do it properly, otherwise there is no point in having documentation at all :) > > > > + * pagebuf must point to page_count consecutive pages. > > > + */ > > > +void skey_set_keys(uint8_t *pagebuf, unsigned long page_count) > > > > this name does not make clear what the function is doing. at first one > > would think that it sets the same key for all pages. > > > > maybe something like set_storage_keys_test_pattern or > > skey_set_test_pattern or something like that > > Oh that's a nice suggestion, thanks. > > > > > > +{ > > > + unsigned char key_to_set; > > > + unsigned long i; > > > + > > > + for (i = 0; i < page_count; i++) { > > > + /* > > > + * Storage keys are 7 bit, lowest bit is always returned as zero > > > + * by iske. > > > + * This loop will set all 7 bits which means we set fetch > > > + * protection as well as reference and change indication for > > > + * some keys. > > > + */ > > > + key_to_set = i * 2; > > > + set_storage_key(pagebuf + i * PAGE_SIZE, key_to_set, 1); > > > > why not just i * 2 instead of using key_to_set ? > > Well you answered that yourself :) > > In patch 2, the key_to_set expression becomes a bit more complex, so the extra > variable makes sense to me. fair enough
On 12/1/22 09:46, Nico Boehr wrote: > Upcoming changes will add a test which is very similar to the existing > skey migration test. To reduce code duplication, move the common > functions to a library which can be re-used by both tests. > NACK We're not putting test specific code into the library. > Signed-off-by: Nico Boehr <nrb@linux.ibm.com> > --- > lib/s390x/skey.c | 92 ++++++++++++++++++++++++++++++++++++++++++ > lib/s390x/skey.h | 32 +++++++++++++++ > s390x/Makefile | 1 + > s390x/migration-skey.c | 44 +++----------------- > 4 files changed, 131 insertions(+), 38 deletions(-) > create mode 100644 lib/s390x/skey.c > create mode 100644 lib/s390x/skey.h > > diff --git a/lib/s390x/skey.c b/lib/s390x/skey.c > new file mode 100644 > index 000000000000..100f0949a244 > --- /dev/null > +++ b/lib/s390x/skey.c > @@ -0,0 +1,92 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Storage key migration test library > + * > + * Copyright IBM Corp. 2022 > + * > + * Authors: > + * Nico Boehr <nrb@linux.ibm.com> > + */ > + > +#include <libcflat.h> > +#include <asm/facility.h> > +#include <asm/mem.h> > +#include <skey.h> > + > +/* > + * Set storage keys on pagebuf. > + * pagebuf must point to page_count consecutive pages. > + */ > +void skey_set_keys(uint8_t *pagebuf, unsigned long page_count) > +{ > + unsigned char key_to_set; > + unsigned long i; > + > + for (i = 0; i < page_count; i++) { > + /* > + * Storage keys are 7 bit, lowest bit is always returned as zero > + * by iske. > + * This loop will set all 7 bits which means we set fetch > + * protection as well as reference and change indication for > + * some keys. > + */ > + key_to_set = i * 2; > + set_storage_key(pagebuf + i * PAGE_SIZE, key_to_set, 1); > + } > +} > + > +/* > + * Verify storage keys on pagebuf. > + * Storage keys must have been set by skey_set_keys on pagebuf before. > + * > + * If storage keys match the expected result, will return a skey_verify_result > + * with verify_failed false. All other fields are then invalid. > + * If there is a mismatch, returned struct will have verify_failed true and will > + * be filled with the details on the first mismatch encountered. > + */ > +struct skey_verify_result skey_verify_keys(uint8_t *pagebuf, unsigned long page_count) > +{ > + union skey expected_key, actual_key; > + struct skey_verify_result result = { > + .verify_failed = true > + }; > + uint8_t *cur_page; > + unsigned long i; > + > + for (i = 0; i < page_count; i++) { > + cur_page = pagebuf + i * PAGE_SIZE; > + actual_key.val = get_storage_key(cur_page); > + expected_key.val = i * 2; > + > + /* > + * The PoP neither gives a guarantee that the reference bit is > + * accurate nor that it won't be cleared by hardware. Hence we > + * don't rely on it and just clear the bits to avoid compare > + * errors. > + */ > + actual_key.str.rf = 0; > + expected_key.str.rf = 0; > + > + if (actual_key.val != expected_key.val) { > + result.expected_key.val = expected_key.val; > + result.actual_key.val = actual_key.val; > + result.page_mismatch_idx = i; > + result.page_mismatch_addr = (unsigned long)cur_page; > + return result; > + } > + } > + > + result.verify_failed = false; > + return result; > +} > + > +void skey_report_verify(struct skey_verify_result * const result) > +{ > + if (result->verify_failed) > + report_fail("page skey mismatch: first page idx = %lu, addr = 0x%lx, " > + "expected_key = 0x%x, actual_key = 0x%x", > + result->page_mismatch_idx, result->page_mismatch_addr, > + result->expected_key.val, result->actual_key.val); > + else > + report_pass("skeys match"); > +} > diff --git a/lib/s390x/skey.h b/lib/s390x/skey.h > new file mode 100644 > index 000000000000..a0f8caa1270b > --- /dev/null > +++ b/lib/s390x/skey.h > @@ -0,0 +1,32 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Storage key migration test library > + * > + * Copyright IBM Corp. 2022 > + * > + * Authors: > + * Nico Boehr <nrb@linux.ibm.com> > + */ > +#ifndef S390X_SKEY_H > +#define S390X_SKEY_H > + > +#include <libcflat.h> > +#include <asm/facility.h> > +#include <asm/page.h> > +#include <asm/mem.h> > + > +struct skey_verify_result { > + bool verify_failed; > + union skey expected_key; > + union skey actual_key; > + unsigned long page_mismatch_idx; > + unsigned long page_mismatch_addr; > +}; > + > +void skey_set_keys(uint8_t *pagebuf, unsigned long page_count); > + > +struct skey_verify_result skey_verify_keys(uint8_t *pagebuf, unsigned long page_count); > + > +void skey_report_verify(struct skey_verify_result * const result); > + > +#endif /* S390X_SKEY_H */ > diff --git a/s390x/Makefile b/s390x/Makefile > index bf1504f9d58c..d097b7071dfb 100644 > --- a/s390x/Makefile > +++ b/s390x/Makefile > @@ -99,6 +99,7 @@ cflatobjs += lib/s390x/malloc_io.o > cflatobjs += lib/s390x/uv.o > cflatobjs += lib/s390x/sie.o > cflatobjs += lib/s390x/fault.o > +cflatobjs += lib/s390x/skey.o > > OBJDIRS += lib/s390x > > diff --git a/s390x/migration-skey.c b/s390x/migration-skey.c > index b7bd82581abe..fed6fc1ed0f8 100644 > --- a/s390x/migration-skey.c > +++ b/s390x/migration-skey.c > @@ -10,55 +10,23 @@ > > #include <libcflat.h> > #include <asm/facility.h> > -#include <asm/page.h> > -#include <asm/mem.h> > -#include <asm/interrupt.h> > #include <hardware.h> > +#include <skey.h> > > #define NUM_PAGES 128 > -static uint8_t pagebuf[NUM_PAGES][PAGE_SIZE] __attribute__((aligned(PAGE_SIZE))); > +static uint8_t pagebuf[NUM_PAGES * PAGE_SIZE] __attribute__((aligned(PAGE_SIZE))); > > static void test_migration(void) > { > - union skey expected_key, actual_key; > - int i, key_to_set, key_mismatches = 0; > + struct skey_verify_result result; > > - for (i = 0; i < NUM_PAGES; i++) { > - /* > - * Storage keys are 7 bit, lowest bit is always returned as zero > - * by iske. > - * This loop will set all 7 bits which means we set fetch > - * protection as well as reference and change indication for > - * some keys. > - */ > - key_to_set = i * 2; > - set_storage_key(pagebuf[i], key_to_set, 1); > - } > + skey_set_keys(pagebuf, NUM_PAGES); > > puts("Please migrate me, then press return\n"); > (void)getchar(); > > - for (i = 0; i < NUM_PAGES; i++) { > - actual_key.val = get_storage_key(pagebuf[i]); > - expected_key.val = i * 2; > - > - /* > - * The PoP neither gives a guarantee that the reference bit is > - * accurate nor that it won't be cleared by hardware. Hence we > - * don't rely on it and just clear the bits to avoid compare > - * errors. > - */ > - actual_key.str.rf = 0; > - expected_key.str.rf = 0; > - > - /* don't log anything when key matches to avoid spamming the log */ > - if (actual_key.val != expected_key.val) { > - key_mismatches++; > - report_fail("page %d expected_key=0x%x actual_key=0x%x", i, expected_key.val, actual_key.val); > - } > - } > - > - report(!key_mismatches, "skeys after migration match"); > + result = skey_verify_keys(pagebuf, NUM_PAGES); > + skey_report_verify(&result); > } > > int main(void)
On 02/12/2022 10.03, Janosch Frank wrote: > On 12/1/22 09:46, Nico Boehr wrote: >> Upcoming changes will add a test which is very similar to the existing >> skey migration test. To reduce code duplication, move the common >> functions to a library which can be re-used by both tests. >> > > NACK > > We're not putting test specific code into the library. Do we need a new file (in the third patch) for the new test at all, or could the new test simply be added to s390x/migration-skey.c instead? Thomas >> Signed-off-by: Nico Boehr <nrb@linux.ibm.com> >> --- >> lib/s390x/skey.c | 92 ++++++++++++++++++++++++++++++++++++++++++ >> lib/s390x/skey.h | 32 +++++++++++++++ >> s390x/Makefile | 1 + >> s390x/migration-skey.c | 44 +++----------------- >> 4 files changed, 131 insertions(+), 38 deletions(-) >> create mode 100644 lib/s390x/skey.c >> create mode 100644 lib/s390x/skey.h >> >> diff --git a/lib/s390x/skey.c b/lib/s390x/skey.c >> new file mode 100644 >> index 000000000000..100f0949a244 >> --- /dev/null >> +++ b/lib/s390x/skey.c >> @@ -0,0 +1,92 @@ >> +/* SPDX-License-Identifier: GPL-2.0-only */ >> +/* >> + * Storage key migration test library >> + * >> + * Copyright IBM Corp. 2022 >> + * >> + * Authors: >> + * Nico Boehr <nrb@linux.ibm.com> >> + */ >> + >> +#include <libcflat.h> >> +#include <asm/facility.h> >> +#include <asm/mem.h> >> +#include <skey.h> >> + >> +/* >> + * Set storage keys on pagebuf. >> + * pagebuf must point to page_count consecutive pages. >> + */ >> +void skey_set_keys(uint8_t *pagebuf, unsigned long page_count) >> +{ >> + unsigned char key_to_set; >> + unsigned long i; >> + >> + for (i = 0; i < page_count; i++) { >> + /* >> + * Storage keys are 7 bit, lowest bit is always returned as zero >> + * by iske. >> + * This loop will set all 7 bits which means we set fetch >> + * protection as well as reference and change indication for >> + * some keys. >> + */ >> + key_to_set = i * 2; >> + set_storage_key(pagebuf + i * PAGE_SIZE, key_to_set, 1); >> + } >> +} >> + >> +/* >> + * Verify storage keys on pagebuf. >> + * Storage keys must have been set by skey_set_keys on pagebuf before. >> + * >> + * If storage keys match the expected result, will return a >> skey_verify_result >> + * with verify_failed false. All other fields are then invalid. >> + * If there is a mismatch, returned struct will have verify_failed true >> and will >> + * be filled with the details on the first mismatch encountered. >> + */ >> +struct skey_verify_result skey_verify_keys(uint8_t *pagebuf, unsigned >> long page_count) >> +{ >> + union skey expected_key, actual_key; >> + struct skey_verify_result result = { >> + .verify_failed = true >> + }; >> + uint8_t *cur_page; >> + unsigned long i; >> + >> + for (i = 0; i < page_count; i++) { >> + cur_page = pagebuf + i * PAGE_SIZE; >> + actual_key.val = get_storage_key(cur_page); >> + expected_key.val = i * 2; >> + >> + /* >> + * The PoP neither gives a guarantee that the reference bit is >> + * accurate nor that it won't be cleared by hardware. Hence we >> + * don't rely on it and just clear the bits to avoid compare >> + * errors. >> + */ >> + actual_key.str.rf = 0; >> + expected_key.str.rf = 0; >> + >> + if (actual_key.val != expected_key.val) { >> + result.expected_key.val = expected_key.val; >> + result.actual_key.val = actual_key.val; >> + result.page_mismatch_idx = i; >> + result.page_mismatch_addr = (unsigned long)cur_page; >> + return result; >> + } >> + } >> + >> + result.verify_failed = false; >> + return result; >> +} >> + >> +void skey_report_verify(struct skey_verify_result * const result) >> +{ >> + if (result->verify_failed) >> + report_fail("page skey mismatch: first page idx = %lu, addr = >> 0x%lx, " >> + "expected_key = 0x%x, actual_key = 0x%x", >> + result->page_mismatch_idx, result->page_mismatch_addr, >> + result->expected_key.val, result->actual_key.val); >> + else >> + report_pass("skeys match"); >> +} >> diff --git a/lib/s390x/skey.h b/lib/s390x/skey.h >> new file mode 100644 >> index 000000000000..a0f8caa1270b >> --- /dev/null >> +++ b/lib/s390x/skey.h >> @@ -0,0 +1,32 @@ >> +/* SPDX-License-Identifier: GPL-2.0-only */ >> +/* >> + * Storage key migration test library >> + * >> + * Copyright IBM Corp. 2022 >> + * >> + * Authors: >> + * Nico Boehr <nrb@linux.ibm.com> >> + */ >> +#ifndef S390X_SKEY_H >> +#define S390X_SKEY_H >> + >> +#include <libcflat.h> >> +#include <asm/facility.h> >> +#include <asm/page.h> >> +#include <asm/mem.h> >> + >> +struct skey_verify_result { >> + bool verify_failed; >> + union skey expected_key; >> + union skey actual_key; >> + unsigned long page_mismatch_idx; >> + unsigned long page_mismatch_addr; >> +}; >> + >> +void skey_set_keys(uint8_t *pagebuf, unsigned long page_count); >> + >> +struct skey_verify_result skey_verify_keys(uint8_t *pagebuf, unsigned >> long page_count); >> + >> +void skey_report_verify(struct skey_verify_result * const result); >> + >> +#endif /* S390X_SKEY_H */ >> diff --git a/s390x/Makefile b/s390x/Makefile >> index bf1504f9d58c..d097b7071dfb 100644 >> --- a/s390x/Makefile >> +++ b/s390x/Makefile >> @@ -99,6 +99,7 @@ cflatobjs += lib/s390x/malloc_io.o >> cflatobjs += lib/s390x/uv.o >> cflatobjs += lib/s390x/sie.o >> cflatobjs += lib/s390x/fault.o >> +cflatobjs += lib/s390x/skey.o >> OBJDIRS += lib/s390x >> diff --git a/s390x/migration-skey.c b/s390x/migration-skey.c >> index b7bd82581abe..fed6fc1ed0f8 100644 >> --- a/s390x/migration-skey.c >> +++ b/s390x/migration-skey.c >> @@ -10,55 +10,23 @@ >> #include <libcflat.h> >> #include <asm/facility.h> >> -#include <asm/page.h> >> -#include <asm/mem.h> >> -#include <asm/interrupt.h> >> #include <hardware.h> >> +#include <skey.h> >> #define NUM_PAGES 128 >> -static uint8_t pagebuf[NUM_PAGES][PAGE_SIZE] >> __attribute__((aligned(PAGE_SIZE))); >> +static uint8_t pagebuf[NUM_PAGES * PAGE_SIZE] >> __attribute__((aligned(PAGE_SIZE))); >> static void test_migration(void) >> { >> - union skey expected_key, actual_key; >> - int i, key_to_set, key_mismatches = 0; >> + struct skey_verify_result result; >> - for (i = 0; i < NUM_PAGES; i++) { >> - /* >> - * Storage keys are 7 bit, lowest bit is always returned as zero >> - * by iske. >> - * This loop will set all 7 bits which means we set fetch >> - * protection as well as reference and change indication for >> - * some keys. >> - */ >> - key_to_set = i * 2; >> - set_storage_key(pagebuf[i], key_to_set, 1); >> - } >> + skey_set_keys(pagebuf, NUM_PAGES); >> puts("Please migrate me, then press return\n"); >> (void)getchar(); >> - for (i = 0; i < NUM_PAGES; i++) { >> - actual_key.val = get_storage_key(pagebuf[i]); >> - expected_key.val = i * 2; >> - >> - /* >> - * The PoP neither gives a guarantee that the reference bit is >> - * accurate nor that it won't be cleared by hardware. Hence we >> - * don't rely on it and just clear the bits to avoid compare >> - * errors. >> - */ >> - actual_key.str.rf = 0; >> - expected_key.str.rf = 0; >> - >> - /* don't log anything when key matches to avoid spamming the log */ >> - if (actual_key.val != expected_key.val) { >> - key_mismatches++; >> - report_fail("page %d expected_key=0x%x actual_key=0x%x", i, >> expected_key.val, actual_key.val); >> - } >> - } >> - >> - report(!key_mismatches, "skeys after migration match"); >> + result = skey_verify_keys(pagebuf, NUM_PAGES); >> + skey_report_verify(&result); >> } >> int main(void) >
Quoting Janosch Frank (2022-12-02 10:03:22) > On 12/1/22 09:46, Nico Boehr wrote: > > Upcoming changes will add a test which is very similar to the existing > > skey migration test. To reduce code duplication, move the common > > functions to a library which can be re-used by both tests. > > > > NACK > > We're not putting test specific code into the library. What do you mean by "test specific"? After all, it is used by two tests now, possibly more in the future. Any alternative suggestions?
Quoting Thomas Huth (2022-12-02 10:09:03) > On 02/12/2022 10.03, Janosch Frank wrote: > > On 12/1/22 09:46, Nico Boehr wrote: > >> Upcoming changes will add a test which is very similar to the existing > >> skey migration test. To reduce code duplication, move the common > >> functions to a library which can be re-used by both tests. > >> > > > > NACK > > > > We're not putting test specific code into the library. > > Do we need a new file (in the third patch) for the new test at all, or could > the new test simply be added to s390x/migration-skey.c instead? Mh, not quite. One test wants to change storage keys *before* migrating, the other *while* migrating. Since we can only migrate once, it is not obvious to me how we could do that in one run. Speaking of one run, what we could do is add a command line argument which decides which test to run and then call the same test with different arguments in unittests.cfg. Other options I can think of (I don't like any of them): - copy the code (probably the worst solution) - header file in s390x which is included by both tests (better, but still bad, means double compilation of the test functions)
On 12/2/22 11:39, Nico Boehr wrote: > Quoting Janosch Frank (2022-12-02 10:03:22) >> On 12/1/22 09:46, Nico Boehr wrote: >>> Upcoming changes will add a test which is very similar to the existing >>> skey migration test. To reduce code duplication, move the common >>> functions to a library which can be re-used by both tests. >>> >> >> NACK >> >> We're not putting test specific code into the library. > > What do you mean by "test specific"? After all, it is used by two tests now, possibly more in the future. > > Any alternative suggestions? For me this is like putting kselftest macros/functions into the kernel. The KUT library is more or less the kernel on which the tests in s390x/ are based on. It provides primitives which (hopefully and mostly) aren't specific to tests. Yes: Providing skey set and get functions for one or multiple pages to tests. I.e. sske and iske wrappers. No: Providing multi-page skey set and verify functions that set and verify skeys based on a pattern which is __hardcoded__ into the function using the skey wrappers. I.e. you're trying to create a new layer (test functionality) and stuffing it into the unit test kernel library. What you want is a separate testlib which would reside in s390x/testlib/ where we can store often repeated functions and macros.
On Fri, 2 Dec 2022 12:20:34 +0100 Janosch Frank <frankja@linux.ibm.com> wrote: > On 12/2/22 11:39, Nico Boehr wrote: > > Quoting Janosch Frank (2022-12-02 10:03:22) > >> On 12/1/22 09:46, Nico Boehr wrote: > >>> Upcoming changes will add a test which is very similar to the existing > >>> skey migration test. To reduce code duplication, move the common > >>> functions to a library which can be re-used by both tests. > >>> > >> > >> NACK > >> > >> We're not putting test specific code into the library. > > > > What do you mean by "test specific"? After all, it is used by two tests now, possibly more in the future. > > > > Any alternative suggestions? > > For me this is like putting kselftest macros/functions into the kernel. > > The KUT library is more or less the kernel on which the tests in s390x/ > are based on. It provides primitives which (hopefully and mostly) aren't > specific to tests. > > Yes: > Providing skey set and get functions for one or multiple pages to tests. > I.e. sske and iske wrappers. > > No: > Providing multi-page skey set and verify functions that set and verify > skeys based on a pattern which is __hardcoded__ into the function using > the skey wrappers. I.e. you're trying to create a new layer (test > functionality) and stuffing it into the unit test kernel library. > > What you want is a separate testlib which would reside in s390x/testlib/ > where we can store often repeated functions and macros. oufff, then we need to also fix the cmma migration tests
On 02/12/2022 11.44, Nico Boehr wrote: > Quoting Thomas Huth (2022-12-02 10:09:03) >> On 02/12/2022 10.03, Janosch Frank wrote: >>> On 12/1/22 09:46, Nico Boehr wrote: >>>> Upcoming changes will add a test which is very similar to the existing >>>> skey migration test. To reduce code duplication, move the common >>>> functions to a library which can be re-used by both tests. >>>> >>> >>> NACK >>> >>> We're not putting test specific code into the library. >> >> Do we need a new file (in the third patch) for the new test at all, or could >> the new test simply be added to s390x/migration-skey.c instead? > > Mh, not quite. One test wants to change storage keys *before* migrating, the other *while* migrating. Since we can only migrate once, it is not obvious to me how we could do that in one run. > > Speaking of one run, what we could do is add a command line argument which decides which test to run and then call the same test with different arguments in unittests.cfg. Yes, that's what I had in mind - use a command line argument to select the test ... should be OK as long as both variants are listed in unittests.cfg, shouldn't it? Thomas
On 12/2/22 12:32, Thomas Huth wrote: > On 02/12/2022 11.44, Nico Boehr wrote: >> Quoting Thomas Huth (2022-12-02 10:09:03) >>> On 02/12/2022 10.03, Janosch Frank wrote: >>>> On 12/1/22 09:46, Nico Boehr wrote: >>>>> Upcoming changes will add a test which is very similar to the existing >>>>> skey migration test. To reduce code duplication, move the common >>>>> functions to a library which can be re-used by both tests. >>>>> >>>> >>>> NACK >>>> >>>> We're not putting test specific code into the library. >>> >>> Do we need a new file (in the third patch) for the new test at all, or could >>> the new test simply be added to s390x/migration-skey.c instead? >> >> Mh, not quite. One test wants to change storage keys *before* migrating, the other *while* migrating. Since we can only migrate once, it is not obvious to me how we could do that in one run. >> >> Speaking of one run, what we could do is add a command line argument which decides which test to run and then call the same test with different arguments in unittests.cfg. > > Yes, that's what I had in mind - use a command line argument to select the > test ... should be OK as long as both variants are listed in unittests.cfg, > shouldn't it? > > Thomas > @Thomas @Claudio: I see two possible solutions if we want a "testlib" at some point (which for the record I don't have anything against): Putting the files into lib/s390x/testlib/* which will then be part of our normal lib. That's a minimal effort solution. It still puts those files into lib/* but they are at least contained in a directory. Putting the files into s390x/testlib/* and creating a proper new lib. Which means we'd need a few more lines of makefile changes. None of that is a huge amount of work.
On 02/12/2022 12.56, Janosch Frank wrote: > On 12/2/22 12:32, Thomas Huth wrote: >> On 02/12/2022 11.44, Nico Boehr wrote: >>> Quoting Thomas Huth (2022-12-02 10:09:03) >>>> On 02/12/2022 10.03, Janosch Frank wrote: >>>>> On 12/1/22 09:46, Nico Boehr wrote: >>>>>> Upcoming changes will add a test which is very similar to the existing >>>>>> skey migration test. To reduce code duplication, move the common >>>>>> functions to a library which can be re-used by both tests. >>>>>> >>>>> >>>>> NACK >>>>> >>>>> We're not putting test specific code into the library. >>>> >>>> Do we need a new file (in the third patch) for the new test at all, or >>>> could >>>> the new test simply be added to s390x/migration-skey.c instead? >>> >>> Mh, not quite. One test wants to change storage keys *before* migrating, >>> the other *while* migrating. Since we can only migrate once, it is not >>> obvious to me how we could do that in one run. >>> >>> Speaking of one run, what we could do is add a command line argument >>> which decides which test to run and then call the same test with >>> different arguments in unittests.cfg. >> >> Yes, that's what I had in mind - use a command line argument to select the >> test ... should be OK as long as both variants are listed in unittests.cfg, >> shouldn't it? >> >> Thomas > > @Thomas @Claudio: > I see two possible solutions if we want a "testlib" at some point (which for > the record I don't have anything against): > > Putting the files into lib/s390x/testlib/* which will then be part of our > normal lib. > That's a minimal effort solution. It still puts those files into lib/* but > they are at least contained in a directory. > > Putting the files into s390x/testlib/* and creating a proper new lib. > Which means we'd need a few more lines of makefile changes. Though this is an excellent topic for a Friday afternoon bikeshedding ... I don't mind much either way. I maybe just got a small preference to not touch the main lib/ folder here. I guess you could even call it s390x/migration-skey-common.c and leave the lib logic out of the game ... but I don't really mind. Up to you to decide ;-) Thomas
On Fri, 2 Dec 2022 13:48:17 +0100 Thomas Huth <thuth@redhat.com> wrote: > On 02/12/2022 12.56, Janosch Frank wrote: > > On 12/2/22 12:32, Thomas Huth wrote: > >> On 02/12/2022 11.44, Nico Boehr wrote: > >>> Quoting Thomas Huth (2022-12-02 10:09:03) > >>>> On 02/12/2022 10.03, Janosch Frank wrote: > >>>>> On 12/1/22 09:46, Nico Boehr wrote: > >>>>>> Upcoming changes will add a test which is very similar to the existing > >>>>>> skey migration test. To reduce code duplication, move the common > >>>>>> functions to a library which can be re-used by both tests. > >>>>>> > >>>>> > >>>>> NACK > >>>>> > >>>>> We're not putting test specific code into the library. > >>>> > >>>> Do we need a new file (in the third patch) for the new test at all, or > >>>> could > >>>> the new test simply be added to s390x/migration-skey.c instead? > >>> > >>> Mh, not quite. One test wants to change storage keys *before* migrating, > >>> the other *while* migrating. Since we can only migrate once, it is not > >>> obvious to me how we could do that in one run. > >>> > >>> Speaking of one run, what we could do is add a command line argument > >>> which decides which test to run and then call the same test with > >>> different arguments in unittests.cfg. > >> > >> Yes, that's what I had in mind - use a command line argument to select the > >> test ... should be OK as long as both variants are listed in unittests.cfg, > >> shouldn't it? > >> > >> Thomas > > > > @Thomas @Claudio: > > I see two possible solutions if we want a "testlib" at some point (which for > > the record I don't have anything against): > > > > Putting the files into lib/s390x/testlib/* which will then be part of our > > normal lib. > > That's a minimal effort solution. It still puts those files into lib/* but > > they are at least contained in a directory. > > > > Putting the files into s390x/testlib/* and creating a proper new lib. > > Which means we'd need a few more lines of makefile changes. > > Though this is an excellent topic for a Friday afternoon bikeshedding ... I > don't mind much either way. I maybe just got a small preference to not touch > the main lib/ folder here. I guess you could even call it > s390x/migration-skey-common.c and leave the lib logic out of the game ... > but I don't really mind. Up to you to decide ;-) > I really like the idea of having only one test and use a commandline parameter to decide which variant to run this way no need to put things in external files > Thomas >
Quoting Claudio Imbrenda (2022-12-02 13:56:47) > On Fri, 2 Dec 2022 13:48:17 +0100 > Thomas Huth <thuth@redhat.com> wrote: > > > On 02/12/2022 12.56, Janosch Frank wrote: > > > On 12/2/22 12:32, Thomas Huth wrote: > > >> On 02/12/2022 11.44, Nico Boehr wrote: > > >>> Quoting Thomas Huth (2022-12-02 10:09:03) > > >>>> On 02/12/2022 10.03, Janosch Frank wrote: > > >>>>> On 12/1/22 09:46, Nico Boehr wrote: > > >>>>>> Upcoming changes will add a test which is very similar to the existing > > >>>>>> skey migration test. To reduce code duplication, move the common > > >>>>>> functions to a library which can be re-used by both tests. > > >>>>>> > > >>>>> > > >>>>> NACK > > >>>>> > > >>>>> We're not putting test specific code into the library. > > >>>> > > >>>> Do we need a new file (in the third patch) for the new test at all, or > > >>>> could > > >>>> the new test simply be added to s390x/migration-skey.c instead? > > >>> > > >>> Mh, not quite. One test wants to change storage keys *before* migrating, > > >>> the other *while* migrating. Since we can only migrate once, it is not > > >>> obvious to me how we could do that in one run. > > >>> > > >>> Speaking of one run, what we could do is add a command line argument > > >>> which decides which test to run and then call the same test with > > >>> different arguments in unittests.cfg. > > >> > > >> Yes, that's what I had in mind - use a command line argument to select the > > >> test ... should be OK as long as both variants are listed in unittests.cfg, > > >> shouldn't it? > > >> > > >> Thomas > > > > > > @Thomas @Claudio: > > > I see two possible solutions if we want a "testlib" at some point (which for > > > the record I don't have anything against): > > > > > > Putting the files into lib/s390x/testlib/* which will then be part of our > > > normal lib. > > > That's a minimal effort solution. It still puts those files into lib/* but > > > they are at least contained in a directory. > > > > > > Putting the files into s390x/testlib/* and creating a proper new lib. > > > Which means we'd need a few more lines of makefile changes. > > > > Though this is an excellent topic for a Friday afternoon bikeshedding ... I > > don't mind much either way. I maybe just got a small preference to not touch > > the main lib/ folder here. I guess you could even call it > > s390x/migration-skey-common.c and leave the lib logic out of the game ... > > but I don't really mind. Up to you to decide ;-) > > > > I really like the idea of having only one test and use a commandline > parameter to decide which variant to run > > this way no need to put things in external files OK, I also like this suggestion, then I'll refactor the tests.
diff --git a/lib/s390x/skey.c b/lib/s390x/skey.c new file mode 100644 index 000000000000..100f0949a244 --- /dev/null +++ b/lib/s390x/skey.c @@ -0,0 +1,92 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Storage key migration test library + * + * Copyright IBM Corp. 2022 + * + * Authors: + * Nico Boehr <nrb@linux.ibm.com> + */ + +#include <libcflat.h> +#include <asm/facility.h> +#include <asm/mem.h> +#include <skey.h> + +/* + * Set storage keys on pagebuf. + * pagebuf must point to page_count consecutive pages. + */ +void skey_set_keys(uint8_t *pagebuf, unsigned long page_count) +{ + unsigned char key_to_set; + unsigned long i; + + for (i = 0; i < page_count; i++) { + /* + * Storage keys are 7 bit, lowest bit is always returned as zero + * by iske. + * This loop will set all 7 bits which means we set fetch + * protection as well as reference and change indication for + * some keys. + */ + key_to_set = i * 2; + set_storage_key(pagebuf + i * PAGE_SIZE, key_to_set, 1); + } +} + +/* + * Verify storage keys on pagebuf. + * Storage keys must have been set by skey_set_keys on pagebuf before. + * + * If storage keys match the expected result, will return a skey_verify_result + * with verify_failed false. All other fields are then invalid. + * If there is a mismatch, returned struct will have verify_failed true and will + * be filled with the details on the first mismatch encountered. + */ +struct skey_verify_result skey_verify_keys(uint8_t *pagebuf, unsigned long page_count) +{ + union skey expected_key, actual_key; + struct skey_verify_result result = { + .verify_failed = true + }; + uint8_t *cur_page; + unsigned long i; + + for (i = 0; i < page_count; i++) { + cur_page = pagebuf + i * PAGE_SIZE; + actual_key.val = get_storage_key(cur_page); + expected_key.val = i * 2; + + /* + * The PoP neither gives a guarantee that the reference bit is + * accurate nor that it won't be cleared by hardware. Hence we + * don't rely on it and just clear the bits to avoid compare + * errors. + */ + actual_key.str.rf = 0; + expected_key.str.rf = 0; + + if (actual_key.val != expected_key.val) { + result.expected_key.val = expected_key.val; + result.actual_key.val = actual_key.val; + result.page_mismatch_idx = i; + result.page_mismatch_addr = (unsigned long)cur_page; + return result; + } + } + + result.verify_failed = false; + return result; +} + +void skey_report_verify(struct skey_verify_result * const result) +{ + if (result->verify_failed) + report_fail("page skey mismatch: first page idx = %lu, addr = 0x%lx, " + "expected_key = 0x%x, actual_key = 0x%x", + result->page_mismatch_idx, result->page_mismatch_addr, + result->expected_key.val, result->actual_key.val); + else + report_pass("skeys match"); +} diff --git a/lib/s390x/skey.h b/lib/s390x/skey.h new file mode 100644 index 000000000000..a0f8caa1270b --- /dev/null +++ b/lib/s390x/skey.h @@ -0,0 +1,32 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Storage key migration test library + * + * Copyright IBM Corp. 2022 + * + * Authors: + * Nico Boehr <nrb@linux.ibm.com> + */ +#ifndef S390X_SKEY_H +#define S390X_SKEY_H + +#include <libcflat.h> +#include <asm/facility.h> +#include <asm/page.h> +#include <asm/mem.h> + +struct skey_verify_result { + bool verify_failed; + union skey expected_key; + union skey actual_key; + unsigned long page_mismatch_idx; + unsigned long page_mismatch_addr; +}; + +void skey_set_keys(uint8_t *pagebuf, unsigned long page_count); + +struct skey_verify_result skey_verify_keys(uint8_t *pagebuf, unsigned long page_count); + +void skey_report_verify(struct skey_verify_result * const result); + +#endif /* S390X_SKEY_H */ diff --git a/s390x/Makefile b/s390x/Makefile index bf1504f9d58c..d097b7071dfb 100644 --- a/s390x/Makefile +++ b/s390x/Makefile @@ -99,6 +99,7 @@ cflatobjs += lib/s390x/malloc_io.o cflatobjs += lib/s390x/uv.o cflatobjs += lib/s390x/sie.o cflatobjs += lib/s390x/fault.o +cflatobjs += lib/s390x/skey.o OBJDIRS += lib/s390x diff --git a/s390x/migration-skey.c b/s390x/migration-skey.c index b7bd82581abe..fed6fc1ed0f8 100644 --- a/s390x/migration-skey.c +++ b/s390x/migration-skey.c @@ -10,55 +10,23 @@ #include <libcflat.h> #include <asm/facility.h> -#include <asm/page.h> -#include <asm/mem.h> -#include <asm/interrupt.h> #include <hardware.h> +#include <skey.h> #define NUM_PAGES 128 -static uint8_t pagebuf[NUM_PAGES][PAGE_SIZE] __attribute__((aligned(PAGE_SIZE))); +static uint8_t pagebuf[NUM_PAGES * PAGE_SIZE] __attribute__((aligned(PAGE_SIZE))); static void test_migration(void) { - union skey expected_key, actual_key; - int i, key_to_set, key_mismatches = 0; + struct skey_verify_result result; - for (i = 0; i < NUM_PAGES; i++) { - /* - * Storage keys are 7 bit, lowest bit is always returned as zero - * by iske. - * This loop will set all 7 bits which means we set fetch - * protection as well as reference and change indication for - * some keys. - */ - key_to_set = i * 2; - set_storage_key(pagebuf[i], key_to_set, 1); - } + skey_set_keys(pagebuf, NUM_PAGES); puts("Please migrate me, then press return\n"); (void)getchar(); - for (i = 0; i < NUM_PAGES; i++) { - actual_key.val = get_storage_key(pagebuf[i]); - expected_key.val = i * 2; - - /* - * The PoP neither gives a guarantee that the reference bit is - * accurate nor that it won't be cleared by hardware. Hence we - * don't rely on it and just clear the bits to avoid compare - * errors. - */ - actual_key.str.rf = 0; - expected_key.str.rf = 0; - - /* don't log anything when key matches to avoid spamming the log */ - if (actual_key.val != expected_key.val) { - key_mismatches++; - report_fail("page %d expected_key=0x%x actual_key=0x%x", i, expected_key.val, actual_key.val); - } - } - - report(!key_mismatches, "skeys after migration match"); + result = skey_verify_keys(pagebuf, NUM_PAGES); + skey_report_verify(&result); } int main(void)
Upcoming changes will add a test which is very similar to the existing skey migration test. To reduce code duplication, move the common functions to a library which can be re-used by both tests. Signed-off-by: Nico Boehr <nrb@linux.ibm.com> --- lib/s390x/skey.c | 92 ++++++++++++++++++++++++++++++++++++++++++ lib/s390x/skey.h | 32 +++++++++++++++ s390x/Makefile | 1 + s390x/migration-skey.c | 44 +++----------------- 4 files changed, 131 insertions(+), 38 deletions(-) create mode 100644 lib/s390x/skey.c create mode 100644 lib/s390x/skey.h