Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210224085915.28751-1-natet@google.com>
In-Reply-To: <20210224085915.28751-1-natet@google.com>
From:   Steve Rutherford <srutherford@google.com>
Date:   Wed, 24 Feb 2021 19:44:40 -0800
Message-ID: <CABayD+cZ1nRwuFWKHGh5a2sVXG5AEB_AyTGqZs_xVQLoWwmaSA@mail.gmail.com>
Subject: Re: [RFC] KVM: x86: Support KVM VMs sharing SEV context
To:     Nathan Tempelman <natet@google.com>
Cc:     Paolo Bonzini <pbonzini@redhat.com>,
        Tom Lendacky <thomas.lendacky@amd.com>,
        X86 ML <x86@kernel.org>, KVM list <kvm@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Sean Christopherson <seanjc@google.com>,
        David Rientjes <rientjes@google.com>,
        Brijesh Singh <brijesh.singh@amd.com>,
        Ashish Kalra <Ashish.Kalra@amd.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Feb 24, 2021 at 1:00 AM Nathan Tempelman <natet@google.com> wrote:
>
> @@ -1186,6 +1195,10 @@ int svm_register_enc_region(struct kvm *kvm,
>         if (!sev_guest(kvm))
>                 return -ENOTTY;
>
> +       /* If kvm is mirroring encryption context it isn't responsible for it */
> +       if (is_mirroring_enc_context(kvm))
> +               return -ENOTTY;
> +

Is this necessary? Same for unregister. When we looked at
sev_pin_memory, I believe we concluded that double pinning was safe.
>
>         if (range->addr > ULONG_MAX || range->size > ULONG_MAX)
>                 return -EINVAL;
>
> @@ -1252,6 +1265,10 @@ int svm_unregister_enc_region(struct kvm *kvm,
>         struct enc_region *region;
>         int ret;
>
> +       /* If kvm is mirroring encryption context it isn't responsible for it */
> +       if (is_mirroring_enc_context(kvm))
> +               return -ENOTTY;
> +
>         mutex_lock(&kvm->lock);
>
>         if (!sev_guest(kvm)) {
> @@ -1282,6 +1299,65 @@ int svm_unregister_enc_region(struct kvm *kvm,
>         return ret;
>  }
>
> +int svm_vm_copy_asid_to(struct kvm *kvm, unsigned int mirror_kvm_fd)
> +{
> +       struct file *mirror_kvm_file;
> +       struct kvm *mirror_kvm;
> +       struct kvm_sev_info *mirror_kvm_sev;
> +       unsigned int asid;
> +       int ret;
> +
> +       if (!sev_guest(kvm))
> +               return -ENOTTY;

You definitely don't want this: this is the function that turns the vm
into an SEV guest (marks SEV as active).

(Not an issue with this patch, but a broader issue) I believe
sev_guest lacks the necessary acquire/release barriers on sev->active,
since it's called without the kvm lock. I mean, it's x86, so the only
one that's going to hose you is the compiler for this type of access.
There should be an smp_rmb() after the access in sev_guest and an
smp_wmb() before the access in SEV_GUEST_INIT and here.
>
> +
> +       mutex_lock(&kvm->lock);
> +
> +       /* Mirrors of mirrors should work, but let's not get silly */
> +       if (is_mirroring_enc_context(kvm)) {
> +               ret = -ENOTTY;
> +               goto failed;
> +       }
> +
> +       mirror_kvm_file = fget(mirror_kvm_fd);
> +       if (!kvm_is_kvm(mirror_kvm_file)) {
> +               ret = -EBADF;
> +               goto failed;
> +       }
> +
> +       mirror_kvm = mirror_kvm_file->private_data;
> +
> +       if (mirror_kvm == kvm || is_mirroring_enc_context(mirror_kvm)) {
Just check if the source is an sev_guest and that the destination is
not an sev_guest.

I reviewed earlier incarnations of this, and think the high-level idea
is sound. I'd like to see kvm-selftests for this patch, and plan on
collaborating with AMD to help make those happen.