On 10/8/17 9:00 AM, Borislav Petkov wrote: > 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? There is a single security processor driver (ccp) which provides the complete functionality including PSP. But the driver should be able to work with multiple devices. e.g In my 2P EPYC configuration, security processor driver is used for the following devices 02:00.2 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device 1456 03:00.1 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device 1468 13:00.2 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device 1456 14:00.1 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device 1468 22:00.2 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device 1456 23:00.1 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device 1468 31:00.2 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device 1456 32:00.1 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device 1468 41:00.2 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device 1456 42:00.1 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device 1468 51:00.2 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device 1456 52:00.1 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device 1468 61:00.2 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device 1456 62:00.1 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device 1468 71:00.2 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device 1456 72:00.1 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device 1468 Some of the these devices support CCP only and some support both PSP and CCP. It's possible that multiple devices support the PSP functionality but only one of them is master which can be used for issuing SEV commands. There isn't a register which we can read to determine whether the device is master. We have to probe all the devices which supports the PSP to determine which one of them is a master. > 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. I was trying to follow the CCP model -- in which sp-dev.c simply forwards the call to ccp-dev.c which does the real work. Currently, sev-dev.c contains barebone common code. IMO, keeping all the PSP private functions and data structure outside the sp-dev.c/.h is right thing. Having said that, I am okay with moving all the PSP stuff in sp-dev.c but before we do that lets see what CCP maintainers say. Tom and Gary, comments please ? Additionally, I would like to highlight that if we decide to go with moving all the PSP functionality in sp-dev.c then we have to add #ifdef CONFIG_CRYPTO_DEV_SP_PSP because PSP feature depends on X86_66, whereas the sp-dev.c gets compiled for all architectures (including aarch64, i386 and x86_64). > > Or am I missing some obvious and important reason?
