| Message ID | 20231213124942.604109-5-nsg@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x: STFLE nested interpretation | expand |
On Wed, 13 Dec 2023 13:49:41 +0100 Nina Schoetterl-Glausch <nsg@linux.ibm.com> wrote: > Replace the existing code for exiting from snippets with the newly > introduced library functionality. > This causes additional report()s, otherwise no change in functionality > intended. > > Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com> > --- > s390x/sie-dat.c | 10 ++-------- > s390x/snippets/c/sie-dat.c | 19 +------------------ > 2 files changed, 3 insertions(+), 26 deletions(-) > > diff --git a/s390x/sie-dat.c b/s390x/sie-dat.c > index 9e60f26e..6b6e6868 100644 > --- a/s390x/sie-dat.c > +++ b/s390x/sie-dat.c > @@ -27,23 +27,17 @@ static void test_sie_dat(void) > uint64_t test_page_gpa, test_page_hpa; > uint8_t *test_page_hva, expected_val; > bool contents_match; > - uint8_t r1; > > /* guest will tell us the guest physical address of the test buffer */ > sie(&vm); > - assert(vm.sblk->icptcode == ICPT_INST && > - (vm.sblk->ipa & 0xff00) == 0x8300 && vm.sblk->ipb == 0x9c0000); > - > - r1 = (vm.sblk->ipa & 0xf0) >> 4; > - test_page_gpa = vm.save_area.guest.grs[r1]; > + assert(snippet_get_force_exit_value(&vm, &test_page_gpa)); but the function inside the assert will already report a failure, right? then why the assert? (the point I'm trying to make is that the function should not report stuff, see my comments in the previous patch) > test_page_hpa = virt_to_pte_phys(guest_root, (void*)test_page_gpa); > test_page_hva = __va(test_page_hpa); > report_info("test buffer gpa=0x%lx hva=%p", test_page_gpa, test_page_hva); > > /* guest will now write to the test buffer and we verify the contents */ > sie(&vm); > - assert(vm.sblk->icptcode == ICPT_INST && > - vm.sblk->ipa == 0x8300 && vm.sblk->ipb == 0x440000); > + assert(snippet_check_force_exit(&vm)); > > contents_match = true; > for (unsigned int i = 0; i < GUEST_TEST_PAGE_COUNT; i++) { > diff --git a/s390x/snippets/c/sie-dat.c b/s390x/snippets/c/sie-dat.c > index ecfcb60e..414afd42 100644 > --- a/s390x/snippets/c/sie-dat.c > +++ b/s390x/snippets/c/sie-dat.c > @@ -9,28 +9,11 @@ > */ > #include <libcflat.h> > #include <asm-generic/page.h> > +#include <snippet-guest.h> > #include "sie-dat.h" > > static uint8_t test_pages[GUEST_TEST_PAGE_COUNT * PAGE_SIZE] __attribute__((__aligned__(PAGE_SIZE))); > > -static inline void force_exit(void) > -{ > - asm volatile("diag 0,0,0x44\n" > - : > - : > - : "memory" > - ); > -} > - > -static inline void force_exit_value(uint64_t val) > -{ > - asm volatile("diag %[val],0,0x9c\n" > - : > - : [val] "d"(val) > - : "memory" > - ); > -} > - > int main(void) > { > uint8_t *invalid_ptr;
On Wed, 2023-12-13 at 17:45 +0100, Claudio Imbrenda wrote: > On Wed, 13 Dec 2023 13:49:41 +0100 > Nina Schoetterl-Glausch <nsg@linux.ibm.com> wrote: > > > Replace the existing code for exiting from snippets with the newly > > introduced library functionality. > > This causes additional report()s, otherwise no change in functionality > > intended. > > > > Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com> > > --- > > s390x/sie-dat.c | 10 ++-------- > > s390x/snippets/c/sie-dat.c | 19 +------------------ > > 2 files changed, 3 insertions(+), 26 deletions(-) > > > > diff --git a/s390x/sie-dat.c b/s390x/sie-dat.c > > index 9e60f26e..6b6e6868 100644 > > --- a/s390x/sie-dat.c > > +++ b/s390x/sie-dat.c > > @@ -27,23 +27,17 @@ static void test_sie_dat(void) > > uint64_t test_page_gpa, test_page_hpa; > > uint8_t *test_page_hva, expected_val; > > bool contents_match; > > - uint8_t r1; > > > > /* guest will tell us the guest physical address of the test buffer */ > > sie(&vm); > > - assert(vm.sblk->icptcode == ICPT_INST && > > - (vm.sblk->ipa & 0xff00) == 0x8300 && vm.sblk->ipb == 0x9c0000); > > - > > - r1 = (vm.sblk->ipa & 0xf0) >> 4; > > - test_page_gpa = vm.save_area.guest.grs[r1]; > > + assert(snippet_get_force_exit_value(&vm, &test_page_gpa)); > > but the function inside the assert will already report a failure, right? > then why the assert? (the point I'm trying to make is that the function The assert makes sense, if it fails something has gone completely off the rails. The question is indeed rather if to report. There is no harm in it, but I'm thinking now that there should be _get_ functions that don't report and optionally also _check_ functions that do. So: snippet_get_force_exit snippet_check_force_exit (exists, but only the _get_ variant is currently required) snippet_get_force_exit_value (exists, required) snippet_check_force_exit_value (exists, but unused) So minimally, we could do: snippet_get_force_exit snippet_get_force_exit_value I'm thinking we should go for the following: bool snippet_has_force_exit(...) bool snippet_has_force_exit_value(...) uint64_t snippet_get_force_exit_value(...) (internally does assert(snippet_has_forced_exit_value)) void snippet_check_force_exit_value(...) (or whoever needs this in the future adds it) (Naming open to suggestions) Then this becomes: /* guest will tell us the guest physical address of the test buffer */ sie(&vm); - assert(vm.sblk->icptcode == ICPT_INST && - (vm.sblk->ipa & 0xff00) == 0x8300 && vm.sblk->ipb == 0x9c0000); - + assert(snippet_has_force_exit_value(&vm); - r1 = (vm.sblk->ipa & 0xf0) >> 4; - test_page_gpa = vm.save_area.guest.grs[r1]; + test_page_gpa = snippet_get_force_exit_value(&vm); > should not report stuff, see my comments in the previous patch) > > > test_page_hpa = virt_to_pte_phys(guest_root, (void*)test_page_gpa); > > test_page_hva = __va(test_page_hpa); > > report_info("test buffer gpa=0x%lx hva=%p", test_page_gpa, test_page_hva); > > > > /* guest will now write to the test buffer and we verify the contents */ > > sie(&vm); > > - assert(vm.sblk->icptcode == ICPT_INST && > > - vm.sblk->ipa == 0x8300 && vm.sblk->ipb == 0x440000); > > + assert(snippet_check_force_exit(&vm)); > > > > contents_match = true; > > for (unsigned int i = 0; i < GUEST_TEST_PAGE_COUNT; i++) { [...]
On Fri, 15 Dec 2023 12:50:15 +0100 Nina Schoetterl-Glausch <nsg@linux.ibm.com> wrote: > On Wed, 2023-12-13 at 17:45 +0100, Claudio Imbrenda wrote: > > On Wed, 13 Dec 2023 13:49:41 +0100 > > Nina Schoetterl-Glausch <nsg@linux.ibm.com> wrote: > > > > > Replace the existing code for exiting from snippets with the newly > > > introduced library functionality. > > > This causes additional report()s, otherwise no change in functionality > > > intended. > > > > > > Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com> > > > --- > > > s390x/sie-dat.c | 10 ++-------- > > > s390x/snippets/c/sie-dat.c | 19 +------------------ > > > 2 files changed, 3 insertions(+), 26 deletions(-) > > > > > > diff --git a/s390x/sie-dat.c b/s390x/sie-dat.c > > > index 9e60f26e..6b6e6868 100644 > > > --- a/s390x/sie-dat.c > > > +++ b/s390x/sie-dat.c > > > @@ -27,23 +27,17 @@ static void test_sie_dat(void) > > > uint64_t test_page_gpa, test_page_hpa; > > > uint8_t *test_page_hva, expected_val; > > > bool contents_match; > > > - uint8_t r1; > > > > > > /* guest will tell us the guest physical address of the test buffer */ > > > sie(&vm); > > > - assert(vm.sblk->icptcode == ICPT_INST && > > > - (vm.sblk->ipa & 0xff00) == 0x8300 && vm.sblk->ipb == 0x9c0000); > > > - > > > - r1 = (vm.sblk->ipa & 0xf0) >> 4; > > > - test_page_gpa = vm.save_area.guest.grs[r1]; > > > + assert(snippet_get_force_exit_value(&vm, &test_page_gpa)); > > > > but the function inside the assert will already report a failure, right? > > then why the assert? (the point I'm trying to make is that the function > > The assert makes sense, if it fails something has gone > completely off the rails. The question is indeed rather if to report. > There is no harm in it, but I'm thinking now that there should be > _get_ functions that don't report and optionally also _check_ functions that do. > > So: > snippet_get_force_exit > snippet_check_force_exit (exists, but only the _get_ variant is currently required) > snippet_get_force_exit_value (exists, required) > snippet_check_force_exit_value (exists, but unused) > > So minimally, we could do: > snippet_get_force_exit > snippet_get_force_exit_value > > I'm thinking we should go for the following: > bool snippet_has_force_exit(...) > bool snippet_has_force_exit_value(...) > uint64_t snippet_get_force_exit_value(...) (internally does assert(snippet_has_forced_exit_value)) > void snippet_check_force_exit_value(...) (or whoever needs this in the future adds it) > (Naming open to suggestions) I like this. I would call the bool ones "snippet_is_force_exit", in line with similar functions. usually "has" is used for capabilities or features. but I'm open to other suggestions/ideas > > Then this becomes: > /* guest will tell us the guest physical address of the test buffer */ > sie(&vm); > - assert(vm.sblk->icptcode == ICPT_INST && > - (vm.sblk->ipa & 0xff00) == 0x8300 && vm.sblk->ipb == 0x9c0000); > - > + assert(snippet_has_force_exit_value(&vm); > - r1 = (vm.sblk->ipa & 0xf0) >> 4; > - test_page_gpa = vm.save_area.guest.grs[r1]; > + test_page_gpa = snippet_get_force_exit_value(&vm); > > > should not report stuff, see my comments in the previous patch) > > > > > test_page_hpa = virt_to_pte_phys(guest_root, (void*)test_page_gpa); > > > test_page_hva = __va(test_page_hpa); > > > report_info("test buffer gpa=0x%lx hva=%p", test_page_gpa, test_page_hva); > > > > > > /* guest will now write to the test buffer and we verify the contents */ > > > sie(&vm); > > > - assert(vm.sblk->icptcode == ICPT_INST && > > > - vm.sblk->ipa == 0x8300 && vm.sblk->ipb == 0x440000); > > > + assert(snippet_check_force_exit(&vm)); > > > > > > contents_match = true; > > > for (unsigned int i = 0; i < GUEST_TEST_PAGE_COUNT; i++) { > > [...]
diff --git a/s390x/sie-dat.c b/s390x/sie-dat.c index 9e60f26e..6b6e6868 100644 --- a/s390x/sie-dat.c +++ b/s390x/sie-dat.c @@ -27,23 +27,17 @@ static void test_sie_dat(void) uint64_t test_page_gpa, test_page_hpa; uint8_t *test_page_hva, expected_val; bool contents_match; - uint8_t r1; /* guest will tell us the guest physical address of the test buffer */ sie(&vm); - assert(vm.sblk->icptcode == ICPT_INST && - (vm.sblk->ipa & 0xff00) == 0x8300 && vm.sblk->ipb == 0x9c0000); - - r1 = (vm.sblk->ipa & 0xf0) >> 4; - test_page_gpa = vm.save_area.guest.grs[r1]; + assert(snippet_get_force_exit_value(&vm, &test_page_gpa)); test_page_hpa = virt_to_pte_phys(guest_root, (void*)test_page_gpa); test_page_hva = __va(test_page_hpa); report_info("test buffer gpa=0x%lx hva=%p", test_page_gpa, test_page_hva); /* guest will now write to the test buffer and we verify the contents */ sie(&vm); - assert(vm.sblk->icptcode == ICPT_INST && - vm.sblk->ipa == 0x8300 && vm.sblk->ipb == 0x440000); + assert(snippet_check_force_exit(&vm)); contents_match = true; for (unsigned int i = 0; i < GUEST_TEST_PAGE_COUNT; i++) { diff --git a/s390x/snippets/c/sie-dat.c b/s390x/snippets/c/sie-dat.c index ecfcb60e..414afd42 100644 --- a/s390x/snippets/c/sie-dat.c +++ b/s390x/snippets/c/sie-dat.c @@ -9,28 +9,11 @@ */ #include <libcflat.h> #include <asm-generic/page.h> +#include <snippet-guest.h> #include "sie-dat.h" static uint8_t test_pages[GUEST_TEST_PAGE_COUNT * PAGE_SIZE] __attribute__((__aligned__(PAGE_SIZE))); -static inline void force_exit(void) -{ - asm volatile("diag 0,0,0x44\n" - : - : - : "memory" - ); -} - -static inline void force_exit_value(uint64_t val) -{ - asm volatile("diag %[val],0,0x9c\n" - : - : [val] "d"(val) - : "memory" - ); -} - int main(void) { uint8_t *invalid_ptr;
Replace the existing code for exiting from snippets with the newly introduced library functionality. This causes additional report()s, otherwise no change in functionality intended. Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com> --- s390x/sie-dat.c | 10 ++-------- s390x/snippets/c/sie-dat.c | 19 +------------------ 2 files changed, 3 insertions(+), 26 deletions(-)