MIME-Version: 1.0
References: <20211117163809.1441845-1-pbonzini@redhat.com> <20211117163809.1441845-5-pbonzini@redhat.com>
In-Reply-To: <20211117163809.1441845-5-pbonzini@redhat.com>
From:   Peter Gonda <pgonda@google.com>
Date:   Wed, 17 Nov 2021 10:46:48 -0700
Message-ID: <CAMkAt6odbAGZ-LgK7yefnNRgoAAs3ekvR2_sZpjTiv_6mfwRKg@mail.gmail.com>
Subject: Re: [PATCH 4/4] KVM: SEV: Do COPY_ENC_CONTEXT_FROM with both VMs locked
To:     Paolo Bonzini <pbonzini@redhat.com>
Cc:     linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
        seanjc@google.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Nov 17, 2021 at 9:38 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> Now that we have a facility to lock two VMs with deadlock
> protection, use it for the creation of mirror VMs as well.  One of
> COPY_ENC_CONTEXT_FROM(dst, src) and COPY_ENC_CONTEXT_FROM(src, dst)
> would always fail, so the combination is nonsensical and it is okay to
> return -EBUSY if it is attempted.
>
> This sidesteps the question of what happens if a VM is
> MOVE_ENC_CONTEXT_FROM'd at the same time as it is
> COPY_ENC_CONTEXT_FROM'd: the locking prevents that from
> happening.
>
> Cc: Peter Gonda <pgonda@google.com>
> Cc: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Acked-by: Peter Gonda <pgonda@google.com>

> ---
>  arch/x86/kvm/svm/sev.c | 68 ++++++++++++++++--------------------------
>  1 file changed, 26 insertions(+), 42 deletions(-)
>
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index f9256ba269e6..47d54df7675c 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -1548,6 +1548,9 @@ static int sev_lock_two_vms(struct kvm *dst_kvm, struct kvm *src_kvm)
>         struct kvm_sev_info *dst_sev = &to_kvm_svm(dst_kvm)->sev_info;
>         struct kvm_sev_info *src_sev = &to_kvm_svm(src_kvm)->sev_info;
>
> +       if (dst_kvm == src_kvm)
> +               return -EINVAL;
> +

Worth adding a migrate/mirror from self fails tests in
test_sev_(migrate|mirror)_parameters()? I guess it's already covered
by "the source cannot be SEV enabled" test cases.

>         /*
>          * Bail if these VMs are already involved in a migration to avoid
>          * deadlock between two VMs trying to migrate to/from each other.
> @@ -1951,76 +1954,57 @@ int svm_vm_copy_asid_from(struct kvm *kvm, unsigned int source_fd)
>  {
>         struct file *source_kvm_file;
>         struct kvm *source_kvm;
> -       struct kvm_sev_info source_sev, *mirror_sev;
> +       struct kvm_sev_info *source_sev, *mirror_sev;
>         int ret;
>
>         source_kvm_file = fget(source_fd);
>         if (!file_is_kvm(source_kvm_file)) {
>                 ret = -EBADF;
> -               goto e_source_put;
> +               goto e_source_fput;
>         }
>
>         source_kvm = source_kvm_file->private_data;
> -       mutex_lock(&source_kvm->lock);
> -
> -       if (!sev_guest(source_kvm)) {
> -               ret = -EINVAL;
> -               goto e_source_unlock;
> -       }
> +       ret = sev_lock_two_vms(kvm, source_kvm);
> +       if (ret)
> +               goto e_source_fput;
>
> -       /* Mirrors of mirrors should work, but let's not get silly */
> -       if (is_mirroring_enc_context(source_kvm) || source_kvm == kvm) {
> +       /*
> +        * Mirrors of mirrors should work, but let's not get silly.  Also
> +        * disallow out-of-band SEV/SEV-ES init if the target is already an
> +        * SEV guest, or if vCPUs have been created.  KVM relies on vCPUs being
> +        * created after SEV/SEV-ES initialization, e.g. to init intercepts.
> +        */
> +       if (sev_guest(kvm) || !sev_guest(source_kvm) ||
> +           is_mirroring_enc_context(source_kvm) || kvm->created_vcpus) {
>                 ret = -EINVAL;
> -               goto e_source_unlock;
> +               goto e_unlock;
>         }
>
> -       memcpy(&source_sev, &to_kvm_svm(source_kvm)->sev_info,
> -              sizeof(source_sev));
> -
>         /*
>          * The mirror kvm holds an enc_context_owner ref so its asid can't
>          * disappear until we're done with it
>          */
> +       source_sev = &to_kvm_svm(source_kvm)->sev_info;
>         kvm_get_kvm(source_kvm);
>
> -       fput(source_kvm_file);
> -       mutex_unlock(&source_kvm->lock);
> -       mutex_lock(&kvm->lock);
> -
> -       /*
> -        * Disallow out-of-band SEV/SEV-ES init if the target is already an
> -        * SEV guest, or if vCPUs have been created.  KVM relies on vCPUs being
> -        * created after SEV/SEV-ES initialization, e.g. to init intercepts.
> -        */
> -       if (sev_guest(kvm) || kvm->created_vcpus) {
> -               ret = -EINVAL;
> -               goto e_mirror_unlock;
> -       }
> -
>         /* Set enc_context_owner and copy its encryption context over */
>         mirror_sev = &to_kvm_svm(kvm)->sev_info;
>         mirror_sev->enc_context_owner = source_kvm;
>         mirror_sev->active = true;
> -       mirror_sev->asid = source_sev.asid;
> -       mirror_sev->fd = source_sev.fd;
> -       mirror_sev->es_active = source_sev.es_active;
> -       mirror_sev->handle = source_sev.handle;
> +       mirror_sev->asid = source_sev->asid;
> +       mirror_sev->fd = source_sev->fd;
> +       mirror_sev->es_active = source_sev->es_active;
> +       mirror_sev->handle = source_sev->handle;
> +       ret = 0;
>         /*
>          * Do not copy ap_jump_table. Since the mirror does not share the same
>          * KVM contexts as the original, and they may have different
>          * memory-views.
>          */
>
> -       mutex_unlock(&kvm->lock);
> -       return 0;
> -
> -e_mirror_unlock:
> -       mutex_unlock(&kvm->lock);
> -       kvm_put_kvm(source_kvm);
> -       return ret;
> -e_source_unlock:
> -       mutex_unlock(&source_kvm->lock);
> -e_source_put:
> +e_unlock:
> +       sev_unlock_two_vms(kvm, source_kvm);
> +e_source_fput:
>         if (source_kvm_file)
>                 fput(source_kvm_file);
>         return ret;
> --
> 2.27.0
>