Re: [PATCH 04/12] KVM: SVM: Add support for KVM_SEV_RECEIVE_START command

From: Steve Rutherford <srutherford@google.com>
To: Ashish Kalra <ashish.kalra@amd.com>
Cc: "Paolo Bonzini" <pbonzini@redhat.com>,
	"Thomas Gleixner" <tglx@linutronix.de>,
	"Ingo Molnar" <mingo@redhat.com>,
	"H. Peter Anvin" <hpa@zytor.com>,
	"Radim Krčmář" <rkrcmar@redhat.com>,
	"Joerg Roedel" <joro@8bytes.org>, "Borislav Petkov" <bp@suse.de>,
	"Tom Lendacky" <thomas.lendacky@amd.com>,
	"David Rientjes" <rientjes@google.com>, "X86 ML" <x86@kernel.org>,
	"KVM list" <kvm@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	"Brijesh Singh" <brijesh.singh@amd.com>
Subject: Re: [PATCH 04/12] KVM: SVM: Add support for KVM_SEV_RECEIVE_START command
Date: Wed, 11 Mar 2020 19:55:21 -0700	[thread overview]
Message-ID: <CABayD+ejsAt3QZGHGhkKh7GDd89R5QzMAbwJV6FW1t88Ne=MNg@mail.gmail.com> (raw)
In-Reply-To: <20200312003855.GA26448@ashkalra_ubuntu_server>

On Wed, Mar 11, 2020 at 5:39 PM Ashish Kalra <ashish.kalra@amd.com> wrote:
>
> But, ret will be the value returned by __sev_issue_cmd(), so why will it
> look like -ENOMEM ?
My bad, this is fine.
>
> >
> > > +       ret = __sev_issue_cmd(argp->sev_fd, SEV_CMD_RECEIVE_START, start,
> > > +                               error);
> > > +       if (ret)
> > > +               goto e_free;
> > > +
> > > +       /* Bind ASID to this guest */
> >
> > Ideally, set ret to another distinct value, since the error spaces for
> > these commands overlap, so you won't be sure which had the problem.
> > You also wouldn't be sure if one succeeded and the other failed vs
> > both failing.
>
> Both commands "may" return the same error code as set by sev_do_cmd(), but
> then we need that very specific error code, sev_do_cmd() can't return
> different error codes for each command it is issuing ?

I'll try to separate my comment into two levels: High level response,
and pragmatic response.

--- High level ---
At the end of the day, I want to be able to handle these errors in a
reasonable way. As often as possible, I'd like userspace to be able to
see a set of errors and know what to do in response. I find this
particularly important for migration, where you are mucking around
with a live VM with customer data you don't want to lose.

One red flag for me is when one pair of {errno, SEV error code}
corresponds to two distinct situations. For example, when, in another
patch in this series, {EFAULT, SUCCESS} could have corresponded to
either the command succeeding or the command never having run. Seems
like a pretty wide range of possibilities for a single error value.

I want to try to give the return codes scrutiny now, since we are
probably going to be stuck with maintaining them indefinitely, even if
there are mistakes.

--- Pragmatic ---
There's probably a strong argument that most situations like this
don't matter, since there's nothing you can do about an error except
kill the VM (or not continue migrating) anyway. I'm pretty open to
this argument. In particular, looking at SEV RECEIVE START, I think
you could throw away this attempt at creating a migration target, and
just make a new one (pretty much without consequence), so I think my
comment on this particular patch is moot. You can't cancel the SEND
START so you will be stuck working with this particular destination
host, but you can mint a new target VM via SEV RECEIVE START.

Looking at the earlier patches, older commands seem to have the same
ambiguity. The command SEV LAUNCH START also has identical errors that
could be sourced from either of two commands. Seems like we're already
committed to ambiguity being ok.

Given that I have no further comments on this particular patch:
Reviewed-by: Steve Rutherford <srutherford@google.com>

>
> >
> > > +       ret = sev_bind_asid(kvm, start->handle, error);
> > > +       if (ret)
> > > +               goto e_free;
> > > +
>
> Thanks,
> Ashish
>