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@xxxxxxxxxx>
>>>>>> Reviewed-by: Collin Walling <walling@xxxxxxxxxxxxx>
>>>>>> ---
>>>>>> 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).
