lkml.org 
[lkml]   [2021]   [Feb]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [RFC] KVM: x86: Support KVM VMs sharing SEV context
From
Date
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.
>

\
 
 \ /
  Last update: 2021-02-25 15:59    [W:0.246 / U:0.156 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site