On 2/24/21 9:44 PM, Steve Rutherford wrote:

On Wed, Feb 24, 2021 at 1:00 AM Nathan Tempelman [off-list ref] wrote:

quoted

@@ -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.

quoted

         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).

The sev_guest() function does not set sev->active, it only checks it. The 
sev_guest_init() function is where sev->active is set.

(Not an issue with this patch, but a broader issue) I believe
sev_guest lacks the necessary acquire/release barriers on sev->active,

The svm_mem_enc_op() takes the kvm lock and that is the only way into the 
sev_guest_init() function where sev->active is set.

Thanks,
Tom

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.

quoted

+
+       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.