Re: [Part2 PATCH v6 13/38] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support

[Borislav Petkov]

On Fri, Oct 27, 2017 at 05:59:23PM -0500, Brijesh Singh wrote:
> Yes it is typo. PEK_GEN wants FW to be in INIT state hence someone need
> to transition from UNINIT -> INIT.

Which, once you've done it once on driver init, is there.

> That's what I am doing except FACTORY_RESET.

Well, not really. Lemme pick a command at random...

PEK_CSR. For that, you do INIT -> PEK_CSR -> SHUTDOWN.

Doc says, platform needs to be in INIT or WORKING state. But nothing
says you should shut it down. Spec says, SHUTDOWN transitions platform
to UNINIT state. So when the next command comes in which needs the
platform to be in INIT state, you go and INIT it again. For no reason
*WHATSOEVER*!

I know, you're gonna say, but what if the next command needs a different
state than INIT. Well, *then* you transition it, in the command
function. When that function executes. But not before that and not in
preparation that *maybe* the next command will be it.

Now, if you did:

INIT once during driver init

PEK_CSR

(platform remains in INIT state)

<--- the next command here can execute directly if it is allowed in INIT
state.

Instead, the platform has been shutdown and you init it again. Do you
see now what I mean?

IOW, once you init the PSP master, you should keep it in the INIT state
- or the state in which most commands expect it to be and thus save
yourself all that unnecessary toggling. If a command needs it to be in a
different state, only *then* you transition it.

Instead, what you have now is that you call INIT and SHUTDOWN
around SEV_PEK_GEN, SEV_PDH_GEN, SEV_PEK_CSR, SEV_PEK_CERT_IMPORT,
SEV_PDH_CERT_EXPORT and for all those, the platform must be in INIT
(for some in WORKING state) but for all in INIT state and "The platform
remains be in the same state after completion." So the whole SHUTDOWN ->
INIT wankery in-between is a pure waste of electrons.

>  I see that we can do a small optimization -- since we already know
> the FW state hence we can avoid issuing PSP command when we know for
> sure that command will fail because we are not in correct state.

As I said before, you should do that regardless by recording the current
state of the PSP in variable so that you can save yourself the status
querying.

> If command needs INIT state and FW is not in INIT state then its safe to
> transition from UNINIT -> INIT. But if command needs UNINIT state and FW
> is in INIT state then its not safe to transition -- in those case we
> simply return EBUSY and let the user retry the command.

Whatever - that doesn't contradict what I'm proposing.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.