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

Claudio Imbrenda
2023-12-15T13:53:45Z

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 …