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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 21.02.20 09:22, David Hildenbrand wrote:
>>>> +
>>>> +		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.


something like this on top:

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 738f7fefcaec..cad04e26dccf 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -2173,6 +2173,11 @@ static void kvm_s390_cpus_from_pv(struct kvm *kvm, u16 *rc, u16 *rrc)
        struct kvm_vcpu *vcpu;
        int i;
 
+       /*
+        * we ignore failures and try to destroy as many CPUs as possible.
+        * At the same time we must not free the assigned ressources when
+        * this fails, as the ultravisor has still access to that memory.
+        */
        kvm_for_each_vcpu(i, vcpu, kvm) {
                mutex_lock(&vcpu->mutex);
                kvm_s390_pv_destroy_cpu(vcpu, rc, rrc);
@@ -2221,6 +2226,7 @@ static int kvm_s390_handle_pv(struct kvm *kvm, struct kvm_pv_cmd *cmd)
                        kvm_s390_pv_dealloc_vm(kvm);
                        break;
                }
+               /* we never switch back to bsca from esca */
                r = kvm_s390_pv_create_vm(kvm, &cmd->rc, &cmd->rrc);
                if (r) {
                        kvm_s390_pv_dealloc_vm(kvm);
diff --git a/arch/s390/kvm/pv.c b/arch/s390/kvm/pv.c
index 6b581b5f8521..f93811a0a441 100644
--- a/arch/s390/kvm/pv.c
+++ b/arch/s390/kvm/pv.c
@@ -16,6 +16,7 @@
 #include <asm/mman.h>
 #include "kvm-s390.h"
 
+/* only free ressources when the destroy was successful */
 void kvm_s390_pv_dealloc_vm(struct kvm *kvm)
 {
        vfree(kvm->arch.pv.stor_var);
@@ -62,6 +63,7 @@ int kvm_s390_pv_alloc_vm(struct kvm *kvm)
        return -ENOMEM;
 }
 
+/* this should not fail, but if it does we must not free the donated memory */
 int kvm_s390_pv_destroy_vm(struct kvm *kvm, u16 *rc, u16 *rrc)
 {
        int cc;




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux