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

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

On Thu, 14 Dec 2023 21:02:53 +0100
Nina Schoetterl-Glausch <nsg@linux.ibm.com> wrote:

> 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.

sounds good

[...]

> > > 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;  
> > 

something like this:

assert(diag == 0x44 || diag == 0x9c);

if (vm->sblk->icptcode != ICPT_INST)
	return false;
if ((vm->sblk->ipa & 0xff00) != 0x8300)
	return false;
if (vm->sblk->ipb & 0xffff)
	return false;

code = ....

return code == diag;

> > 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.

I think it would.

maybe something like:

union ip_text {
	struct {
		unsigned long ipa:16;
		unsigned long ipb:32;
	};
	struct {
		unsigned long opcode:8;
		...
	};
}

then you can do this at the beginning of the function:

union ip_text ip = { .ipa = vm->sblk->ipa, .ipb = ... };

and then use only the bitfields

[...]