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

[Claudio Imbrenda <imbrenda@linux.ibm.com>]

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++) {  
> 
> [...]