[v2,4/7] KVM: s390: enable MSA9 keywrapping functions depending on cpu model
diff mbox series

Message ID 20190417182925.50744-1-borntraeger@de.ibm.com
State New
Headers show
Series
  • Untitled series #107191
Related show

Commit Message

Christian Borntraeger April 17, 2019, 6:29 p.m. UTC
Instead of adding a new machine option to disable/enable the keywrapping
options of pckmo (like for AES and DEA) we can now use the CPU model to
decide.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
Reviewed-by: Collin Walling <walling@linux.ibm.com>
---
v1->v2: - enable vsie
	- also check if the host has the pckmo functions
 arch/s390/include/asm/kvm_host.h | 1 +
 arch/s390/kvm/kvm-s390.c         | 7 +++++++
 arch/s390/kvm/vsie.c             | 5 ++++-
 3 files changed, 12 insertions(+), 1 deletion(-)

Comments

Christian Borntraeger April 18, 2019, 7:35 a.m. UTC | #1
On 17.04.19 20:29, Christian Borntraeger wrote:
> Instead of adding a new machine option to disable/enable the keywrapping
> options of pckmo (like for AES and DEA) we can now use the CPU model to
> decide.
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Reviewed-by: Collin Walling <walling@linux.ibm.com>
> ---
> v1->v2: - enable vsie
> 	- also check if the host has the pckmo functions
>  arch/s390/include/asm/kvm_host.h | 1 +
>  arch/s390/kvm/kvm-s390.c         | 7 +++++++
>  arch/s390/kvm/vsie.c             | 5 ++++-
>  3 files changed, 12 insertions(+), 1 deletion(-)

FWIW, I tested this variant successfully with some printk debugging to 
check if the settings are good. The only question is: does anybody cares
about
	if ((vcpu->kvm->arch.model.subfuncs.pckmo[4] & kvm_s390_available_subfunc.pckmo[4] & 0xe0) ||
	    (vcpu->kvm->arch.model.subfuncs.pckmo[5] & kvm_s390_available_subfunc.pckmo[5] & 0xc0))

being too long? I find this more readable than 

	if ((vcpu->kvm->arch.model.subfuncs.pckmo[4] &
	     kvm_s390_available_subfunc.pckmo[4] & 0xe0) ||
	    (vcpu->kvm->arch.model.subfuncs.pckmo[5] &
	     kvm_s390_available_subfunc.pckmo[5] & 0xc0))

Christian
David Hildenbrand April 18, 2019, 7:49 a.m. UTC | #2
On 18.04.19 09:35, Christian Borntraeger wrote:
> On 17.04.19 20:29, Christian Borntraeger wrote:
>> Instead of adding a new machine option to disable/enable the keywrapping
>> options of pckmo (like for AES and DEA) we can now use the CPU model to
>> decide.
>>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> Reviewed-by: Collin Walling <walling@linux.ibm.com>
>> ---
>> v1->v2: - enable vsie
>> 	- also check if the host has the pckmo functions
>>  arch/s390/include/asm/kvm_host.h | 1 +
>>  arch/s390/kvm/kvm-s390.c         | 7 +++++++
>>  arch/s390/kvm/vsie.c             | 5 ++++-
>>  3 files changed, 12 insertions(+), 1 deletion(-)
> 
> FWIW, I tested this variant successfully with some printk debugging to 
> check if the settings are good. The only question is: does anybody cares
> about
> 	if ((vcpu->kvm->arch.model.subfuncs.pckmo[4] & kvm_s390_available_subfunc.pckmo[4] & 0xe0) ||
> 	    (vcpu->kvm->arch.model.subfuncs.pckmo[5] & kvm_s390_available_subfunc.pckmo[5] & 0xc0))
> 
> being too long? I find this more readable than 
> 
> 	if ((vcpu->kvm->arch.model.subfuncs.pckmo[4] &
> 	     kvm_s390_available_subfunc.pckmo[4] & 0xe0) ||
> 	    (vcpu->kvm->arch.model.subfuncs.pckmo[5] &
> 	     kvm_s390_available_subfunc.pckmo[5] & 0xc0))
> 

Can you just factor that out into a function / makro?

> Christian
>
David Hildenbrand April 18, 2019, 7:54 a.m. UTC | #3
On 17.04.19 20:29, Christian Borntraeger wrote:
> Instead of adding a new machine option to disable/enable the keywrapping
> options of pckmo (like for AES and DEA) we can now use the CPU model to
> decide.
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Reviewed-by: Collin Walling <walling@linux.ibm.com>
> ---
> v1->v2: - enable vsie
> 	- also check if the host has the pckmo functions
>  arch/s390/include/asm/kvm_host.h | 1 +
>  arch/s390/kvm/kvm-s390.c         | 7 +++++++
>  arch/s390/kvm/vsie.c             | 5 ++++-
>  3 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index c47e22bba87fa..e224246ff93c6 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -278,6 +278,7 @@ struct kvm_s390_sie_block {
>  #define ECD_HOSTREGMGMT	0x20000000
>  #define ECD_MEF		0x08000000
>  #define ECD_ETOKENF	0x02000000
> +#define ECD_ECC		0x00200000
>  	__u32	ecd;			/* 0x01c8 */
>  	__u8	reserved1cc[18];	/* 0x01cc */
>  	__u64	pp;			/* 0x01de */
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 0dad61ccde3d6..9869d785677f1 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -2933,6 +2933,13 @@ int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu)
>  		VCPU_EVENT(vcpu, 3, "AIV gisa format-%u enabled for cpu %03u",
>  			   vcpu->arch.sie_block->gd & 0x3, vcpu->vcpu_id);
>  	}
> +	/*
> +	 * if any of 32,33,34,40,41 is active in host AND guest,
> +	 * we enable pckmo for ecc
> +	 */
> +	if ((vcpu->kvm->arch.model.subfuncs.pckmo[4] & kvm_s390_available_subfunc.pckmo[4] & 0xe0) ||
> +	    (vcpu->kvm->arch.model.subfuncs.pckmo[5] & kvm_s390_available_subfunc.pckmo[5] & 0xc0))

Maybe some helper like

bool kvm_has_pckmo_subfunc(kvm, nr)
{
	/* magic for one number */
}
...

if (kvm_has_pckmo_subfunc(kvm, 32) || kvm_has_pckmo_subfunc(kvm, 33))
...

then you can also get rid of the comment.

> +		vcpu->arch.sie_block->ecd |= ECD_ECC;
>  	vcpu->arch.sie_block->sdnxo = ((unsigned long) &vcpu->run->s.regs.sdnx)
>  					| SDNXC;
>  	vcpu->arch.sie_block->riccbd = (unsigned long) &vcpu->run->s.regs.riccb;
> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
> index d62fa148558b9..c6983d962abfd 100644
> --- a/arch/s390/kvm/vsie.c
> +++ b/arch/s390/kvm/vsie.c
> @@ -288,6 +288,7 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>  	const u32 crycb_addr = crycbd_o & 0x7ffffff8U;
>  	unsigned long *b1, *b2;
>  	u8 ecb3_flags;
> +	u32 ecd_flags;
>  	int apie_h;
>  	int key_msk = test_kvm_facility(vcpu->kvm, 76);
>  	int fmt_o = crycbd_o & CRYCB_FORMAT_MASK;
> @@ -320,7 +321,8 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>  	/* we may only allow it if enabled for guest 2 */
>  	ecb3_flags = scb_o->ecb3 & vcpu->arch.sie_block->ecb3 &
>  		     (ECB3_AES | ECB3_DEA);
> -	if (!ecb3_flags)
> +	ecd_flags = scb_o->ecd & vcpu->arch.sie_block->ecd & ECD_ECC;
> +	if (!ecb3_flags && !ecd_flags)
>  		goto end;

Just so I get it right, there are no *new* wrapping keys? Which wrapping
keys are used then?

>  
>  	/* copy only the wrapping keys */
> @@ -329,6 +331,7 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>  		return set_validity_icpt(scb_s, 0x0035U);
>  
>  	scb_s->ecb3 |= ecb3_flags;
> +	scb_s->ecd |= ecd_flags;
>  
>  	/* xor both blocks in one run */
>  	b1 = (unsigned long *) vsie_page->crycb.dea_wrapping_key_mask;
>
Christian Borntraeger April 18, 2019, 8:58 a.m. UTC | #4
On 18.04.19 09:54, David Hildenbrand wrote:
> On 17.04.19 20:29, Christian Borntraeger wrote:
>> Instead of adding a new machine option to disable/enable the keywrapping
>> options of pckmo (like for AES and DEA) we can now use the CPU model to
>> decide.
>>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> Reviewed-by: Collin Walling <walling@linux.ibm.com>
>> ---
>> v1->v2: - enable vsie
>> 	- also check if the host has the pckmo functions
>>  arch/s390/include/asm/kvm_host.h | 1 +
>>  arch/s390/kvm/kvm-s390.c         | 7 +++++++
>>  arch/s390/kvm/vsie.c             | 5 ++++-
>>  3 files changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
>> index c47e22bba87fa..e224246ff93c6 100644
>> --- a/arch/s390/include/asm/kvm_host.h
>> +++ b/arch/s390/include/asm/kvm_host.h
>> @@ -278,6 +278,7 @@ struct kvm_s390_sie_block {
>>  #define ECD_HOSTREGMGMT	0x20000000
>>  #define ECD_MEF		0x08000000
>>  #define ECD_ETOKENF	0x02000000
>> +#define ECD_ECC		0x00200000
>>  	__u32	ecd;			/* 0x01c8 */
>>  	__u8	reserved1cc[18];	/* 0x01cc */
>>  	__u64	pp;			/* 0x01de */
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index 0dad61ccde3d6..9869d785677f1 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -2933,6 +2933,13 @@ int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu)
>>  		VCPU_EVENT(vcpu, 3, "AIV gisa format-%u enabled for cpu %03u",
>>  			   vcpu->arch.sie_block->gd & 0x3, vcpu->vcpu_id);
>>  	}
>> +	/*
>> +	 * if any of 32,33,34,40,41 is active in host AND guest,
>> +	 * we enable pckmo for ecc
>> +	 */
>> +	if ((vcpu->kvm->arch.model.subfuncs.pckmo[4] & kvm_s390_available_subfunc.pckmo[4] & 0xe0) ||
>> +	    (vcpu->kvm->arch.model.subfuncs.pckmo[5] & kvm_s390_available_subfunc.pckmo[5] & 0xc0))
> 
> Maybe some helper like
> 
> bool kvm_has_pckmo_subfunc(kvm, nr)
> {
> 	/* magic for one number */
> }
> ...
> 
> if (kvm_has_pckmo_subfunc(kvm, 32) || kvm_has_pckmo_subfunc(kvm, 33))
> ...
> 
> then you can also get rid of the comment.

Will give it a try.
> 
>> +		vcpu->arch.sie_block->ecd |= ECD_ECC;
>>  	vcpu->arch.sie_block->sdnxo = ((unsigned long) &vcpu->run->s.regs.sdnx)
>>  					| SDNXC;
>>  	vcpu->arch.sie_block->riccbd = (unsigned long) &vcpu->run->s.regs.riccb;
>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
>> index d62fa148558b9..c6983d962abfd 100644
>> --- a/arch/s390/kvm/vsie.c
>> +++ b/arch/s390/kvm/vsie.c
>> @@ -288,6 +288,7 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>  	const u32 crycb_addr = crycbd_o & 0x7ffffff8U;
>>  	unsigned long *b1, *b2;
>>  	u8 ecb3_flags;
>> +	u32 ecd_flags;
>>  	int apie_h;
>>  	int key_msk = test_kvm_facility(vcpu->kvm, 76);
>>  	int fmt_o = crycbd_o & CRYCB_FORMAT_MASK;
>> @@ -320,7 +321,8 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>  	/* we may only allow it if enabled for guest 2 */
>>  	ecb3_flags = scb_o->ecb3 & vcpu->arch.sie_block->ecb3 &
>>  		     (ECB3_AES | ECB3_DEA);
>> -	if (!ecb3_flags)
>> +	ecd_flags = scb_o->ecd & vcpu->arch.sie_block->ecd & ECD_ECC;
>> +	if (!ecb3_flags && !ecd_flags)
>>  		goto end;
> 
> Just so I get it right, there are no *new* wrapping keys? Which wrapping
> keys are used then?

Yes, AES.
David Hildenbrand April 18, 2019, 9:13 a.m. UTC | #5
On 18.04.19 10:58, Christian Borntraeger wrote:
> 
> 
> On 18.04.19 09:54, David Hildenbrand wrote:
>> On 17.04.19 20:29, Christian Borntraeger wrote:
>>> Instead of adding a new machine option to disable/enable the keywrapping
>>> options of pckmo (like for AES and DEA) we can now use the CPU model to
>>> decide.
>>>
>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>> Reviewed-by: Collin Walling <walling@linux.ibm.com>
>>> ---
>>> v1->v2: - enable vsie
>>> 	- also check if the host has the pckmo functions
>>>  arch/s390/include/asm/kvm_host.h | 1 +
>>>  arch/s390/kvm/kvm-s390.c         | 7 +++++++
>>>  arch/s390/kvm/vsie.c             | 5 ++++-
>>>  3 files changed, 12 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
>>> index c47e22bba87fa..e224246ff93c6 100644
>>> --- a/arch/s390/include/asm/kvm_host.h
>>> +++ b/arch/s390/include/asm/kvm_host.h
>>> @@ -278,6 +278,7 @@ struct kvm_s390_sie_block {
>>>  #define ECD_HOSTREGMGMT	0x20000000
>>>  #define ECD_MEF		0x08000000
>>>  #define ECD_ETOKENF	0x02000000
>>> +#define ECD_ECC		0x00200000
>>>  	__u32	ecd;			/* 0x01c8 */
>>>  	__u8	reserved1cc[18];	/* 0x01cc */
>>>  	__u64	pp;			/* 0x01de */
>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>>> index 0dad61ccde3d6..9869d785677f1 100644
>>> --- a/arch/s390/kvm/kvm-s390.c
>>> +++ b/arch/s390/kvm/kvm-s390.c
>>> @@ -2933,6 +2933,13 @@ int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu)
>>>  		VCPU_EVENT(vcpu, 3, "AIV gisa format-%u enabled for cpu %03u",
>>>  			   vcpu->arch.sie_block->gd & 0x3, vcpu->vcpu_id);
>>>  	}
>>> +	/*
>>> +	 * if any of 32,33,34,40,41 is active in host AND guest,
>>> +	 * we enable pckmo for ecc
>>> +	 */
>>> +	if ((vcpu->kvm->arch.model.subfuncs.pckmo[4] & kvm_s390_available_subfunc.pckmo[4] & 0xe0) ||
>>> +	    (vcpu->kvm->arch.model.subfuncs.pckmo[5] & kvm_s390_available_subfunc.pckmo[5] & 0xc0))
>>
>> Maybe some helper like
>>
>> bool kvm_has_pckmo_subfunc(kvm, nr)
>> {
>> 	/* magic for one number */
>> }
>> ...
>>
>> if (kvm_has_pckmo_subfunc(kvm, 32) || kvm_has_pckmo_subfunc(kvm, 33))
>> ...
>>
>> then you can also get rid of the comment.
> 
> Will give it a try.
>>
>>> +		vcpu->arch.sie_block->ecd |= ECD_ECC;
>>>  	vcpu->arch.sie_block->sdnxo = ((unsigned long) &vcpu->run->s.regs.sdnx)
>>>  					| SDNXC;
>>>  	vcpu->arch.sie_block->riccbd = (unsigned long) &vcpu->run->s.regs.riccb;
>>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
>>> index d62fa148558b9..c6983d962abfd 100644
>>> --- a/arch/s390/kvm/vsie.c
>>> +++ b/arch/s390/kvm/vsie.c
>>> @@ -288,6 +288,7 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>>  	const u32 crycb_addr = crycbd_o & 0x7ffffff8U;
>>>  	unsigned long *b1, *b2;
>>>  	u8 ecb3_flags;
>>> +	u32 ecd_flags;
>>>  	int apie_h;
>>>  	int key_msk = test_kvm_facility(vcpu->kvm, 76);
>>>  	int fmt_o = crycbd_o & CRYCB_FORMAT_MASK;
>>> @@ -320,7 +321,8 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>>  	/* we may only allow it if enabled for guest 2 */
>>>  	ecb3_flags = scb_o->ecb3 & vcpu->arch.sie_block->ecb3 &
>>>  		     (ECB3_AES | ECB3_DEA);
>>> -	if (!ecb3_flags)
>>> +	ecd_flags = scb_o->ecd & vcpu->arch.sie_block->ecd & ECD_ECC;
>>> +	if (!ecb3_flags && !ecd_flags)
>>>  		goto end;
>>
>> Just so I get it right, there are no *new* wrapping keys? Which wrapping
>> keys are used then?
> 
> Yes, AES.
> 

Hmmmm, so if user space doesn't call KVM_S390_VM_CRYPTO_ENABLE_AES_KW,
the wrapping key is basically uninitialized (kvm_s390_vm_set_crypto()),
but will be used.

I guess you should also check against kvm->arch.crypto.aes_kw before
turning the ecd bit on, just to be sure.
Christian Borntraeger April 18, 2019, 10:17 a.m. UTC | #6
On 18.04.19 11:13, David Hildenbrand wrote:
> On 18.04.19 10:58, Christian Borntraeger wrote:
>>
>>
>> On 18.04.19 09:54, David Hildenbrand wrote:
>>> On 17.04.19 20:29, Christian Borntraeger wrote:
>>>> Instead of adding a new machine option to disable/enable the keywrapping
>>>> options of pckmo (like for AES and DEA) we can now use the CPU model to
>>>> decide.
>>>>
>>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>>> Reviewed-by: Collin Walling <walling@linux.ibm.com>
>>>> ---
>>>> v1->v2: - enable vsie
>>>> 	- also check if the host has the pckmo functions
>>>>  arch/s390/include/asm/kvm_host.h | 1 +
>>>>  arch/s390/kvm/kvm-s390.c         | 7 +++++++
>>>>  arch/s390/kvm/vsie.c             | 5 ++++-
>>>>  3 files changed, 12 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
>>>> index c47e22bba87fa..e224246ff93c6 100644
>>>> --- a/arch/s390/include/asm/kvm_host.h
>>>> +++ b/arch/s390/include/asm/kvm_host.h
>>>> @@ -278,6 +278,7 @@ struct kvm_s390_sie_block {
>>>>  #define ECD_HOSTREGMGMT	0x20000000
>>>>  #define ECD_MEF		0x08000000
>>>>  #define ECD_ETOKENF	0x02000000
>>>> +#define ECD_ECC		0x00200000
>>>>  	__u32	ecd;			/* 0x01c8 */
>>>>  	__u8	reserved1cc[18];	/* 0x01cc */
>>>>  	__u64	pp;			/* 0x01de */
>>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>>>> index 0dad61ccde3d6..9869d785677f1 100644
>>>> --- a/arch/s390/kvm/kvm-s390.c
>>>> +++ b/arch/s390/kvm/kvm-s390.c
>>>> @@ -2933,6 +2933,13 @@ int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu)
>>>>  		VCPU_EVENT(vcpu, 3, "AIV gisa format-%u enabled for cpu %03u",
>>>>  			   vcpu->arch.sie_block->gd & 0x3, vcpu->vcpu_id);
>>>>  	}
>>>> +	/*
>>>> +	 * if any of 32,33,34,40,41 is active in host AND guest,
>>>> +	 * we enable pckmo for ecc
>>>> +	 */
>>>> +	if ((vcpu->kvm->arch.model.subfuncs.pckmo[4] & kvm_s390_available_subfunc.pckmo[4] & 0xe0) ||
>>>> +	    (vcpu->kvm->arch.model.subfuncs.pckmo[5] & kvm_s390_available_subfunc.pckmo[5] & 0xc0))
>>>
>>> Maybe some helper like
>>>
>>> bool kvm_has_pckmo_subfunc(kvm, nr)
>>> {
>>> 	/* magic for one number */
>>> }
>>> ...
>>>
>>> if (kvm_has_pckmo_subfunc(kvm, 32) || kvm_has_pckmo_subfunc(kvm, 33))
>>> ...
>>>
>>> then you can also get rid of the comment.
>>
>> Will give it a try.
>>>
>>>> +		vcpu->arch.sie_block->ecd |= ECD_ECC;
>>>>  	vcpu->arch.sie_block->sdnxo = ((unsigned long) &vcpu->run->s.regs.sdnx)
>>>>  					| SDNXC;
>>>>  	vcpu->arch.sie_block->riccbd = (unsigned long) &vcpu->run->s.regs.riccb;
>>>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
>>>> index d62fa148558b9..c6983d962abfd 100644
>>>> --- a/arch/s390/kvm/vsie.c
>>>> +++ b/arch/s390/kvm/vsie.c
>>>> @@ -288,6 +288,7 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>>>  	const u32 crycb_addr = crycbd_o & 0x7ffffff8U;
>>>>  	unsigned long *b1, *b2;
>>>>  	u8 ecb3_flags;
>>>> +	u32 ecd_flags;
>>>>  	int apie_h;
>>>>  	int key_msk = test_kvm_facility(vcpu->kvm, 76);
>>>>  	int fmt_o = crycbd_o & CRYCB_FORMAT_MASK;
>>>> @@ -320,7 +321,8 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>>>  	/* we may only allow it if enabled for guest 2 */
>>>>  	ecb3_flags = scb_o->ecb3 & vcpu->arch.sie_block->ecb3 &
>>>>  		     (ECB3_AES | ECB3_DEA);
>>>> -	if (!ecb3_flags)
>>>> +	ecd_flags = scb_o->ecd & vcpu->arch.sie_block->ecd & ECD_ECC;
>>>> +	if (!ecb3_flags && !ecd_flags)
>>>>  		goto end;
>>>
>>> Just so I get it right, there are no *new* wrapping keys? Which wrapping
>>> keys are used then?
>>
>> Yes, AES.
>>
> 
> Hmmmm, so if user space doesn't call KVM_S390_VM_CRYPTO_ENABLE_AES_KW,
> the wrapping key is basically uninitialized (kvm_s390_vm_set_crypto()),
> but will be used.
> 
> I guess you should also check against kvm->arch.crypto.aes_kw before
> turning the ecd bit on, just to be sure.

We should rather initialize the aes value when ecc wrapping is enabled.
David Hildenbrand April 18, 2019, 10:31 a.m. UTC | #7
On 18.04.19 12:17, Christian Borntraeger wrote:
> 
> 
> On 18.04.19 11:13, David Hildenbrand wrote:
>> On 18.04.19 10:58, Christian Borntraeger wrote:
>>>
>>>
>>> On 18.04.19 09:54, David Hildenbrand wrote:
>>>> On 17.04.19 20:29, Christian Borntraeger wrote:
>>>>> Instead of adding a new machine option to disable/enable the keywrapping
>>>>> options of pckmo (like for AES and DEA) we can now use the CPU model to
>>>>> decide.
>>>>>
>>>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>>>> Reviewed-by: Collin Walling <walling@linux.ibm.com>
>>>>> ---
>>>>> v1->v2: - enable vsie
>>>>> 	- also check if the host has the pckmo functions
>>>>>  arch/s390/include/asm/kvm_host.h | 1 +
>>>>>  arch/s390/kvm/kvm-s390.c         | 7 +++++++
>>>>>  arch/s390/kvm/vsie.c             | 5 ++++-
>>>>>  3 files changed, 12 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
>>>>> index c47e22bba87fa..e224246ff93c6 100644
>>>>> --- a/arch/s390/include/asm/kvm_host.h
>>>>> +++ b/arch/s390/include/asm/kvm_host.h
>>>>> @@ -278,6 +278,7 @@ struct kvm_s390_sie_block {
>>>>>  #define ECD_HOSTREGMGMT	0x20000000
>>>>>  #define ECD_MEF		0x08000000
>>>>>  #define ECD_ETOKENF	0x02000000
>>>>> +#define ECD_ECC		0x00200000
>>>>>  	__u32	ecd;			/* 0x01c8 */
>>>>>  	__u8	reserved1cc[18];	/* 0x01cc */
>>>>>  	__u64	pp;			/* 0x01de */
>>>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>>>>> index 0dad61ccde3d6..9869d785677f1 100644
>>>>> --- a/arch/s390/kvm/kvm-s390.c
>>>>> +++ b/arch/s390/kvm/kvm-s390.c
>>>>> @@ -2933,6 +2933,13 @@ int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu)
>>>>>  		VCPU_EVENT(vcpu, 3, "AIV gisa format-%u enabled for cpu %03u",
>>>>>  			   vcpu->arch.sie_block->gd & 0x3, vcpu->vcpu_id);
>>>>>  	}
>>>>> +	/*
>>>>> +	 * if any of 32,33,34,40,41 is active in host AND guest,
>>>>> +	 * we enable pckmo for ecc
>>>>> +	 */
>>>>> +	if ((vcpu->kvm->arch.model.subfuncs.pckmo[4] & kvm_s390_available_subfunc.pckmo[4] & 0xe0) ||
>>>>> +	    (vcpu->kvm->arch.model.subfuncs.pckmo[5] & kvm_s390_available_subfunc.pckmo[5] & 0xc0))
>>>>
>>>> Maybe some helper like
>>>>
>>>> bool kvm_has_pckmo_subfunc(kvm, nr)
>>>> {
>>>> 	/* magic for one number */
>>>> }
>>>> ...
>>>>
>>>> if (kvm_has_pckmo_subfunc(kvm, 32) || kvm_has_pckmo_subfunc(kvm, 33))
>>>> ...
>>>>
>>>> then you can also get rid of the comment.
>>>
>>> Will give it a try.
>>>>
>>>>> +		vcpu->arch.sie_block->ecd |= ECD_ECC;
>>>>>  	vcpu->arch.sie_block->sdnxo = ((unsigned long) &vcpu->run->s.regs.sdnx)
>>>>>  					| SDNXC;
>>>>>  	vcpu->arch.sie_block->riccbd = (unsigned long) &vcpu->run->s.regs.riccb;
>>>>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
>>>>> index d62fa148558b9..c6983d962abfd 100644
>>>>> --- a/arch/s390/kvm/vsie.c
>>>>> +++ b/arch/s390/kvm/vsie.c
>>>>> @@ -288,6 +288,7 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>>>>  	const u32 crycb_addr = crycbd_o & 0x7ffffff8U;
>>>>>  	unsigned long *b1, *b2;
>>>>>  	u8 ecb3_flags;
>>>>> +	u32 ecd_flags;
>>>>>  	int apie_h;
>>>>>  	int key_msk = test_kvm_facility(vcpu->kvm, 76);
>>>>>  	int fmt_o = crycbd_o & CRYCB_FORMAT_MASK;
>>>>> @@ -320,7 +321,8 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>>>>  	/* we may only allow it if enabled for guest 2 */
>>>>>  	ecb3_flags = scb_o->ecb3 & vcpu->arch.sie_block->ecb3 &
>>>>>  		     (ECB3_AES | ECB3_DEA);
>>>>> -	if (!ecb3_flags)
>>>>> +	ecd_flags = scb_o->ecd & vcpu->arch.sie_block->ecd & ECD_ECC;
>>>>> +	if (!ecb3_flags && !ecd_flags)
>>>>>  		goto end;
>>>>
>>>> Just so I get it right, there are no *new* wrapping keys? Which wrapping
>>>> keys are used then?
>>>
>>> Yes, AES.
>>>
>>
>> Hmmmm, so if user space doesn't call KVM_S390_VM_CRYPTO_ENABLE_AES_KW,
>> the wrapping key is basically uninitialized (kvm_s390_vm_set_crypto()),
>> but will be used.
>>
>> I guess you should also check against kvm->arch.crypto.aes_kw before
>> turning the ecd bit on, just to be sure.
> 
> We should rather initialize the aes value when ecc wrapping is enabled.
> 

If you don't glue this to KVM_S390_VM_CRYPTO_ENABLE_AES_KW, you will
have to find another way to reset AES keys during resets.
Christian Borntraeger April 18, 2019, 10:40 a.m. UTC | #8
On 18.04.19 12:31, David Hildenbrand wrote:
> On 18.04.19 12:17, Christian Borntraeger wrote:
>>
>>
>> On 18.04.19 11:13, David Hildenbrand wrote:
>>> On 18.04.19 10:58, Christian Borntraeger wrote:
>>>>
>>>>
>>>> On 18.04.19 09:54, David Hildenbrand wrote:
>>>>> On 17.04.19 20:29, Christian Borntraeger wrote:
>>>>>> Instead of adding a new machine option to disable/enable the keywrapping
>>>>>> options of pckmo (like for AES and DEA) we can now use the CPU model to
>>>>>> decide.
>>>>>>
>>>>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>>>>> Reviewed-by: Collin Walling <walling@linux.ibm.com>
>>>>>> ---
>>>>>> v1->v2: - enable vsie
>>>>>> 	- also check if the host has the pckmo functions
>>>>>>  arch/s390/include/asm/kvm_host.h | 1 +
>>>>>>  arch/s390/kvm/kvm-s390.c         | 7 +++++++
>>>>>>  arch/s390/kvm/vsie.c             | 5 ++++-
>>>>>>  3 files changed, 12 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
>>>>>> index c47e22bba87fa..e224246ff93c6 100644
>>>>>> --- a/arch/s390/include/asm/kvm_host.h
>>>>>> +++ b/arch/s390/include/asm/kvm_host.h
>>>>>> @@ -278,6 +278,7 @@ struct kvm_s390_sie_block {
>>>>>>  #define ECD_HOSTREGMGMT	0x20000000
>>>>>>  #define ECD_MEF		0x08000000
>>>>>>  #define ECD_ETOKENF	0x02000000
>>>>>> +#define ECD_ECC		0x00200000
>>>>>>  	__u32	ecd;			/* 0x01c8 */
>>>>>>  	__u8	reserved1cc[18];	/* 0x01cc */
>>>>>>  	__u64	pp;			/* 0x01de */
>>>>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>>>>>> index 0dad61ccde3d6..9869d785677f1 100644
>>>>>> --- a/arch/s390/kvm/kvm-s390.c
>>>>>> +++ b/arch/s390/kvm/kvm-s390.c
>>>>>> @@ -2933,6 +2933,13 @@ int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu)
>>>>>>  		VCPU_EVENT(vcpu, 3, "AIV gisa format-%u enabled for cpu %03u",
>>>>>>  			   vcpu->arch.sie_block->gd & 0x3, vcpu->vcpu_id);
>>>>>>  	}
>>>>>> +	/*
>>>>>> +	 * if any of 32,33,34,40,41 is active in host AND guest,
>>>>>> +	 * we enable pckmo for ecc
>>>>>> +	 */
>>>>>> +	if ((vcpu->kvm->arch.model.subfuncs.pckmo[4] & kvm_s390_available_subfunc.pckmo[4] & 0xe0) ||
>>>>>> +	    (vcpu->kvm->arch.model.subfuncs.pckmo[5] & kvm_s390_available_subfunc.pckmo[5] & 0xc0))
>>>>>
>>>>> Maybe some helper like
>>>>>
>>>>> bool kvm_has_pckmo_subfunc(kvm, nr)
>>>>> {
>>>>> 	/* magic for one number */
>>>>> }
>>>>> ...
>>>>>
>>>>> if (kvm_has_pckmo_subfunc(kvm, 32) || kvm_has_pckmo_subfunc(kvm, 33))
>>>>> ...
>>>>>
>>>>> then you can also get rid of the comment.
>>>>
>>>> Will give it a try.
>>>>>
>>>>>> +		vcpu->arch.sie_block->ecd |= ECD_ECC;
>>>>>>  	vcpu->arch.sie_block->sdnxo = ((unsigned long) &vcpu->run->s.regs.sdnx)
>>>>>>  					| SDNXC;
>>>>>>  	vcpu->arch.sie_block->riccbd = (unsigned long) &vcpu->run->s.regs.riccb;
>>>>>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
>>>>>> index d62fa148558b9..c6983d962abfd 100644
>>>>>> --- a/arch/s390/kvm/vsie.c
>>>>>> +++ b/arch/s390/kvm/vsie.c
>>>>>> @@ -288,6 +288,7 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>>>>>  	const u32 crycb_addr = crycbd_o & 0x7ffffff8U;
>>>>>>  	unsigned long *b1, *b2;
>>>>>>  	u8 ecb3_flags;
>>>>>> +	u32 ecd_flags;
>>>>>>  	int apie_h;
>>>>>>  	int key_msk = test_kvm_facility(vcpu->kvm, 76);
>>>>>>  	int fmt_o = crycbd_o & CRYCB_FORMAT_MASK;
>>>>>> @@ -320,7 +321,8 @@ static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>>>>>  	/* we may only allow it if enabled for guest 2 */
>>>>>>  	ecb3_flags = scb_o->ecb3 & vcpu->arch.sie_block->ecb3 &
>>>>>>  		     (ECB3_AES | ECB3_DEA);
>>>>>> -	if (!ecb3_flags)
>>>>>> +	ecd_flags = scb_o->ecd & vcpu->arch.sie_block->ecd & ECD_ECC;
>>>>>> +	if (!ecb3_flags && !ecd_flags)
>>>>>>  		goto end;
>>>>>
>>>>> Just so I get it right, there are no *new* wrapping keys? Which wrapping
>>>>> keys are used then?
>>>>
>>>> Yes, AES.
>>>>
>>>
>>> Hmmmm, so if user space doesn't call KVM_S390_VM_CRYPTO_ENABLE_AES_KW,
>>> the wrapping key is basically uninitialized (kvm_s390_vm_set_crypto()),
>>> but will be used.
>>>
>>> I guess you should also check against kvm->arch.crypto.aes_kw before
>>> turning the ecd bit on, just to be sure.
>>
>> We should rather initialize the aes value when ecc wrapping is enabled.
>>
> 
> If you don't glue this to KVM_S390_VM_CRYPTO_ENABLE_AES_KW, you will
> have to find another way to reset AES keys during resets.

I think it is better to add 2 new attributes as we can have pckmo for ecc but
not for aes (e.g. ECB3_AES == 0 but ECD_ECC ==1).
Christian Borntraeger April 18, 2019, 10:46 a.m. UTC | #9
On 18.04.19 12:40, Christian Borntraeger wrote:
>>
>> If you don't glue this to KVM_S390_VM_CRYPTO_ENABLE_AES_KW, you will
>> have to find another way to reset AES keys during resets.
> 
> I think it is better to add 2 new attributes as we can have pckmo for ecc but
> not for aes (e.g. ECB3_AES == 0 but ECD_ECC ==1).


On the other hand: MSA9 requires MSA3 and MSA4 to be present. So simply checking for
kvm->arch.crypto.aes_kw ==1 as you suggested is very likely good enough.

Patch
diff mbox series

diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index c47e22bba87fa..e224246ff93c6 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -278,6 +278,7 @@  struct kvm_s390_sie_block {
 #define ECD_HOSTREGMGMT	0x20000000
 #define ECD_MEF		0x08000000
 #define ECD_ETOKENF	0x02000000
+#define ECD_ECC		0x00200000
 	__u32	ecd;			/* 0x01c8 */
 	__u8	reserved1cc[18];	/* 0x01cc */
 	__u64	pp;			/* 0x01de */
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 0dad61ccde3d6..9869d785677f1 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -2933,6 +2933,13 @@  int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu)
 		VCPU_EVENT(vcpu, 3, "AIV gisa format-%u enabled for cpu %03u",
 			   vcpu->arch.sie_block->gd & 0x3, vcpu->vcpu_id);
 	}
+	/*
+	 * if any of 32,33,34,40,41 is active in host AND guest,
+	 * we enable pckmo for ecc
+	 */
+	if ((vcpu->kvm->arch.model.subfuncs.pckmo[4] & kvm_s390_available_subfunc.pckmo[4] & 0xe0) ||
+	    (vcpu->kvm->arch.model.subfuncs.pckmo[5] & kvm_s390_available_subfunc.pckmo[5] & 0xc0))
+		vcpu->arch.sie_block->ecd |= ECD_ECC;
 	vcpu->arch.sie_block->sdnxo = ((unsigned long) &vcpu->run->s.regs.sdnx)
 					| SDNXC;
 	vcpu->arch.sie_block->riccbd = (unsigned long) &vcpu->run->s.regs.riccb;
diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
index d62fa148558b9..c6983d962abfd 100644
--- a/arch/s390/kvm/vsie.c
+++ b/arch/s390/kvm/vsie.c
@@ -288,6 +288,7 @@  static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
 	const u32 crycb_addr = crycbd_o & 0x7ffffff8U;
 	unsigned long *b1, *b2;
 	u8 ecb3_flags;
+	u32 ecd_flags;
 	int apie_h;
 	int key_msk = test_kvm_facility(vcpu->kvm, 76);
 	int fmt_o = crycbd_o & CRYCB_FORMAT_MASK;
@@ -320,7 +321,8 @@  static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
 	/* we may only allow it if enabled for guest 2 */
 	ecb3_flags = scb_o->ecb3 & vcpu->arch.sie_block->ecb3 &
 		     (ECB3_AES | ECB3_DEA);
-	if (!ecb3_flags)
+	ecd_flags = scb_o->ecd & vcpu->arch.sie_block->ecd & ECD_ECC;
+	if (!ecb3_flags && !ecd_flags)
 		goto end;
 
 	/* copy only the wrapping keys */
@@ -329,6 +331,7 @@  static int shadow_crycb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
 		return set_validity_icpt(scb_s, 0x0035U);
 
 	scb_s->ecb3 |= ecb3_flags;
+	scb_s->ecd |= ecd_flags;
 
 	/* xor both blocks in one run */
 	b1 = (unsigned long *) vsie_page->crycb.dea_wrapping_key_mask;