Re: [kvm-unit-tests PATCH 3/5] s390x: Add library functions for exiting from snippet

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

On Wed, 2023-12-13 at 17:42 +0100, Claudio Imbrenda wrote:
> On Wed, 13 Dec 2023 13:49:40 +0100
> Nina Schoetterl-Glausch <nsg@linux.ibm.com> wrote:
> 
> > It is useful to be able to force an exit to the host from the snippet,
> > as well as do so while returning a value.
> > Add this functionality, also add helper functions for the host to check
> > for an exit and get or check the value.
> > Use diag 0x44 and 0x9c for this.
> > Add a guest specific snippet header file and rename the host's.
> 
> you should also mention here that you are splitting snippet.h into a
> host-only part and a guest-only part

Well, I'm not splitting anything. Is it not clear that "the host's"
refers to snippet.h?

How about:
Add a guest specific snippet header file and rename snippet.h to reflect
that it is host specific.
> 
> > 
> > Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> > ---
> >  s390x/Makefile                          |  1 +
> >  lib/s390x/asm/arch_def.h                | 13 ++++++++
> >  lib/s390x/sie.h                         |  1 +
> >  lib/s390x/snippet-guest.h               | 26 ++++++++++++++++
> >  lib/s390x/{snippet.h => snippet-host.h} |  9 ++++--
> >  lib/s390x/sie.c                         | 28 +++++++++++++++++
> >  lib/s390x/snippet-host.c                | 40 +++++++++++++++++++++++++
> >  lib/s390x/uv.c                          |  2 +-
> >  s390x/mvpg-sie.c                        |  2 +-
> >  s390x/pv-diags.c                        |  2 +-
> >  s390x/pv-icptcode.c                     |  2 +-
> >  s390x/pv-ipl.c                          |  2 +-
> >  s390x/sie-dat.c                         |  2 +-
> >  s390x/spec_ex-sie.c                     |  2 +-
> >  s390x/uv-host.c                         |  2 +-
> >  15 files changed, 123 insertions(+), 11 deletions(-)
> >  create mode 100644 lib/s390x/snippet-guest.h
> >  rename lib/s390x/{snippet.h => snippet-host.h} (93%)
> >  create mode 100644 lib/s390x/snippet-host.c
> 
> [...]
> 
> > diff --git a/lib/s390x/sie.c b/lib/s390x/sie.c
> > index 40936bd2..908b0130 100644
> > --- a/lib/s390x/sie.c
> > +++ b/lib/s390x/sie.c
> > @@ -42,6 +42,34 @@ void sie_check_validity(struct vm *vm, uint16_t vir_exp)
> >  	report(vir_exp == vir, "VALIDITY: %x", vir);
> >  }
> >  
> > +bool sie_is_diag_icpt(struct vm *vm, unsigned int diag)
> > +{
> > +	uint32_t ipb = vm->sblk->ipb;
> > +	uint64_t code;
> 
> uint64_t code = 0;
> 
> > +	uint16_t displace;
> > +	uint8_t base;
> > +	bool ret = true;
> 
> bool ret;
> 
> > +
> > +	ret = ret && vm->sblk->icptcode == ICPT_INST;
> > +	ret = ret && (vm->sblk->ipa & 0xff00) == 0x8300;
> 
> ret = vm->sblk->icptcode == ICPT_INST && (vm->sblk->ipa & 0xff00) ==
> 0x8300;

(*) see below
> 
> > +	switch (diag) {
> > +	case 0x44:
> > +	case 0x9c:
> > +		ret = ret && !(ipb & 0xffff);
> > +		ipb >>= 16;
> > +		displace = ipb & 0xfff;
> 
> maybe it's more readable to avoid shifting thigs around all the time:

I don't know, now I gotta be able to do rudimentary arithmetic :D
I don't really have a preference.
I wonder if defining a bit field would be worth it.
> 
> displace = (ipb >> 16) & 0xfff;
> base = (ipb >> 28) & 0xf;
> if (base)
> 	code = vm->....[base];
> code = (code + displace) & 0xffff;
> if (ipb & 0xffff || code != diag)
> 	return false;
> 
> > +		ipb >>= 12;
> > +		base = ipb & 0xf;
> > +		code = base ? vm->save_area.guest.grs[base] + displace : displace;
> > +		code &= 0xffff;
> > +		ret = ret && (code == diag);
> > +		break;
> > +	default:
> > +		abort(); /* not implemented */
> > +	}
> > +	return ret;
> 
> although I have the feeling that this would be more readable if you
> would check diag immediately, and avoid using ret

Not sure what you mean, do you want an early return at (*)?
> 
> > +}
> > +
> >  void sie_handle_validity(struct vm *vm)
> >  {
> >  	if (vm->sblk->icptcode != ICPT_VALIDITY)
> > diff --git a/lib/s390x/snippet-host.c b/lib/s390x/snippet-host.c
> > new file mode 100644
> > index 00000000..a829c1d5
> > --- /dev/null
> > +++ b/lib/s390x/snippet-host.c
> > @@ -0,0 +1,40 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Snippet functionality for the host.
> > + *
> > + * Copyright IBM Corp. 2023
> > + */
> > +
> > +#include <libcflat.h>
> > +#include <snippet-host.h>
> > +#include <sie.h>
> > +
> > +bool snippet_check_force_exit(struct vm *vm)
> > +{
> > +	bool r;
> > +
> > +	r = sie_is_diag_icpt(vm, 0x44);
> > +	report(r, "guest forced exit");
> > +	return r;
> > +}
> > +
> > +bool snippet_get_force_exit_value(struct vm *vm, uint64_t *value)
> > +{
> > +	struct kvm_s390_sie_block *sblk = vm->sblk;
> > +
> > +	if (sie_is_diag_icpt(vm, 0x9c)) {
> > +		*value = vm->save_area.guest.grs[(sblk->ipa & 0xf0) >> 4];
> > +		report_pass("guest forced exit with value: 0x%lx", *value);
> > +		return true;
> > +	}
> > +	report_fail("guest forced exit with value");
> > +	return false;
> > +}
> > +
> > +void snippet_check_force_exit_value(struct vm *vm, uint64_t value_exp)
> > +{
> > +	uint64_t value;
> > +
> > +	if (snippet_get_force_exit_value(vm, &value))
> > +		report(value == value_exp, "guest exit value matches 0x%lx", value_exp);
> > +}
> 
> from a readability and a consistency perspective, it would be better if
> the functions would only check stuff and return a bool or a value, and
> do the report() in the body of the testcase

Hmm, I chose to do the report in order to be consistent with check_pgm_int_code.
> 
> 
> [...]