Re: [kvm-unit-tests PATCH v3 4/7] s390x: Add function for checking diagnose intercepts

["Nicholas Piggin" <npiggin@gmail.com>]

On Tue Jun 25, 2024 at 6:11 PM AEST, Nina Schoetterl-Glausch wrote:
> On Tue, 2024-06-25 at 12:14 +1000, Nicholas Piggin wrote:
> > On Fri Jun 21, 2024 at 12:16 AM AEST, Nina Schoetterl-Glausch wrote:
> > > sie_is_diag_icpt() checks if the intercept is due to an expected
> > > diagnose call and is valid.
> > > It subsumes pv_icptdata_check_diag.
> > > 
> > > Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> > > ---
> > >  lib/s390x/pv_icptdata.h | 42 --------------------------------
> > >  lib/s390x/sie.h         | 12 ++++++++++
> > >  lib/s390x/sie.c         | 53 +++++++++++++++++++++++++++++++++++++++++
> > >  s390x/pv-diags.c        |  8 +++----
> > >  s390x/pv-icptcode.c     | 11 ++++-----
> > >  s390x/pv-ipl.c          |  7 +++---
> > >  6 files changed, 76 insertions(+), 57 deletions(-)
> > >  delete mode 100644 lib/s390x/pv_icptdata.h
>
> [...]
>
> > > +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 };
> > > +	uint8_t icptcode;
> > > +	uint64_t code;
> > > +
> > > +	switch (diag) {
> > > +	case 0x44:
> > > +	case 0x9c:
> > > +	case 0x288:
> > > +	case 0x308:
> > > +		icptcode = ICPT_PV_NOTIFY;
> > > +		break;
> > > +	case 0x500:
> > > +		icptcode = ICPT_PV_INSTR;
> > > +		break;
> > > +	default:
> > > +		/* If a new diag is introduced add it to the cases above! */
> > > +		assert_msg(false, "unknown diag");
> > > +	}
> > > +
> > > +	if (sie_is_pv(vm)) {
> > > +		if (instr.r_1 != 0 || instr.r_2 != 2 || instr.r_base != 5)
> > > +			return false;
> > > +		if (instr.displace)
> > > +			return false;
> > > +	} else {
> > > +		icptcode = ICPT_INST;
> > > +	}
> > > +	if (vm->sblk->icptcode != icptcode)
> > > +		return false;
> > > +	if (instr.opcode != 0x83 || instr.zero)
> > > +		return false;
> > > +	code = instr.r_base ? vm->save_area.guest.grs[instr.r_base] : 0;
> > > +	code = (code + instr.displace) & 0xffff;
> > > +	return code == diag;
> > > +}
> > 
> > It looks like this transformation is equivalent for the PV case.
>
> Yes, the PV case just has hardcoded values that we want to check.
>
> > You
> > could put the switch into the sie_is_pv() branch? Otherwise looks okay.
>
> I want to validate diag for both PV and non PV.

To make sure it is one of listed cases, okay I missed that
point. All those same diag numbers are valid for !PV?
In that case it's fine.

Thanks,
Nick