Re: [kvm-unit-tests PATCH 4/5] s390x: Use library functions for snippet exit

[Nina Schoetterl-Glausch <nsg@linux.ibm.com>]

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++) {

[...]