| Message ID | 20171007184049.jrbxebb4jlciu3hj@pd.tnic (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
On 10/7/17 1:40 PM, Borislav Petkov wrote: ... > A bunch of fixes ontop: > > * sev_fops_registered is superfluous if you can use psp->has_sev_fops I am okay with all your fixes except this one. I will add my comment below. ... > static int sev_ops_init(struct psp_device *psp) > { > struct miscdevice *misc = &psp->sev_misc; > - int ret = 0; > + int ret; > > /* > - * SEV feature support can be detected on the multiple devices but the > - * SEV FW commands must be issued on the master. During probe time we > - * do not know the master hence we create /dev/sev on the first device > - * probe. sev_handle_cmd() finds the right master device to when issuing > - * the command to the firmware. > + * SEV feature support can be detected on multiple devices but the SEV > + * FW commands must be issued on the master. During probe, we do not > + * know the master hence we create /dev/sev on the first device probe. > + * sev_do_cmd() finds the right master device to which to issue the > + * command to the firmware. > */ > - if (!sev_fops_registered) { > - misc->minor = MISC_DYNAMIC_MINOR; > - misc->name = DEVICE_NAME; > - misc->fops = &sev_fops; > - > - ret = misc_register(misc); > - if (!ret) { > - sev_fops_registered = true; > - psp->has_sev_fops = true; > - init_waitqueue_head(&psp->sev_int_queue); > - dev_info(psp->dev, "registered SEV device\n"); > - } > + if (psp->has_sev_fops) > + return 0; > + This will always be false. The struct psp_device is used for storing per-device instance. > + misc->minor = MISC_DYNAMIC_MINOR; > + misc->name = DEVICE_NAME; > + misc->fops = &sev_fops; > + > + ret = misc_register(misc); > + if (!ret) { > + psp->has_sev_fops = true; > + init_waitqueue_head(&psp->sev_int_queue); > + dev_info(psp->dev, "registered SEV device\n"); > } 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. With your changes, the first device instance will able to create /dev/sev node but all other instances will fail hence the probe routine for other instances will also fail. Basically we need some variable which is outside the per-device structure so that we don't end up creating multiple /dev/sev nodes. If needed, I think we can remove 'has_sev_fops' variable from struct psp_device if we decide to dynamic alloc 'struct miscdevice sev_misc' in struct psp_device. The 'has_sev_fops' is mainly used in sev_exit(). If we decide to dynamic alloc sev_misc then in sev_exit() we can use psp->sev_misc != NULL instead of psp->has_sev_ops. > > return ret; > diff --git a/drivers/crypto/ccp/psp-dev.h b/drivers/crypto/ccp/psp-dev.h > index 6e8f83b41521..60a33f5ff79f 100644 > --- a/drivers/crypto/ccp/psp-dev.h > +++ b/drivers/crypto/ccp/psp-dev.h > @@ -36,7 +36,7 @@ > #define PSP_CMDBUFF_ADDR_HI PSP_C2PMSG(57) > #define PSP_FEATURE_REG PSP_C2PMSG(63) > > -#define PSP_P2CMSG(_num) (_num << 2) > +#define PSP_P2CMSG(_num) ((_num) << 2) > #define PSP_CMD_COMPLETE_REG 1 > #define PSP_CMD_COMPLETE PSP_P2CMSG(PSP_CMD_COMPLETE_REG) > > diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h > index 2b334fd853c9..10b843cce75f 100644 > --- a/include/linux/psp-sev.h > +++ b/include/linux/psp-sev.h > @@ -512,8 +512,7 @@ struct sev_data_dbg { > u32 len; /* In */ > } __packed; > > -#if defined(CONFIG_CRYPTO_DEV_SP_PSP) > - > +#ifdef CONFIG_CRYPTO_DEV_SP_PSP > /** > * sev_platform_init - perform SEV INIT command > * > @@ -562,9 +561,9 @@ int sev_platform_status(struct sev_data_status *status, int *error); > * sev_issue_cmd_external_user - issue SEV command by other driver with a file > * handle. > * > - * The function can be used by other drivers to issue a SEV command on > - * behalf by userspace. The caller must pass a valid SEV file descriptor > - * so that we know that caller has access to SEV device. > + * This function can be used by other drivers to issue a SEV command on > + * behalf of userspace. The caller must pass a valid SEV file descriptor > + * so that we know that it has access to SEV device. > * > * @filep - SEV device file pointer > * @cmd - command to issue >
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?
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?
On Sun, Oct 08, 2017 at 07:11:04PM -0500, Brijesh Singh wrote: > 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 So we have a lot of drivers which claim more than one PCI device. It is not mandatory to have a driver per PCI device. Actually, the decisive argument is what is the easiest way to manage those devices and what makes the communication between them the easiest. > 03:00.1 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device > 1468 > 13:00.2 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device > 1456 Btw, what do those PCI functions each do? Public PPR doesn't have them documented. > 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. Sure, and if you manage all the devices in a single driver, you can simply keep them all in a linked list or in an array and iterating over them is trivial. Because right now you have 1. sp-pci.c::sp_pci_probe() execute upon the PCI device detection 2. at some point, it does sp-dev.c::sp_init() which decides whether CCP or PSP 3. If PSP, it calls pcp-dev.c::psp_dev_init() which gets that sp->dev_vdata->psp_vdata which is nothing more than a simple offset 0x10500 which is where the PSP io regs are. For example, if this offset is hardcoded, why are we even passing that vdata? Just set psp->io_regs = 0x10500. No need for all that passing of structs around. 4. and finally, after that *whole* init has been done, you get to do ->set_psp_master_device(sp); Or, you can save yourself all that jumping through hoops, merge sp-pci.c and sp-dev.c into a single sp.c and put *everything* sp-related into it. And then do the whole work of picking hw apart, detection and configuration in sp_pci_probe() and have helper functions preparing and configuring the device. At the end, it adds it to the list of devices sp.c manages and done. You actually have that list already: static LIST_HEAD(sp_units); in sp-dev.c. You don't need the set_master thing either - you simply set the sp_dev_master pointer inside sp.c sp_init() can then go and you can replace it with its function body, deciding whether it is a CCP or PSP and then call the respective function which is also in sp.c or ccp-dev.c And then all those separate compilation units and the interfaces between them disappear - you have only the calls into the PSP and that's it. Btw, the CCP thing could remain separate initially, I guess, with all that ccp-* stuff in there. > 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. And you don't really need that - you can do the real work directly in sp-dev.c or sp.c or whatever. > 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. By this model probably, but it causes all that init and registration jump-through-hoops for no real reason. It is basically wasting cycles and energy. I'm all for splitting if it makes sense. But right now I don't see much sense in this - it is basically a bunch of small compilation units calling each other. And they could be merged into a single sp.c which does it all in one go, without a lot of blabla. > 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). That's fine. You can build it on 32-bit but add to the init function if (IS_ENABLED(CONFIG_X86_32)) return -ENODEV; and be done with it. No need for the ifdeffery.
On 10/09/2017 10:21 AM, Borislav Petkov wrote: ... > >> 03:00.1 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device >> 1468 >> 13:00.2 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device >> 1456 > > Btw, what do those PCI functions each do? Public PPR doesn't have them > documented. Looking at the pci_device_id table (sp-pci.c), the devices id 0x1468 provides the support CCP support directly on the x86-side and device id 0x1456 provides the support for both CCP and PSP features through the AMD Secure Processor (AMD-SP). > > Sure, and if you manage all the devices in a single driver, you can > simply keep them all in a linked list or in an array and iterating over > them is trivial. > > Because right now you have > > 1. sp-pci.c::sp_pci_probe() execute upon the PCI device detection > > 2. at some point, it does sp-dev.c::sp_init() which decides whether CCP or PSP > > 3. If PSP, it calls pcp-dev.c::psp_dev_init() which gets that > sp->dev_vdata->psp_vdata which is nothing more than a simple offset > 0x10500 which is where the PSP io regs are. For example, if this offset > is hardcoded, why are we even passing that vdata? Just set psp->io_regs = > 0x10500. No need for all that passing of structs around. > > 4. and finally, after that *whole* init has been done, you get to do > ->set_psp_master_device(sp); > > Or, you can save yourself all that jumping through hoops, merge sp-pci.c > and sp-dev.c into a single sp.c and put *everything* sp-related into > it. And then do the whole work of picking hw apart, detection and > configuration in sp_pci_probe() and have helper functions preparing and > configuring the device. > > At the end, it adds it to the list of devices sp.c manages and done. You > actually have that list already: > > static LIST_HEAD(sp_units); > > in sp-dev.c. > > You don't need the set_master thing either - you simply set the > sp_dev_master pointer inside sp.c > I was trying to avoid putting PSP/SEV specific changes in sp-dev.* files. But if sp.c approach is acceptable to the maintainer then I can work towards merging sp-dev.c and sp-pci.c into sp.c and then add the PSP/SEV support. > sp_init() can then go and you can replace it with its function body, > deciding whether it is a CCP or PSP and then call the respective > function which is also in sp.c or ccp-dev.c > > And then all those separate compilation units and the interfaces between > them disappear - you have only the calls into the PSP and that's it. > > Btw, the CCP thing could remain separate initially, I guess, with all > that ccp-* stuff in there. > Yep, if we decide to go with your recommended approach then we should leave the CCP as-is for now. >> 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. > > And you don't really need that - you can do the real work directly in > sp-dev.c or sp.c or whatever. > >> 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. > > By this model probably, but it causes all that init and registration > jump-through-hoops for no real reason. It is basically wasting cycles > and energy. > > I'm all for splitting if it makes sense. But right now I don't see much > sense in this - it is basically a bunch of small compilation units > calling each other. And they could be merged into a single sp.c which > does it all in one go, without a lot of blabla. >> 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). > > That's fine. You can build it on 32-bit but add to the init function > > if (IS_ENABLED(CONFIG_X86_32)) > return -ENODEV; > > and be done with it. No need for the ifdeffery. > OK, i will use IS_ENABLED where applicable.
On 10/10/2017 10:00 AM, Brijesh Singh wrote: > > > On 10/09/2017 10:21 AM, Borislav Petkov wrote: > ... > >> >>> 03:00.1 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device >>> 1468 >>> 13:00.2 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device >>> 1456 >> >> Btw, what do those PCI functions each do? Public PPR doesn't have them >> documented. > > > Looking at the pci_device_id table (sp-pci.c), the devices id 0x1468 > provides the support CCP support directly on the x86-side and device id > 0x1456 provides the support for both CCP and PSP features through the AMD > Secure Processor (AMD-SP). > > >> >> Sure, and if you manage all the devices in a single driver, you can >> simply keep them all in a linked list or in an array and iterating over >> them is trivial. >> >> Because right now you have >> >> 1. sp-pci.c::sp_pci_probe() execute upon the PCI device detection >> >> 2. at some point, it does sp-dev.c::sp_init() which decides whether CCP >> or PSP >> >> 3. If PSP, it calls pcp-dev.c::psp_dev_init() which gets that >> sp->dev_vdata->psp_vdata which is nothing more than a simple offset >> 0x10500 which is where the PSP io regs are. For example, if this offset >> is hardcoded, why are we even passing that vdata? Just set psp->io_regs = >> 0x10500. No need for all that passing of structs around. Maybe for the very first implementation we could do that and that was what was originally done for the CCP. But as you can see the CCP does not have a set register offset between various iterations of the device and it can be expected the same will hold true for the PSP. This just makes future changes easier in order to support newer devices. >> >> 4. and finally, after that *whole* init has been done, you get to do >> ->set_psp_master_device(sp); >> >> Or, you can save yourself all that jumping through hoops, merge sp-pci.c >> and sp-dev.c into a single sp.c and put *everything* sp-related into >> it. And then do the whole work of picking hw apart, detection and >> configuration in sp_pci_probe() and have helper functions preparing and >> configuring the device. >> >> At the end, it adds it to the list of devices sp.c manages and done. You >> actually have that list already: >> >> static LIST_HEAD(sp_units); >> >> in sp-dev.c. >> >> You don't need the set_master thing either - you simply set the >> sp_dev_master pointer inside sp.c >> > > > I was trying to avoid putting PSP/SEV specific changes in sp-dev.* files. > But if sp.c approach is acceptable to the maintainer then I can work > towards merging sp-dev.c and sp-pci.c into sp.c and then add the PSP/SEV > support. I would prefer to keep things separated as they are. The common code is one place and the pci/platform specific code resides in unique files. For sp-pci.c, it can be excluded from compilation if CONFIG_PCI is not defined vs. adding #ifdefs into sp-dev.c or sp.c. The code is working nicely and, at least to me, seems easily maintainable this way. If we really want to avoid the extra calls during probing, etc. then we can take a closer look afterwards and determine what is the best approach taking into account the CCP and some of the other PSP functionality that is coming. Thanks, Tom > > >> sp_init() can then go and you can replace it with its function body, >> deciding whether it is a CCP or PSP and then call the respective >> function which is also in sp.c or ccp-dev.c >> >> And then all those separate compilation units and the interfaces between >> them disappear - you have only the calls into the PSP and that's it. >> >> Btw, the CCP thing could remain separate initially, I guess, with all >> that ccp-* stuff in there. >> > > Yep, if we decide to go with your recommended approach then we should > leave the CCP as-is for now. > > >>> 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. >> >> And you don't really need that - you can do the real work directly in >> sp-dev.c or sp.c or whatever. >> >> 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. >> >> By this model probably, but it causes all that init and registration >> jump-through-hoops for no real reason. It is basically wasting cycles >> and energy. >> >> I'm all for splitting if it makes sense. But right now I don't see much >> sense in this - it is basically a bunch of small compilation units >> calling each other. And they could be merged into a single sp.c which >> does it all in one go, without a lot of blabla. > >>> 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). >> >> That's fine. You can build it on 32-bit but add to the init function >> >> if (IS_ENABLED(CONFIG_X86_32)) >> return -ENODEV; >> >> and be done with it. No need for the ifdeffery. >> > > OK, i will use IS_ENABLED where applicable.
On Tue, Oct 10, 2017 at 01:43:22PM -0500, Tom Lendacky wrote: > Maybe for the very first implementation we could do that and that was what > was originally done for the CCP. But as you can see the CCP does not have > a set register offset between various iterations of the device and it can > be expected the same will hold true for the PSP. This just makes future > changes easier in order to support newer devices. Well, that's a simple switch-case statement which maps device iteration to offset. > I would prefer to keep things separated as they are. The common code is > one place and the pci/platform specific code resides in unique files. For > sp-pci.c, it can be excluded from compilation if CONFIG_PCI is not defined > vs. adding #ifdefs into sp-dev.c or sp.c. The code is working nicely and, > at least to me, seems easily maintainable this way. But you do see that you're doing a bunch of unnecessary things during probing. And all those different devices: SP, CCP, PSP, TEE and whatnot, they're just too granulary and it is a bunch of registration code and one compilation unit calling into the other for no good reason. Or at least I don't see it. > If we really want to avoid the extra calls during probing, etc. > then we can take a closer look afterwards and determine what is the > best approach taking into account the CCP and some of the other PSP > functionality that is coming. And that coming functionality won't make things easier - you'll most likely have more things wanting to talk to more other things. But ok, your call. Just note that changing things later is always harder.
On Sun, Oct 08, 2017 at 08:30:47AM -0500, Brijesh Singh wrote: > Basically we need some variable which is outside the per-device > structure so that we don't end up creating multiple /dev/sev nodes. If > needed, I think we can remove 'has_sev_fops' variable from struct > psp_device if we decide to dynamic alloc 'struct miscdevice sev_misc' in > struct psp_device. The 'has_sev_fops' is mainly used in sev_exit(). If > we decide to dynamic alloc sev_misc then in sev_exit() we can use > psp->sev_misc != NULL instead of psp->has_sev_ops. Yap, that sounds better than an explicit variable.
On 10/11/2017 09:19 AM, Borislav Petkov wrote: > On Sun, Oct 08, 2017 at 08:30:47AM -0500, Brijesh Singh wrote: >> Basically we need some variable which is outside the per-device >> structure so that we don't end up creating multiple /dev/sev nodes. If >> needed, I think we can remove 'has_sev_fops' variable from struct >> psp_device if we decide to dynamic alloc 'struct miscdevice sev_misc' in >> struct psp_device. The 'has_sev_fops' is mainly used in sev_exit(). If >> we decide to dynamic alloc sev_misc then in sev_exit() we can use >> psp->sev_misc != NULL instead of psp->has_sev_ops. > > Yap, that sounds better than an explicit variable. > Sure, I will implement it and send you v5.2. thanks
diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c index e9b776c3acb2..f0e0fc1fb512 100644 --- a/drivers/crypto/ccp/psp-dev.c +++ b/drivers/crypto/ccp/psp-dev.c @@ -31,7 +31,6 @@ #define DEVICE_NAME "sev" static DEFINE_MUTEX(sev_cmd_mutex); -static bool sev_fops_registered; static struct psp_device *psp_alloc_struct(struct sp_device *sp) { @@ -121,7 +120,7 @@ static int sev_cmd_buffer_len(int cmd) return 0; } -static int sev_handle_cmd(int cmd, void *data, int *psp_ret) +static int sev_do_cmd(int cmd, void *data, int *psp_ret) { unsigned int phys_lsb, phys_msb; struct psp_device *psp; @@ -182,26 +181,26 @@ static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg) return -ENOTTY; } -const struct file_operations sev_fops = { +static const struct file_operations sev_fops = { .owner = THIS_MODULE, .unlocked_ioctl = sev_ioctl, }; int sev_platform_init(struct sev_data_init *data, int *error) { - return sev_handle_cmd(SEV_CMD_INIT, data, error); + return sev_do_cmd(SEV_CMD_INIT, data, error); } EXPORT_SYMBOL_GPL(sev_platform_init); int sev_platform_shutdown(int *error) { - return sev_handle_cmd(SEV_CMD_SHUTDOWN, 0, error); + return sev_do_cmd(SEV_CMD_SHUTDOWN, 0, error); } EXPORT_SYMBOL_GPL(sev_platform_shutdown); int sev_platform_status(struct sev_data_status *data, int *error) { - return sev_handle_cmd(SEV_CMD_PLATFORM_STATUS, data, error); + return sev_do_cmd(SEV_CMD_PLATFORM_STATUS, data, error); } EXPORT_SYMBOL_GPL(sev_platform_status); @@ -211,58 +210,58 @@ int sev_issue_cmd_external_user(struct file *filep, unsigned int cmd, if (!filep || filep->f_op != &sev_fops) return -EBADF; - return sev_handle_cmd(cmd, data, error); + return sev_do_cmd(cmd, data, error); } EXPORT_SYMBOL_GPL(sev_issue_cmd_external_user); int sev_guest_deactivate(struct sev_data_deactivate *data, int *error) { - return sev_handle_cmd(SEV_CMD_DEACTIVATE, data, error); + return sev_do_cmd(SEV_CMD_DEACTIVATE, data, error); } EXPORT_SYMBOL_GPL(sev_guest_deactivate); int sev_guest_activate(struct sev_data_activate *data, int *error) { - return sev_handle_cmd(SEV_CMD_ACTIVATE, data, error); + return sev_do_cmd(SEV_CMD_ACTIVATE, data, error); } EXPORT_SYMBOL_GPL(sev_guest_activate); int sev_guest_decommission(struct sev_data_decommission *data, int *error) { - return sev_handle_cmd(SEV_CMD_DECOMMISSION, data, error); + return sev_do_cmd(SEV_CMD_DECOMMISSION, data, error); } EXPORT_SYMBOL_GPL(sev_guest_decommission); int sev_guest_df_flush(int *error) { - return sev_handle_cmd(SEV_CMD_DF_FLUSH, 0, error); + return sev_do_cmd(SEV_CMD_DF_FLUSH, 0, error); } EXPORT_SYMBOL_GPL(sev_guest_df_flush); static int sev_ops_init(struct psp_device *psp) { struct miscdevice *misc = &psp->sev_misc; - int ret = 0; + int ret; /* - * SEV feature support can be detected on the multiple devices but the - * SEV FW commands must be issued on the master. During probe time we - * do not know the master hence we create /dev/sev on the first device - * probe. sev_handle_cmd() finds the right master device to when issuing - * the command to the firmware. + * SEV feature support can be detected on multiple devices but the SEV + * FW commands must be issued on the master. During probe, we do not + * know the master hence we create /dev/sev on the first device probe. + * sev_do_cmd() finds the right master device to which to issue the + * command to the firmware. */ - if (!sev_fops_registered) { - misc->minor = MISC_DYNAMIC_MINOR; - misc->name = DEVICE_NAME; - misc->fops = &sev_fops; - - ret = misc_register(misc); - if (!ret) { - sev_fops_registered = true; - psp->has_sev_fops = true; - init_waitqueue_head(&psp->sev_int_queue); - dev_info(psp->dev, "registered SEV device\n"); - } + if (psp->has_sev_fops) + return 0; + + misc->minor = MISC_DYNAMIC_MINOR; + misc->name = DEVICE_NAME; + misc->fops = &sev_fops; + + ret = misc_register(misc); + if (!ret) { + psp->has_sev_fops = true; + init_waitqueue_head(&psp->sev_int_queue); + dev_info(psp->dev, "registered SEV device\n"); } return ret; diff --git a/drivers/crypto/ccp/psp-dev.h b/drivers/crypto/ccp/psp-dev.h index 6e8f83b41521..60a33f5ff79f 100644 --- a/drivers/crypto/ccp/psp-dev.h +++ b/drivers/crypto/ccp/psp-dev.h @@ -36,7 +36,7 @@ #define PSP_CMDBUFF_ADDR_HI PSP_C2PMSG(57) #define PSP_FEATURE_REG PSP_C2PMSG(63) -#define PSP_P2CMSG(_num) (_num << 2) +#define PSP_P2CMSG(_num) ((_num) << 2) #define PSP_CMD_COMPLETE_REG 1 #define PSP_CMD_COMPLETE PSP_P2CMSG(PSP_CMD_COMPLETE_REG) diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h index 2b334fd853c9..10b843cce75f 100644 --- a/include/linux/psp-sev.h +++ b/include/linux/psp-sev.h @@ -512,8 +512,7 @@ struct sev_data_dbg { u32 len; /* In */ } __packed; -#if defined(CONFIG_CRYPTO_DEV_SP_PSP) - +#ifdef CONFIG_CRYPTO_DEV_SP_PSP /** * sev_platform_init - perform SEV INIT command * @@ -562,9 +561,9 @@ int sev_platform_status(struct sev_data_status *status, int *error); * sev_issue_cmd_external_user - issue SEV command by other driver with a file * handle. * - * The function can be used by other drivers to issue a SEV command on - * behalf by userspace. The caller must pass a valid SEV file descriptor - * so that we know that caller has access to SEV device. + * This function can be used by other drivers to issue a SEV command on + * behalf of userspace. The caller must pass a valid SEV file descriptor + * so that we know that it has access to SEV device. * * @filep - SEV device file pointer * @cmd - command to issue