>>> +
>>> + break;
>>> + }
>>> + case KVM_PV_DISABLE: {
>>> + r = -EINVAL;
>>> + if (!kvm_s390_pv_is_protected(kvm))
>>> + break;
>>> +
>>> + kvm_s390_cpus_from_pv(kvm, &cmd->rc, &cmd->rrc);
>>> + r = kvm_s390_pv_destroy_vm(kvm, &cmd->rc, &cmd->rrc);
>>> + if (!r)
>>> + kvm_s390_pv_dealloc_vm(kvm);
>>
>> Hm, if destroy fails, the CPUs would already have been removed.
>>
>> Is there a way to make kvm_s390_pv_destroy_vm() never fail? The return
>> value is always ignored except here ... which looks wrong.
>
> This should not fail. But if it does we should not free the memory that
> we donated to the ultravisor. We then do have a memory leak, but this is
> necessary to keep the integrity of the host.
> I will fix the other places to only call dealloc when destroy succeeded.
>
> Same for VCPU destroy. If that fails I should not free arch.pv.stor_base
> will fix.
A comment would be nice, documenting exactly that.
> */
>>> int kvm_s390_handle_wait(struct kvm_vcpu *vcpu);
>>> void kvm_s390_vcpu_wakeup(struct kvm_vcpu *vcpu);
>>> diff --git a/arch/s390/kvm/pv.c b/arch/s390/kvm/pv.c
>>> new file mode 100644
>>> index 000000000000..67ea9a18ed8f
>>> --- /dev/null
>>> +++ b/arch/s390/kvm/pv.c
>>> @@ -0,0 +1,256 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Hosting Secure Execution virtual machines
>>> + *
>>> + * Copyright IBM Corp. 2019
>>> + * Author(s): Janosch Frank <frankja@xxxxxxxxxxxxx>
>>
>> I'd assume you're an author as well at this point ;)
>
> I personally prefer to not have authors in files and after all
> I am just cleaning up so that Janosch can take care of QEMU.
> But I will at least fixup the Copyright year.
For me, that counts as co-author, but yeah, totally your decision :)
>>> + *rc = uvcb.header.rc;
>>> + *rrc = uvcb.header.rrc;
>>> +
>>> + if (ret && ret != -EAGAIN)
>>> + KVM_UV_EVENT(kvm, 3, "PROTVIRT VM UNPACK: failed addr %llx with rc %x rrc %x",
>>> + uvcb.gaddr, *rc, *rrc);
>>> + return ret;
>>> +}
>>> +
>>> +int kvm_s390_pv_unpack(struct kvm *kvm, unsigned long addr, unsigned long size,
>>> + unsigned long tweak, u16 *rc, u16 *rrc)
>>> +{
>>> + u64 tw[2] = {tweak, 0};
>>
>> I have no idea what tweaks are in this context. So I have to trust you
>> guys on the implementation, because I don't understand it.
>
> Its the crypto term. Basically similar idea like salt or nonce.
Understood. "offset"/"address" makes this clearer in this code -
although it will be used as a salt in HW.
>
>>
>> Especially, why can't we simply have
>>
>> s/tweak/tweak/
>
> ?
Ignore, it was part of the process of me figuring out what's going on in
this code :)
--
Thanks,
David / dhildenb
