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

From: Christian Borntraeger <borntraeger@de.ibm.com>
To: David Hildenbrand <david@redhat.com>,
	Janosch Frank <frankja@linux.vnet.ibm.com>
Cc: KVM <kvm@vger.kernel.org>, "Cornelia Huck" <cohuck@redhat.com>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Radim Krčmář" <rkrcmar@redhat.com>,
	"Collin Walling" <walling@linux.ibm.com>,
	"Jason J . Herne" <jjherne@linux.ibm.com>
Subject: Re: [PATCH v2 4/7] KVM: s390: enable MSA9 keywrapping functions depending on cpu model
Date: Thu, 18 Apr 2019 12:17:21 +0200	[thread overview]
Message-ID: <2a8aa30a-2712-7c8e-66ae-4fe05444cee0@de.ibm.com> (raw)
In-Reply-To: <60ae810e-f84e-873b-c731-b0a31f3cd9c1@redhat.com>

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.