diff mbox series

[RFC,13/37] KVM: s390: protvirt: Add interruption injection controls

Message ID 20191024114059.102802-14-frankja@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series KVM: s390: Add support for protected VMs | expand

Commit Message

Janosch Frank Oct. 24, 2019, 11:40 a.m. UTC 
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.

Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
---
 arch/s390/include/asm/kvm_host.h | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

Comments

David Hildenbrand Oct. 30, 2019, 3:53 p.m. UTC | #1 
On 24.10.19 13:40, Janosch Frank 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.
> 
> 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 6cc3b73ca904..82443236d4cc 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -215,7 +215,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 */
> @@ -252,7 +260,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 */
> @@ -268,8 +277,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 */
> +		};

I only wonder if we should give this member a fitting name, e.g., "ioparams"

> +	} __packed;
> +	__u8	reservedf4[8];		/* 0x00f4 */
>   #define CRYCB_FORMAT_MASK 0x00000003
>   #define CRYCB_FORMAT0 0x00000000
>   #define CRYCB_FORMAT1 0x00000001
>
Michael Mueller Oct. 31, 2019, 8:48 a.m. UTC | #2 
On 30.10.19 16:53, David Hildenbrand wrote:
>> @@ -268,8 +277,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 */
>> +        };
> 
> I only wonder if we should give this member a fitting name, e.g., 
> "ioparams"

Do you see a real gain for that? We have a lot of other unnamed structs
defined here as well.


> 
>> +    } __packed;
>> +    __u8    reservedf4[8];        /* 0x00f4 */
>>   #define CRYCB_FORMAT_MASK 0x00000003
>>   #define CRYCB_FORMAT0 0x00000000
>>   #define CRYCB_FORMAT1 0x00000001
>>

Thanks,
Michael
David Hildenbrand Oct. 31, 2019, 9:15 a.m. UTC | #3 
On 31.10.19 09:48, Michael Mueller wrote:
> 
> 
> On 30.10.19 16:53, David Hildenbrand wrote:
>>> @@ -268,8 +277,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 */
>>> +        };
>>
>> I only wonder if we should give this member a fitting name, e.g., 
>> "ioparams"
> 
> Do you see a real gain for that? We have a lot of other unnamed structs
> defined here as well.

I was wondering if we could just copy the whole struct when delivering
the interrupt.

You could even reuse  "struct kvm_s390_io_info" here to make that more
clear.
Michael Mueller Oct. 31, 2019, 12:10 p.m. UTC | #4 
On 31.10.19 10:15, David Hildenbrand wrote:
> On 31.10.19 09:48, Michael Mueller wrote:
>>
>>
>> On 30.10.19 16:53, David Hildenbrand wrote:
>>>> @@ -268,8 +277,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 */
>>>> +        };
>>>
>>> I only wonder if we should give this member a fitting name, e.g.,
>>> "ioparams"
>>
>> Do you see a real gain for that? We have a lot of other unnamed structs
>> defined here as well.
> 
> I was wondering if we could just copy the whole struct when delivering
> the interrupt.
> 
> You could even reuse  "struct kvm_s390_io_info" here to make that more
> clear.

I want to keep it the way it is to have the fields in the SCB
declaration explicit.

Thanks,
Michael
Cornelia Huck Nov. 5, 2019, 5:51 p.m. UTC | #5 
On Thu, 24 Oct 2019 07:40:35 -0400
Janosch Frank <frankja@linux.ibm.com> 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.
> 
> 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 6cc3b73ca904..82443236d4cc 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -215,7 +215,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 */
> @@ -252,7 +260,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 */
> @@ -268,8 +277,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 */

IIUC, for protected guests, you won't get an interception for which
tecmc would be valid anymore, but need to put the I/O interruption
stuff at the same place, right?

My main issue is that this makes the control block definition a bit
ugly, since the f0 value that's unused in the non-protvirt case is not
obvious anymore; but I don't know how to express this without making it
even uglier :(

>  #define CRYCB_FORMAT_MASK 0x00000003
>  #define CRYCB_FORMAT0 0x00000000
>  #define CRYCB_FORMAT1 0x00000001
Michael Mueller Nov. 7, 2019, 12:42 p.m. UTC | #6 
On 05.11.19 18:51, Cornelia Huck wrote:
> On Thu, 24 Oct 2019 07:40:35 -0400
> Janosch Frank <frankja@linux.ibm.com> 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.
>>
>> 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 6cc3b73ca904..82443236d4cc 100644
>> --- a/arch/s390/include/asm/kvm_host.h
>> +++ b/arch/s390/include/asm/kvm_host.h
>> @@ -215,7 +215,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 */
>> @@ -252,7 +260,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 */
>> @@ -268,8 +277,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 */
> 
> IIUC, for protected guests, you won't get an interception for which
> tecmc would be valid anymore, but need to put the I/O interruption
> stuff at the same place, right?

Yes, the format 4 architecture defines this.

> 
> My main issue is that this makes the control block definition a bit
> ugly, since the f0 value that's unused in the non-protvirt case is not
> obvious anymore; but I don't know how to express this without making it
> even uglier :(

:)

> 
>>   #define CRYCB_FORMAT_MASK 0x00000003
>>   #define CRYCB_FORMAT0 0x00000000
>>   #define CRYCB_FORMAT1 0x00000001
> 

Thanks
Michael
Thomas Huth Nov. 14, 2019, 11:48 a.m. UTC | #7 
On 24/10/2019 13.40, Janosch Frank 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.
> 
> 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 6cc3b73ca904..82443236d4cc 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -215,7 +215,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 */
> @@ -252,7 +260,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 */
> @@ -268,8 +277,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 */

I have to admit that I always have to think twice where the compiler
might put the padding in this case. Maybe you could do that manually to
make it obvious and wrap it in a struct, too:

                struct {
			__u64	tecmc;		/* 0x00e8 */
			__u8	reservedf0[4];	/* 0x00f0 */
 		};

?

Just my 0.02 €, though.

 Thomas


> +		struct {
> +			__u16	subchannel_id;	/* 0x00e8 */
> +			__u16	subchannel_nr;	/* 0x00ea */
> +			__u32	io_int_parm;	/* 0x00ec */
> +			__u32	io_int_word;	/* 0x00f0 */
> +		};
> +	} __packed;
> +	__u8	reservedf4[8];		/* 0x00f4 */
>  #define CRYCB_FORMAT_MASK 0x00000003
>  #define CRYCB_FORMAT0 0x00000000
>  #define CRYCB_FORMAT1 0x00000001
>
diff mbox series

Patch

diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index 6cc3b73ca904..82443236d4cc 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -215,7 +215,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 */
@@ -252,7 +260,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 */
@@ -268,8 +277,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 */
 #define CRYCB_FORMAT_MASK 0x00000003
 #define CRYCB_FORMAT0 0x00000000
 #define CRYCB_FORMAT1 0x00000001