Re: [RFC] KVM: x86: Support KVM VMs sharing SEV context

From: Sean Christopherson <seanjc@google.com>
To: Ashish Kalra <ashish.kalra@amd.com>
Cc: Steve Rutherford <srutherford@google.com>,
	Tom Lendacky <thomas.lendacky@amd.com>,
	Nathan Tempelman <natet@google.com>,
	Paolo Bonzini <pbonzini@redhat.com>, X86 ML <x86@kernel.org>,
	KVM list <kvm@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	David Rientjes <rientjes@google.com>,
	Brijesh Singh <brijesh.singh@amd.com>
Subject: Re: [RFC] KVM: x86: Support KVM VMs sharing SEV context
Date: Tue, 9 Mar 2021 09:45:01 -0800	[thread overview]
Message-ID: <YEe0HWlwXyNvu9ps@google.com> (raw)
In-Reply-To: <20210305223647.GA2289@ashkalra_ubuntu_server>

On Fri, Mar 05, 2021, Ashish Kalra wrote:
> On Thu, Feb 25, 2021 at 10:49:00AM -0800, Steve Rutherford wrote:
> > On Thu, Feb 25, 2021 at 6:57 AM Tom Lendacky <thomas.lendacky@amd.com> wrote:
> > > >> +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.
> > Sorry, bad use of the english on my part: the "this" was referring to
> > svm_vm_copy_asid_to. Right now, you could only pass this sev_guest
> > check if you had already called sev_guest_init, which seems incorrect.
> > >
> > > >
> > > > (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.
> > There are a few places that check sev->active which don't have the kvm
> > lock, which is not problematic if we add in a few compiler barriers
> > (ala irqchip_split et al).

Eh, I don't see the point in taking on the complexity of barriers.  Ignoring the
vCPU behavior, the only existing call that isn't safe is svm_register_enc_region().
Fixing that is trivial and easy to understand.

As for the vCPU stuff, adding barriers will not make them safe.  E.g. a barrier
won't magically make init_vmcb() go back in time and set SVM_NESTED_CTL_SEV_ENABLE
if SEV is enabled after vCPUs are created.

> Probably, sev->active accesses can be made safe using READ_ONCE() &
> WRITE_ONCE().