Messages in this thread | | | Subject | Re: [RFC] KVM: x86: Support KVM VMs sharing SEV context | From | Tom Lendacky <> | Date | Thu, 25 Feb 2021 08:57:33 -0600 |
| |
On 2/24/21 9:44 PM, Steve Rutherford wrote: > 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).
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. >> >> + >> + 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. >
| |