Re: [Part2 PATCH v5.1 12.1/31] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support

[Borislav Petkov <bp@suse.de>]

On Sun, Oct 08, 2017 at 08:30:47AM -0500, Brijesh Singh wrote:
> During the device probe, sev_ops_init() will be called for every device
> instance which claims to support the SEV.  One of the device will be
> 'master' but we don't the master until we probe all the instances. Hence
> the probe for all SEV devices must return success.

I still am wondering whether that design with multiple devices - master
and non-master devices is optimal. Why isn't the security processor a
single driver which provides the whole functionality, PSP including? Why
do you need all that register and unregister glue and get_master bla if
you can simply put the whole thing in a single module?

And the fact that you need a global variable to mark that you've
registered the misc device should already tell you that something's
not optimal. Because if you had a single driver, it will go, detect
the whole functionality, initialize it, register services and be done
with it. No registering of devices, no finding of masters, no global
variables, no unnecessary glue.

IOW, in this diagram:

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

why isn't the whole PSP functionality part of drivers/crypto/ccp/sp-dev.c ?

That driver is almost barebones minimal thing. You can very well add the
PSP/SEV stuff in there and if there's an *actual* reason to carve pieces
out, only then to do so, not to split it now unnecessarily and make your
life complicated for no reason.

Or am I missing some obvious and important reason?

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--