Re: [RFCv2 14/37] KVM: s390: protvirt: Add interruption injection controls

[Cornelia Huck <cohuck@redhat.com>]

On Wed, 5 Feb 2020 09:54:23 +0100
Michael Mueller <mimu@linux.ibm.com> wrote:

> On 05.02.20 07:59, Thomas Huth wrote:
> > On 03/02/2020 14.19, Christian Borntraeger wrote:  
> >> From: Michael Mueller <mimu@linux.ibm.com>
> >>
> >> Define the interruption injection codes and the related fields in the
> >> sie control block for PVM interruption injection.  
> > 
> > You seem to only add the details for external interrupts and I/O
> > interrupts here? Maybe mention this in the description ... otherwise it
> > is confusing when you read patch 17 later ... or maybe merge this patch
> > here with patch 17 ?  
> 
> In that case I will merge patch 17 into this one.

Merging patch 17 sounds good to me, as it gets us all of the new
interrupt controls in one go.

> 
> >   
> >> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
> >> ---
> >>   arch/s390/include/asm/kvm_host.h | 25 +++++++++++++++++++++----
> >>   1 file changed, 21 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> >> index 58845b315be0..a45d10d87a8a 100644
> >> --- a/arch/s390/include/asm/kvm_host.h
> >> +++ b/arch/s390/include/asm/kvm_host.h
> >> @@ -222,7 +222,15 @@ struct kvm_s390_sie_block {
> >>   	__u8	icptcode;		/* 0x0050 */
> >>   	__u8	icptstatus;		/* 0x0051 */
> >>   	__u16	ihcpu;			/* 0x0052 */
> >> -	__u8	reserved54[2];		/* 0x0054 */
> >> +	__u8	reserved54;		/* 0x0054 */
> >> +#define IICTL_CODE_NONE		 0x00
> >> +#define IICTL_CODE_MCHK		 0x01
> >> +#define IICTL_CODE_EXT		 0x02
> >> +#define IICTL_CODE_IO		 0x03
> >> +#define IICTL_CODE_RESTART	 0x04
> >> +#define IICTL_CODE_SPECIFICATION 0x10
> >> +#define IICTL_CODE_OPERAND	 0x11
> >> +	__u8	iictl;			/* 0x0055 */
> >>   	__u16	ipa;			/* 0x0056 */
> >>   	__u32	ipb;			/* 0x0058 */
> >>   	__u32	scaoh;			/* 0x005c */
> >> @@ -259,7 +267,8 @@ struct kvm_s390_sie_block {
> >>   #define HPID_KVM	0x4
> >>   #define HPID_VSIE	0x5
> >>   	__u8	hpid;			/* 0x00b8 */
> >> -	__u8	reservedb9[11];		/* 0x00b9 */
> >> +	__u8	reservedb9[7];		/* 0x00b9 */
> >> +	__u32	eiparams;		/* 0x00c0 */
> >>   	__u16	extcpuaddr;		/* 0x00c4 */
> >>   	__u16	eic;			/* 0x00c6 */
> >>   	__u32	reservedc8;		/* 0x00c8 */
> >> @@ -275,8 +284,16 @@ struct kvm_s390_sie_block {
> >>   	__u8	oai;			/* 0x00e2 */
> >>   	__u8	armid;			/* 0x00e3 */
> >>   	__u8	reservede4[4];		/* 0x00e4 */
> >> -	__u64	tecmc;			/* 0x00e8 */
> >> -	__u8	reservedf0[12];		/* 0x00f0 */
> >> +	union {
> >> +		__u64	tecmc;		/* 0x00e8 */
> >> +		struct {
> >> +			__u16	subchannel_id;	/* 0x00e8 */
> >> +			__u16	subchannel_nr;	/* 0x00ea */
> >> +			__u32	io_int_parm;	/* 0x00ec */
> >> +			__u32	io_int_word;	/* 0x00f0 */
> >> +		};
> >> +	} __packed;
> >> +	__u8	reservedf4[8];		/* 0x00f4 */  
> > 
> > Maybe add a comment to the new struct for which injection type it is
> > good for ... otherwise this might get hard to understand in the future
> > (especially if more stuff gets added like in patch 17).  
> 
> Up to know we don't have comments in the the SIE control block struct at 
> all as this is part of the documentation, also for protected virtualization.

FWIW, I find the sie control block structure already hard to understand
right now. Some documentation would be useful, if possible.

> 
> Any other opinions on that are welcome.
> 
> > 
> > Anyway,
> > Reviewed-by: Thomas Huth <thuth@redhat.com>  
> 
> thanks
> 
> >   
> 
> Michael
>