Re: [RFC Part2 PATCH v3 02/26] crypto: ccp: Add Platform Security Processor (PSP) device support

From: Brijesh Singh
Date: Fri Sep 08 2017 - 09:54:35 EST




On 09/08/2017 03:40 AM, Borislav Petkov wrote:
On Thu, Sep 07, 2017 at 05:19:32PM -0500, Brijesh Singh wrote:
At high level, AMD-SP (AMD Secure Processor) (i.e CCP driver) will provide the
support for CCP, SEV and TEE FW commands.


+--- CCP
|
AMD-SP --|
| +--- SEV
| |
+---- PSP ---*
|
+---- TEE

I still don't see the need for such finegrained separation, though.
There's no "this is a separate compilation unit because... ". At least
the PSP branch could be a single driver without the interface.

For example, psp_request_sev_irq() is called only by sev_dev_init(). So
why is sev-dev a separate compilation unit? Is anything else going to
use the PSP interface?


I don't know anything about the TEE support hence I don't have very strong
reason for finegrained separation -- I just wanted to ensure that the SEV
enablement does not interfere with TEE support in the future.


If not, just put it all in a psp-dev file and that's it. We have a
gazillion config options and having two more just because, is not a good
reason. You can always carve it out later if there's real need. But if
the SEV thing can't function without the PSP thing, then you can just as
well put it inside it.

This way you can save yourself a bunch of exported functions and the
like.

Another example for not optimal design is psp_request_tee_irq() - it
doesn't really request an irq by calling into the IRQ core but simply
assigns a handler. Which looks to me like you're simulating an interface
where one is not really needed. Ditto for the sev_irq version, btw.


It's possible that both TEE and SEV share the same interrupt but their
interrupt handling approach could be totally different hence I tried to
abstract it.

I am making several assumption on TEE side without knowing in detail ;)

I can go with your recommendation -- we can always crave it out later once
the TEE support is visible.

-Brijesh