MIME-Version: 1.0
References: <20211102142331.3753798-1-pgonda@google.com> <20211102142331.3753798-2-pgonda@google.com>
 <YYqhT+Enba5xa4cO@google.com> <CAMkAt6oVySH-1g+EXKvxQ9vmBV8rwTLq=qfe2+VZC+c6vATL7w@mail.gmail.com>
 <45af7051-aab0-2b19-2904-64eceec93df5@amd.com>
In-Reply-To: <45af7051-aab0-2b19-2904-64eceec93df5@amd.com>
From:   Peter Gonda <pgonda@google.com>
Date:   Wed, 10 Nov 2021 10:29:54 -0700
Message-ID: <CAMkAt6pivYG=nV_EhP9Uj2gB8kBQB4dAnTcSeGd9249KuWzsyg@mail.gmail.com>
Subject: Re: [PATCH V3 1/4] crypto: ccp - Fix SEV_INIT error logging on init
To:     Tom Lendacky <Thomas.Lendacky@amd.com>
Cc:     Sean Christopherson <seanjc@google.com>,
        Marc Orr <marcorr@google.com>,
        David Rientjes <rientjes@google.com>,
        Brijesh Singh <brijesh.singh@amd.com>,
        Joerg Roedel <jroedel@suse.de>,
        Herbert Xu <herbert@gondor.apana.org.au>,
        John Allen <john.allen@amd.com>,
        "David S. Miller" <davem@davemloft.net>,
        Paolo Bonzini <pbonzini@redhat.com>,
        linux-crypto@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Nov 9, 2021 at 12:25 PM Tom Lendacky <thomas.lendacky@amd.com> wrote:
>
> On 11/9/21 10:46 AM, Peter Gonda wrote:
> > On Tue, Nov 9, 2021 at 9:27 AM Sean Christopherson <seanjc@google.com> wrote:
> >> On Tue, Nov 02, 2021, Peter Gonda wrote:
>
> ...
>
> >>
> >>          SEV: failed to INIT error 0, rc -16
> >>
> >> which a bit head scratching without looking at the code.  AFAICT, the PSP return
> >> codes aren't intrinsically hex, so printing error as a signed demical and thus
> >>
> >>          SEV: failed to INIT error -1, rc -16
> >>
> >> would be less confusing.
> >>
> >> And IMO requiring the caller to initialize error is will be neverending game of
> >> whack-a-mole.  E.g. sev_ioctl() fails to set "error" in the userspace structure,
> >> and literally every function exposed via include/linux/psp-sev.h has this same
> >> issue.  Case in point, the retry path fails to re-initialize "error" and will
> >> display stale information if the second sev_platform_init() fails without reaching
> >> the PSP.
> >
> > OK I can update __sev_platform_init_locked() to set error to -1. That
> > seems pretty reasonable. Tom, is that OK with you?
>
> Yup, I'm ok with using -1.
>
> >
>
> ...
>
> >
> > These comments seem fine to me. But I'll refrain from updating
> > anything here since this seems out-of-scope of this series. Happy to
> > discuss further and work on that if Tom is interested in those
> > refactors too.
>
> That's one of those things we've wanted to get around to improving but
> just haven't had the time. So, yes, if you wish to refactor the 'error'
> related area, that would be great.

OK so when I actually sat down to work on this. I realized this is
bigger than I thought. If we want to have error == -1 for all return
from psp-sev.h functions where the PSP isn't called yet there are a
lot of changes. For example if CONFIG_CRYPTO_DEV_SP_PSP is not defined
all these stubs need to be to set `*errror == -`, basically all these
functions need to be updated.

So to keep this series more targeted. I think I'll drop the error
here. And just have this patch print the rc value. If what I said
above seems reasonable I'll do those error refactors. Are people
envisioning something else for the error fixups?

>
> Thanks,
> Tom
>
> >