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

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

On Mon, 2024-01-08 at 13:47 +0100, Janosch Frank wrote:
> On 1/5/24 23:54, Nina Schoetterl-Glausch 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 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} | 10 ++++--
> >   lib/s390x/sie.c                         | 31 ++++++++++++++++++
> >   lib/s390x/snippet-host.c                | 42 +++++++++++++++++++++++++
> >   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, 129 insertions(+), 11 deletions(-)
> >   create mode 100644 lib/s390x/snippet-guest.h
> >   rename lib/s390x/{snippet.h => snippet-host.h} (92%)
> >   create mode 100644 lib/s390x/snippet-host.c

[..]
 
> > +bool sie_is_diag_icpt(struct vm *vm, unsigned int diag)
> > +{
> > +	union {
> > +		struct {
> > +			uint64_t     : 16;
> > +			uint64_t ipa : 16;
> > +			uint64_t ipb : 32;
> > +		};
> > +		struct {
> > +			uint64_t          : 16;
> > +			uint64_t opcode   :  8;
> > +			uint64_t r_1      :  4;
> > +			uint64_t r_2      :  4;
> > +			uint64_t r_base   :  4;
> > +			uint64_t displace : 12;
> > +			uint64_t zero     : 16;
> > +		};
> > +	} instr = { .ipa = vm->sblk->ipa, .ipb = vm->sblk->ipb };
> > +	uint64_t code;
> > +
> > +	assert(diag == 0x44 || diag == 0x9c);
> 
> You're calling it is_diag_icpt and only allow two.
> Do you have a reason for clamping this down?

I should have left the comment.
They're just "not implemented".
The PoP doesn't specify how diags are generally interpreted,
so I intended that if any other diags are needed whoever needs them
just checks if the existing logic works or if changes are required.
> 
> I was considering consolidating pv_icptdata_check_diag() into this.