Re: [PATCH v3 09/37] KVM: s390: protvirt: Add initial vm and cpu lifecycle handling

[David Hildenbrand <david@redhat.com>]

>>> +
>>> +		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@linux.ibm.com>
>>
>> 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