diff mbox series

[4/7] KVM: s390: enable MSA9 keywrapping functions depending on cpu model

Message ID 20190417152842.71730-5-borntraeger@de.ibm.com (mailing list archive)
State New, archived
Headers show
Series KVM: s390: new guest facilities | expand

Commit Message

Christian Borntraeger April 17, 2019, 3:28 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>
---
 arch/s390/include/asm/kvm_host.h | 1 +
 arch/s390/kvm/kvm-s390.c         | 4 ++++
 2 files changed, 5 insertions(+)

Comments

David Hildenbrand April 17, 2019, 3:38 p.m. UTC | #1 
On 17.04.19 17:28, 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>
> ---
>  arch/s390/include/asm/kvm_host.h | 1 +
>  arch/s390/kvm/kvm-s390.c         | 4 ++++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index c47e22bba87f..e224246ff93c 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 0dad61ccde3d..ff2444d935fd 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -2933,6 +2933,10 @@ 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, enable pckmo for ecc */
> +	if ((vcpu->kvm->arch.model.subfuncs.pckmo[4] & 0xe0) ||
> +	    (vcpu->kvm->arch.model.subfuncs.pckmo[5] & 0xc0))
> +		vcpu->arch.sie_block->ecd |= ECD_ECC;

I see, that's why you sent the patch to remember this.

vsie?

If I am not wrong, you should allow to set ECD_ECC there similarly, if
the configured cpu model has these facilitites.

>  	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;
>
David Hildenbrand April 17, 2019, 3:47 p.m. UTC | #2 
On 17.04.19 17:28, 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>
> ---
>  arch/s390/include/asm/kvm_host.h | 1 +
>  arch/s390/kvm/kvm-s390.c         | 4 ++++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index c47e22bba87f..e224246ff93c 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 0dad61ccde3d..ff2444d935fd 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -2933,6 +2933,10 @@ 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, enable pckmo for ecc */
> +	if ((vcpu->kvm->arch.model.subfuncs.pckmo[4] & 0xe0) ||
> +	    (vcpu->kvm->arch.model.subfuncs.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;
> 
Just noting here that user space can set pckmo and friends even though
HW does not support it (if I remember correctly), resulting in ECD_ECC
being able to be set from user space although not supported. Is that
sane? Shouldn't we somehow check if the HW actually supports it?

(features should only be used if the indication is on, not because it
"might" work)
Christian Borntraeger April 17, 2019, 3:48 p.m. UTC | #3 
On 17.04.19 17:38, David Hildenbrand wrote:
> On 17.04.19 17:28, 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>
>> ---
>>  arch/s390/include/asm/kvm_host.h | 1 +
>>  arch/s390/kvm/kvm-s390.c         | 4 ++++
>>  2 files changed, 5 insertions(+)
>>
>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
>> index c47e22bba87f..e224246ff93c 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 0dad61ccde3d..ff2444d935fd 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -2933,6 +2933,10 @@ 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, enable pckmo for ecc */
>> +	if ((vcpu->kvm->arch.model.subfuncs.pckmo[4] & 0xe0) ||
>> +	    (vcpu->kvm->arch.model.subfuncs.pckmo[5] & 0xc0))
>> +		vcpu->arch.sie_block->ecd |= ECD_ECC;
> 
> I see, that's why you sent the patch to remember this.
> 
> vsie?

i split this out because this is different from what we did with KVM_S390_VM_CRYPTO_ENABLE_AES_KW
and friends. (we had no cpu model back then).

But you are right. I will look into vsie, should be straightforward.
> 
> If I am not wrong, you should allow to set ECD_ECC there similarly, if
> the configured cpu model has these facilitites.
> 
>>  	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;
>>
> 
>
Christian Borntraeger April 17, 2019, 3:50 p.m. UTC | #4 
On 17.04.19 17:47, David Hildenbrand wrote:
> On 17.04.19 17:28, 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>
>> ---
>>  arch/s390/include/asm/kvm_host.h | 1 +
>>  arch/s390/kvm/kvm-s390.c         | 4 ++++
>>  2 files changed, 5 insertions(+)
>>
>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
>> index c47e22bba87f..e224246ff93c 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 0dad61ccde3d..ff2444d935fd 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -2933,6 +2933,10 @@ 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, enable pckmo for ecc */
>> +	if ((vcpu->kvm->arch.model.subfuncs.pckmo[4] & 0xe0) ||
>> +	    (vcpu->kvm->arch.model.subfuncs.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;
>>
> Just noting here that user space can set pckmo and friends even though
> HW does not support it (if I remember correctly), resulting in ECD_ECC
> being able to be set from user space although not supported. Is that
> sane? Shouldn't we somehow check if the HW actually supports it?
> 
> (features should only be used if the indication is on, not because it
> "might" work)

For those machines ECD_ECC will be ignored, but yes, adding an additional check
that we have these in the real machine might be safer.
Christian Borntraeger April 17, 2019, 4:16 p.m. UTC | #5 
On 17.04.19 17:47, David Hildenbrand wrote:
> On 17.04.19 17:28, 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>
>> ---
>>  arch/s390/include/asm/kvm_host.h | 1 +
>>  arch/s390/kvm/kvm-s390.c         | 4 ++++
>>  2 files changed, 5 insertions(+)
>>
>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
>> index c47e22bba87f..e224246ff93c 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 0dad61ccde3d..ff2444d935fd 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -2933,6 +2933,10 @@ 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, enable pckmo for ecc */
>> +	if ((vcpu->kvm->arch.model.subfuncs.pckmo[4] & 0xe0) ||
>> +	    (vcpu->kvm->arch.model.subfuncs.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;
>>
> Just noting here that user space can set pckmo and friends even though
> HW does not support it (if I remember correctly), resulting in ECD_ECC
> being able to be set from user space although not supported. Is that
> sane? Shouldn't we somehow check if the HW actually supports it?

untested patch, something like this on top

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 7dae78ccefc6..dadd1105b5a4 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -2985,9 +2985,14 @@ 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, enable pckmo for ecc */
-       if ((vcpu->kvm->arch.model.subfuncs.pckmo[4] & 0xe0) ||
-           (vcpu->kvm->arch.model.subfuncs.pckmo[5] & 0xc0))
+       /*
+        * 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] & 0xe0) ||
+            (vcpu->kvm->arch.model.subfuncs.pckmo[5] & 0xc0)) &&
+           ((kvm_s390_available_subfunc.pckmo[4] & 0xe0) ||
+            (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;


> 
> (features should only be used if the indication is on, not because it
> "might" work)
>
Christian Borntraeger April 17, 2019, 4:25 p.m. UTC | #6 
On 17.04.19 17:48, Christian Borntraeger wrote:
> 
> 
> On 17.04.19 17:38, David Hildenbrand wrote:
>> On 17.04.19 17:28, 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>
>>> ---
>>>  arch/s390/include/asm/kvm_host.h | 1 +
>>>  arch/s390/kvm/kvm-s390.c         | 4 ++++
>>>  2 files changed, 5 insertions(+)
>>>
>>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
>>> index c47e22bba87f..e224246ff93c 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 0dad61ccde3d..ff2444d935fd 100644
>>> --- a/arch/s390/kvm/kvm-s390.c
>>> +++ b/arch/s390/kvm/kvm-s390.c
>>> @@ -2933,6 +2933,10 @@ 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, enable pckmo for ecc */
>>> +	if ((vcpu->kvm->arch.model.subfuncs.pckmo[4] & 0xe0) ||
>>> +	    (vcpu->kvm->arch.model.subfuncs.pckmo[5] & 0xc0))
>>> +		vcpu->arch.sie_block->ecd |= ECD_ECC;
>>
>> I see, that's why you sent the patch to remember this.
>>
>> vsie?
> 
> i split this out because this is different from what we did with KVM_S390_VM_CRYPTO_ENABLE_AES_KW
> and friends. (we had no cpu model back then).
> 
> But you are right. I will look into vsie, should be straightforward.


untested but something like this on top.

diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
index d62fa148558b..d0510d8c77e0 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;
+       u8 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 && !ecbflags)
                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;
diff mbox series

Patch

diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index c47e22bba87f..e224246ff93c 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 0dad61ccde3d..ff2444d935fd 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -2933,6 +2933,10 @@  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, enable pckmo for ecc */
+	if ((vcpu->kvm->arch.model.subfuncs.pckmo[4] & 0xe0) ||
+	    (vcpu->kvm->arch.model.subfuncs.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;