On Fri, 15 Dec 2023 12:50:15 +0100
Nina Schoetterl-Glausch <nsg@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> > > ---
> > > 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++) {
>
> [...]
