| Message ID | 20170724200303.12197-3-brijesh.singh@amd.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
Hi, minor misspelling, On 24.07.2017 22:02, Brijesh Singh wrote: > Platform Security Processor (PSP) is part of AMD Secure Processor (AMD-SP), > PSP is a dedicated processor that provides the support for key management > commands in a Secure Encrypted Virtualiztion (SEV) mode, along with > software-based Tursted Executation Environment (TEE) to enable the ----------------- ^ Trusted > third-party tursted applications. -------------- ^ trusted [...]
On 07/25/2017 03:29 AM, Kamil Konieczny wrote: > Hi, > > minor misspelling, > > On 24.07.2017 22:02, Brijesh Singh wrote: >> Platform Security Processor (PSP) is part of AMD Secure Processor (AMD-SP), >> PSP is a dedicated processor that provides the support for key management >> commands in a Secure Encrypted Virtualiztion (SEV) mode, along with >> software-based Tursted Executation Environment (TEE) to enable the > ----------------- ^ Trusted >> third-party tursted applications. > -------------- ^ trusted > [...] > Noted. thanks -Brijesh
On Mon, Jul 24, 2017 at 03:02:39PM -0500, Brijesh Singh wrote: > Platform Security Processor (PSP) is part of AMD Secure Processor (AMD-SP), > PSP is a dedicated processor that provides the support for key management > commands in a Secure Encrypted Virtualiztion (SEV) mode, along with > software-based Tursted Executation Environment (TEE) to enable the > third-party tursted applications. > > Cc: Herbert Xu <herbert@gondor.apana.org.au> > Cc: David S. Miller <davem@davemloft.net> > Cc: Gary Hook <gary.hook@amd.com> > Cc: linux-crypto@vger.kernel.org > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > --- > drivers/crypto/ccp/Kconfig | 9 ++ > drivers/crypto/ccp/Makefile | 1 + > drivers/crypto/ccp/psp-dev.c | 226 +++++++++++++++++++++++++++++++++++++++++++ > drivers/crypto/ccp/psp-dev.h | 82 ++++++++++++++++ > drivers/crypto/ccp/sp-dev.c | 43 ++++++++ > drivers/crypto/ccp/sp-dev.h | 41 +++++++- > drivers/crypto/ccp/sp-pci.c | 46 +++++++++ > 7 files changed, 447 insertions(+), 1 deletion(-) > create mode 100644 drivers/crypto/ccp/psp-dev.c > create mode 100644 drivers/crypto/ccp/psp-dev.h ... > diff --git a/drivers/crypto/ccp/sp-dev.c b/drivers/crypto/ccp/sp-dev.c > index a017233..d263ba4 100644 > --- a/drivers/crypto/ccp/sp-dev.c > +++ b/drivers/crypto/ccp/sp-dev.c Hunk #1 succeeded at 24 (offset -7 lines). checking file drivers/crypto/ccp/Makefile Hunk #1 FAILED at 7. 1 out of 1 hunk FAILED checking file drivers/crypto/ccp/psp-dev.c checking file drivers/crypto/ccp/psp-dev.h can't find file to patch at input line 414 Perhaps you used the wrong -p or --strip option? The text leading up to this was: -------------------------- |diff --git a/drivers/crypto/ccp/sp-dev.c b/drivers/crypto/ccp/sp-dev.c |index a017233..d263ba4 100644 |--- a/drivers/crypto/ccp/sp-dev.c |+++ b/drivers/crypto/ccp/sp-dev.c -------------------------- What tree is that against? In any case, it doesn't apply here. > This RFC is based on tip/master commit : 22db3de (Merge branch 'x86/mm'). $ git show 22db3de fatal: ambiguous argument '22db3de': unknown revision or path not in the working tree. Do you have updated version of the series which you can send out? > @@ -67,6 +74,10 @@ struct sp_device { > /* DMA caching attribute support */ > unsigned int axcache; > > + /* get and set master device */ > + struct sp_device*(*get_psp_master_device)(void); > + void(*set_psp_master_device)(struct sp_device *); WARNING: missing space after return type #502: FILE: drivers/crypto/ccp/sp-dev.h:79: + void(*set_psp_master_device)(struct sp_device *); Don't forget to run all patches through checkpatch. Some of the warnings make sense. Thx.
Hi Boris, On 09/06/2017 12:00 PM, Borislav Petkov wrote: ... > -------------------------- > |diff --git a/drivers/crypto/ccp/sp-dev.c b/drivers/crypto/ccp/sp-dev.c > |index a017233..d263ba4 100644 > |--- a/drivers/crypto/ccp/sp-dev.c > |+++ b/drivers/crypto/ccp/sp-dev.c > -------------------------- > > What tree is that against? In any case, it doesn't apply here. > >> This RFC is based on tip/master commit : 22db3de (Merge branch 'x86/mm'). > This bit of my struggle -- tip/master is not in sync with cryptodev-2.6 [1]. In order to expand the CCP driver we need the following commits from the cryptodev-2.6 57de3aefb73f crypto: ccp - remove ccp_present() check from device initialize d0ebbc0c407a crypto: ccp - rename ccp driver initialize files as sp device f4d18d656f88 crypto: ccp - Abstract interrupt registeration 720419f01832 crypto: ccp - Introduce the AMD Secure Processor device 970e8303cb8d crypto: ccp - Use devres interface to allocate PCI/iomap and cleanup I cherry-picked these patches into tip/master before starting the SEV work. Since these patches were already reviewed and accepted hence I did not include it in my RFC series. I am not sure what is best way to handle it. Should I include these patches in the series ? or just mention them in cover letter ? I am looking for suggestions on how to best communicate it. thanks [1] https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git/ My staging tree on github contain these precursor patches. > $ git show 22db3de > fatal: ambiguous argument '22db3de': unknown revision or path not in the working tree. > > Do you have updated version of the series which you can send out? > >> @@ -67,6 +74,10 @@ struct sp_device { >> /* DMA caching attribute support */ >> unsigned int axcache; >> >> + /* get and set master device */ >> + struct sp_device*(*get_psp_master_device)(void); >> + void(*set_psp_master_device)(struct sp_device *); > > WARNING: missing space after return type > #502: FILE: drivers/crypto/ccp/sp-dev.h:79: > + void(*set_psp_master_device)(struct sp_device *); > > Don't forget to run all patches through checkpatch. Some of the warnings > make sense. > > Thx. >
On Wed, Sep 06, 2017 at 03:38:38PM -0500, Brijesh Singh wrote: > This bit of my struggle -- tip/master is not in sync with cryptodev-2.6 [1]. Aaha. > In order to expand the CCP driver we need the following commits from the > cryptodev-2.6 > > 57de3aefb73f crypto: ccp - remove ccp_present() check from device initialize > d0ebbc0c407a crypto: ccp - rename ccp driver initialize files as sp device > f4d18d656f88 crypto: ccp - Abstract interrupt registeration > 720419f01832 crypto: ccp - Introduce the AMD Secure Processor device > 970e8303cb8d crypto: ccp - Use devres interface to allocate PCI/iomap and cleanup > > I cherry-picked these patches into tip/master before starting the SEV work. > > Since these patches were already reviewed and accepted hence I did not include it > in my RFC series. I am not sure what is best way to handle it. Should I include > these patches in the series ? or just mention them in cover letter ? I am looking > for suggestions on how to best communicate it. thanks Right, so I'm assuming those will go upstream this merge window, no? Because if so, the rest of your patches will apply, once Linus merges them. I guess for the purposes of reviewing, I can cherrypick them too. Thx.
On 09/06/2017 03:46 PM, Borislav Petkov wrote: > On Wed, Sep 06, 2017 at 03:38:38PM -0500, Brijesh Singh wrote: >> This bit of my struggle -- tip/master is not in sync with cryptodev-2.6 [1]. > > Aaha. > >> In order to expand the CCP driver we need the following commits from the >> cryptodev-2.6 >> >> 57de3aefb73f crypto: ccp - remove ccp_present() check from device initialize >> d0ebbc0c407a crypto: ccp - rename ccp driver initialize files as sp device >> f4d18d656f88 crypto: ccp - Abstract interrupt registeration >> 720419f01832 crypto: ccp - Introduce the AMD Secure Processor device >> 970e8303cb8d crypto: ccp - Use devres interface to allocate PCI/iomap and cleanup >> >> I cherry-picked these patches into tip/master before starting the SEV work. >> >> Since these patches were already reviewed and accepted hence I did not include it >> in my RFC series. I am not sure what is best way to handle it. Should I include >> these patches in the series ? or just mention them in cover letter ? I am looking >> for suggestions on how to best communicate it. thanks > > Right, so I'm assuming those will go upstream this merge window, no? They were included in a pull request (for 4.14) from Herbert, dated Monday.
On Wed, Sep 06, 2017 at 04:26:52PM -0500, Gary R Hook wrote:
> They were included in a pull request (for 4.14) from Herbert, dated Monday.
Right. I rebased the SEV pile ontop of latest upstream and now it applies much
better:
checking file drivers/crypto/ccp/Kconfig
Hunk #1 succeeded at 32 (offset 1 line).
checking file drivers/crypto/ccp/Makefile
checking file drivers/crypto/ccp/psp-dev.c
checking file drivers/crypto/ccp/psp-dev.h
checking file drivers/crypto/ccp/sp-dev.c
Hunk #3 succeeded at 225 (offset 1 line).
Hunk #4 FAILED at 243.
1 out of 4 hunks FAILED
checking file drivers/crypto/ccp/sp-dev.h
Hunk #1 succeeded at 42 with fuzz 2 (offset 1 line).
Hunk #2 succeeded at 75 (offset 1 line).
Hunk #3 succeeded at 114 (offset 1 line).
Hunk #4 succeeded at 143 (offset 1 line).
checking file drivers/crypto/ccp/sp-pci.c
The failed hunk is due to #ifdef CONFIG_PM guards but that's trivial to fix.
Thanks.
On Mon, Jul 24, 2017 at 03:02:39PM -0500, Brijesh Singh wrote: > Platform Security Processor (PSP) is part of AMD Secure Processor (AMD-SP), > PSP is a dedicated processor that provides the support for key management > commands in a Secure Encrypted Virtualiztion (SEV) mode, along with > software-based Tursted Executation Environment (TEE) to enable the ^^^^^^^^^^^^^^^^^^^ > third-party tursted applications. ^^^^^^^ Spellcheck pls. > Cc: Herbert Xu <herbert@gondor.apana.org.au> > Cc: David S. Miller <davem@davemloft.net> > Cc: Gary Hook <gary.hook@amd.com> > Cc: linux-crypto@vger.kernel.org > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > --- > drivers/crypto/ccp/Kconfig | 9 ++ > drivers/crypto/ccp/Makefile | 1 + > drivers/crypto/ccp/psp-dev.c | 226 +++++++++++++++++++++++++++++++++++++++++++ > drivers/crypto/ccp/psp-dev.h | 82 ++++++++++++++++ > drivers/crypto/ccp/sp-dev.c | 43 ++++++++ > drivers/crypto/ccp/sp-dev.h | 41 +++++++- > drivers/crypto/ccp/sp-pci.c | 46 +++++++++ > 7 files changed, 447 insertions(+), 1 deletion(-) > create mode 100644 drivers/crypto/ccp/psp-dev.c > create mode 100644 drivers/crypto/ccp/psp-dev.h > > diff --git a/drivers/crypto/ccp/Kconfig b/drivers/crypto/ccp/Kconfig > index 15b63fd..41c0ff5 100644 > --- a/drivers/crypto/ccp/Kconfig > +++ b/drivers/crypto/ccp/Kconfig > @@ -31,3 +31,12 @@ config CRYPTO_DEV_CCP_CRYPTO > Support for using the cryptographic API with the AMD Cryptographic > Coprocessor. This module supports offload of SHA and AES algorithms. > If you choose 'M' here, this module will be called ccp_crypto. > + > +config CRYPTO_DEV_SP_PSP > + bool "Platform Security Processor device" > + default y > + depends on CRYPTO_DEV_CCP_DD > + help > + Provide the support for AMD Platform Security Processor (PSP) device > + which can be used for communicating with Secure Encryption Virtualization > + (SEV) firmware. The commit message above reads better to me as the help text than what you have here. Also, in order to make it easier for the user, I think we'll need a CONFIG_AMD_MEM_ENCRYPT_SEV or so and make that depend on CONFIG_KVM_AMD, this above and all the other pieces that are needed. Just so that when the user builds such a kernel, all is enabled and not her having to go look for what else is needed. And then put the sev code behind that config option. Depending on how ugly it gets... > diff --git a/drivers/crypto/ccp/Makefile b/drivers/crypto/ccp/Makefile > index 5f2adc5..8aae4ff 100644 > --- a/drivers/crypto/ccp/Makefile > +++ b/drivers/crypto/ccp/Makefile > @@ -7,6 +7,7 @@ ccp-$(CONFIG_CRYPTO_DEV_SP_CCP) += ccp-dev.o \ > ccp-dmaengine.o \ > ccp-debugfs.o > ccp-$(CONFIG_PCI) += sp-pci.o > +ccp-$(CONFIG_CRYPTO_DEV_SP_PSP) += psp-dev.o > > obj-$(CONFIG_CRYPTO_DEV_CCP_CRYPTO) += ccp-crypto.o > ccp-crypto-objs := ccp-crypto-main.o \ > diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c > new file mode 100644 > index 0000000..bb0ea9a > --- /dev/null > +++ b/drivers/crypto/ccp/psp-dev.c > @@ -0,0 +1,226 @@ > +/* > + * AMD Platform Security Processor (PSP) interface > + * > + * Copyright (C) 2016 Advanced Micro Devices, Inc. > + * > + * Author: Brijesh Singh <brijesh.singh@amd.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > + > +#include <linux/module.h> > +#include <linux/kernel.h> > +#include <linux/kthread.h> > +#include <linux/sched.h> > +#include <linux/interrupt.h> > +#include <linux/spinlock.h> > +#include <linux/spinlock_types.h> > +#include <linux/types.h> > +#include <linux/mutex.h> > +#include <linux/delay.h> > +#include <linux/hw_random.h> > +#include <linux/ccp.h> > + > +#include "sp-dev.h" > +#include "psp-dev.h" > + > +static LIST_HEAD(psp_devs); > +static DEFINE_SPINLOCK(psp_devs_lock); > + > +const struct psp_vdata psp_entry = { > + .offset = 0x10500, > +}; > + > +void psp_add_device(struct psp_device *psp) That function is needlessly global and should be static, AFAICT. Better yet, it is called only once and its body is trivial so you can completely get rid of it and meld it into the callsite. > +{ > + unsigned long flags; > + > + spin_lock_irqsave(&psp_devs_lock, flags); > + > + list_add_tail(&psp->entry, &psp_devs); > + > + spin_unlock_irqrestore(&psp_devs_lock, flags); > +} > + > +void psp_del_device(struct psp_device *psp) Ditto. > +{ > + unsigned long flags; > + > + spin_lock_irqsave(&psp_devs_lock, flags); > + > + list_del(&psp->entry); > + spin_unlock_irqrestore(&psp_devs_lock, flags); > +} > + > +static struct psp_device *psp_alloc_struct(struct sp_device *sp) "psp_alloc()" is enough I guess. > +{ > + struct device *dev = sp->dev; > + struct psp_device *psp; > + > + psp = devm_kzalloc(dev, sizeof(*psp), GFP_KERNEL); > + if (!psp) > + return NULL; > + > + psp->dev = dev; > + psp->sp = sp; > + > + snprintf(psp->name, sizeof(psp->name), "psp-%u", sp->ord); > + > + return psp; > +} > + > +irqreturn_t psp_irq_handler(int irq, void *data) static. Please audit all your functions in the psp pile and make them static if not needed outside of their compilation unit. > +{ > + unsigned int status; > + irqreturn_t ret = IRQ_HANDLED; > + struct psp_device *psp = data; Please sort function local variables declaration in a reverse christmas tree order: <type> longest_variable_name; <type> shorter_var_name; <type> even_shorter; <type> i; > + > + /* read the interrupt status */ > + status = ioread32(psp->io_regs + PSP_P2CMSG_INTSTS); > + > + /* invoke subdevice interrupt handlers */ > + if (status) { > + if (psp->sev_irq_handler) > + ret = psp->sev_irq_handler(irq, psp->sev_irq_data); > + if (psp->tee_irq_handler) > + ret = psp->tee_irq_handler(irq, psp->tee_irq_data); > + } > + > + /* clear the interrupt status */ > + iowrite32(status, psp->io_regs + PSP_P2CMSG_INTSTS); We're clearing the status by writing the same value back?!? Shouldn't that be: iowrite32(0, psp->io_regs + PSP_P2CMSG_INTSTS); Below I see iowrite32(0xffffffff, psp->io_regs + PSP_P2CMSG_INTSTS); which is supposed to clear IRQs. Btw, you can write that: iowrite32(-1, psp->io_regs + PSP_P2CMSG_INTSTS); > + > + return ret; > +} > + > +static int psp_init(struct psp_device *psp) > +{ > + psp_add_device(psp); > + > + return 0; > +} A function which should be returning void. > + > +int psp_dev_init(struct sp_device *sp) > +{ > + struct device *dev = sp->dev; > + struct psp_device *psp; > + int ret; > + > + ret = -ENOMEM; > + psp = psp_alloc_struct(sp); > + if (!psp) > + goto e_err; <---- newline here. > + sp->psp_data = psp; > + > + psp->vdata = (struct psp_vdata *)sp->dev_vdata->psp_vdata; > + if (!psp->vdata) { > + ret = -ENODEV; > + dev_err(dev, "missing driver data\n"); > + goto e_err; > + } > + > + psp->io_regs = sp->io_map + psp->vdata->offset; > + > + /* Disable and clear interrupts until ready */ > + iowrite32(0, psp->io_regs + PSP_P2CMSG_INTEN); > + iowrite32(0xffffffff, psp->io_regs + PSP_P2CMSG_INTSTS); > + > + dev_dbg(dev, "requesting an IRQ ...\n"); > + /* Request an irq */ > + ret = sp_request_psp_irq(psp->sp, psp_irq_handler, psp->name, psp); > + if (ret) { > + dev_err(dev, "psp: unable to allocate an IRQ\n"); > + goto e_err; > + } > + > + sp_set_psp_master(sp); So this function is called only once and declared somewhere else. You could simply do here: if (sp->set_psp_master_device) sp->set_psp_master_device(sp); and get rid of one more global function. > + > + dev_dbg(dev, "initializing psp\n"); > + ret = psp_init(psp); > + if (ret) { > + dev_err(dev, "failed to init psp\n"); > + goto e_irq; > + } That error handling will never execute. Thus the e_irq label is not needed too. > + > + /* Enable interrupt */ > + dev_dbg(dev, "Enabling interrupts ...\n"); > + iowrite32(7, psp->io_regs + PSP_P2CMSG_INTEN); Uh, a magic "7"! Exciting! I wonder what that means and whether it could be a define with an explanatory name instead. Ditto for the other values... > + > + dev_notice(dev, "psp enabled\n"); > + > + return 0; > + > +e_irq: > + sp_free_psp_irq(psp->sp, psp); > +e_err: > + sp->psp_data = NULL; > + > + dev_notice(dev, "psp initialization failed\n"); > + > + return ret; > +} > + > +void psp_dev_destroy(struct sp_device *sp) > +{ > + struct psp_device *psp = sp->psp_data; > + > + sp_free_psp_irq(sp, psp); > + > + psp_del_device(psp); > +} > + > +int psp_dev_resume(struct sp_device *sp) > +{ > + return 0; > +} > + > +int psp_dev_suspend(struct sp_device *sp, pm_message_t state) > +{ > + return 0; > +} Those last two are completely useless. Delete them pls. > + > +int psp_request_tee_irq(struct psp_device *psp, irq_handler_t handler, > + void *data) > +{ > + psp->tee_irq_data = data; > + psp->tee_irq_handler = handler; > + > + return 0; > +} void > + > +int psp_free_tee_irq(struct psp_device *psp, void *data) > +{ > + if (psp->tee_irq_handler) { > + psp->tee_irq_data = NULL; > + psp->tee_irq_handler = NULL; > + } > + > + return 0; > +} void > + > +int psp_request_sev_irq(struct psp_device *psp, irq_handler_t handler, > + void *data) > +{ > + psp->sev_irq_data = data; > + psp->sev_irq_handler = handler; > + > + return 0; > +} > + > +int psp_free_sev_irq(struct psp_device *psp, void *data) > +{ > + if (psp->sev_irq_handler) { > + psp->sev_irq_data = NULL; > + psp->sev_irq_handler = NULL; > + } > + > + return 0; > +} Both void. Please do not return values from functions which are simply void functions by design. > + > +struct psp_device *psp_get_master_device(void) > +{ > + struct sp_device *sp = sp_get_psp_master_device(); > + > + return sp ? sp->psp_data : NULL; > +} > diff --git a/drivers/crypto/ccp/psp-dev.h b/drivers/crypto/ccp/psp-dev.h > new file mode 100644 > index 0000000..6e167b8 > --- /dev/null > +++ b/drivers/crypto/ccp/psp-dev.h > @@ -0,0 +1,82 @@ > +/* > + * AMD Platform Security Processor (PSP) interface driver > + * > + * Copyright (C) 2017 Advanced Micro Devices, Inc. > + * > + * Author: Brijesh Singh <brijesh.singh@amd.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > + > +#ifndef __PSP_DEV_H__ > +#define __PSP_DEV_H__ > + > +#include <linux/device.h> > +#include <linux/pci.h> > +#include <linux/spinlock.h> > +#include <linux/mutex.h> > +#include <linux/list.h> > +#include <linux/wait.h> > +#include <linux/dmapool.h> > +#include <linux/hw_random.h> > +#include <linux/bitops.h> > +#include <linux/interrupt.h> > +#include <linux/irqreturn.h> > +#include <linux/dmaengine.h> > + > +#include "sp-dev.h" > + > +#define PSP_P2CMSG_INTEN 0x0110 > +#define PSP_P2CMSG_INTSTS 0x0114 > + > +#define PSP_C2PMSG_ATTR_0 0x0118 > +#define PSP_C2PMSG_ATTR_1 0x011c > +#define PSP_C2PMSG_ATTR_2 0x0120 > +#define PSP_C2PMSG_ATTR_3 0x0124 > +#define PSP_P2CMSG_ATTR_0 0x0128 > + > +#define PSP_CMDRESP_CMD_SHIFT 16 > +#define PSP_CMDRESP_IOC BIT(0) > +#define PSP_CMDRESP_RESP BIT(31) > +#define PSP_CMDRESP_ERR_MASK 0xffff > + > +#define MAX_PSP_NAME_LEN 16 > + > +struct psp_device { > + struct list_head entry; > + > + struct psp_vdata *vdata; > + char name[MAX_PSP_NAME_LEN]; > + > + struct device *dev; > + struct sp_device *sp; > + > + void __iomem *io_regs; > + > + irq_handler_t sev_irq_handler; > + void *sev_irq_data; > + irq_handler_t tee_irq_handler; > + void *tee_irq_data; > + > + void *sev_data; > + void *tee_data; > +}; > + > +void psp_add_device(struct psp_device *psp); > +void psp_del_device(struct psp_device *psp); > + > +int psp_request_sev_irq(struct psp_device *psp, irq_handler_t handler, > + void *data); > +int psp_free_sev_irq(struct psp_device *psp, void *data); > + > +int psp_request_tee_irq(struct psp_device *psp, irq_handler_t handler, > + void *data); Let them stick out. > +int psp_free_tee_irq(struct psp_device *psp, void *data); > + > +struct psp_device *psp_get_master_device(void); > + > +extern const struct psp_vdata psp_entry; > + > +#endif /* __PSP_DEV_H */ > diff --git a/drivers/crypto/ccp/sp-dev.c b/drivers/crypto/ccp/sp-dev.c So this file is called sp-dev and the other psp-dev. Confusing. And in general, why isn't the whole thing a single psp-dev and you can save yourself all the registering blabla and have a single driver for the whole PSP functionality? Distros will have to enable everything anyway and the whole CCP/PSP code is only a couple of KBs so you can just as well put it all into a single driver. Hm. > @@ -102,6 +113,8 @@ void sp_free_ccp_irq(struct sp_device *sp, void *data); > int sp_request_psp_irq(struct sp_device *sp, irq_handler_t handler, > const char *name, void *data); > void sp_free_psp_irq(struct sp_device *sp, void *data); > +void sp_set_psp_master(struct sp_device *sp); > +struct sp_device *sp_get_psp_master_device(void); > > #ifdef CONFIG_CRYPTO_DEV_SP_CCP > > @@ -129,4 +142,30 @@ static inline int ccp_dev_resume(struct sp_device *sp) > } > #endif /* CONFIG_CRYPTO_DEV_SP_CCP */ > > +#ifdef CONFIG_CRYPTO_DEV_SP_PSP > + > +int psp_dev_init(struct sp_device *sp); > +void psp_dev_destroy(struct sp_device *sp); > + > +int psp_dev_suspend(struct sp_device *sp, pm_message_t state); > +int psp_dev_resume(struct sp_device *sp); > +#else /* !CONFIG_CRYPTO_DEV_SP_PSP */ > + > +static inline int psp_dev_init(struct sp_device *sp) > +{ > + return 0; > +} > +static inline void psp_dev_destroy(struct sp_device *sp) { } > + > +static inline int psp_dev_suspend(struct sp_device *sp, pm_message_t state) > +{ > + return 0; > +} > +static inline int psp_dev_resume(struct sp_device *sp) > +{ > + return 0; > +} Put them on a single line: static inline int psp_dev_resume(struct sp_device *sp) { return 0; } and so on... Do that for the rest of the function stubs in the headers pls. Thx.
Hi Boris, On 09/07/2017 09:27 AM, Borislav Petkov wrote: ... > > The commit message above reads better to me as the help text than what > you have here. > > Also, in order to make it easier for the user, I think we'll need a > CONFIG_AMD_MEM_ENCRYPT_SEV or so and make that depend on CONFIG_KVM_AMD, > this above and all the other pieces that are needed. Just so that when > the user builds such a kernel, all is enabled and not her having to go > look for what else is needed. > > And then put the sev code behind that config option. Depending on how > ugly it gets... > I will add more detail in the help text. I will look into adding some depends. ... >> + >> +void psp_add_device(struct psp_device *psp) > > That function is needlessly global and should be static, AFAICT. > > Better yet, it is called only once and its body is trivial so you can > completely get rid of it and meld it into the callsite. > Agreed, will do. ..... >> + >> +static struct psp_device *psp_alloc_struct(struct sp_device *sp) > > "psp_alloc()" is enough I guess. > I was trying to adhere to the existing ccp-dev.c function naming conversion. .... > > static. > > Please audit all your functions in the psp pile and make them static if > not needed outside of their compilation unit. > Will do. >> +{ >> + unsigned int status; >> + irqreturn_t ret = IRQ_HANDLED; >> + struct psp_device *psp = data; > > Please sort function local variables declaration in a reverse christmas > tree order: > > <type> longest_variable_name; > <type> shorter_var_name; > <type> even_shorter; > <type> i; > Got it, will do >> + >> + /* read the interrupt status */ >> + status = ioread32(psp->io_regs + PSP_P2CMSG_INTSTS); >> + >> + /* invoke subdevice interrupt handlers */ >> + if (status) { >> + if (psp->sev_irq_handler) >> + ret = psp->sev_irq_handler(irq, psp->sev_irq_data); >> + if (psp->tee_irq_handler) >> + ret = psp->tee_irq_handler(irq, psp->tee_irq_data); >> + } >> + >> + /* clear the interrupt status */ >> + iowrite32(status, psp->io_regs + PSP_P2CMSG_INTSTS); > > We're clearing the status by writing the same value back?!? Shouldn't > that be: > > iowrite32(0, psp->io_regs + PSP_P2CMSG_INTSTS); > Actually the SW should write "1" to clear the bit. To make it clear, I can use value 1 and add comment. > Below I see > > iowrite32(0xffffffff, psp->io_regs + PSP_P2CMSG_INTSTS); > > which is supposed to clear IRQs. Btw, you can write that: > > iowrite32(-1, psp->io_regs + PSP_P2CMSG_INTSTS); > Sure, I will do that ... ... >> + >> + sp_set_psp_master(sp); > > So this function is called only once and declared somewhere else. You > could simply do here: > > if (sp->set_psp_master_device) > sp->set_psp_master_device(sp); > > and get rid of one more global function. Sure I can do that. .... >> + /* Enable interrupt */ >> + dev_dbg(dev, "Enabling interrupts ...\n"); >> + iowrite32(7, psp->io_regs + PSP_P2CMSG_INTEN); > > Uh, a magic "7"! Exciting! > > I wonder what that means and whether it could be a define with an > explanatory name instead. Ditto for the other values... > I will try to define some macro instead of hard coded values. .... >> + >> +int psp_dev_resume(struct sp_device *sp) >> +{ >> + return 0; >> +} >> + >> +int psp_dev_suspend(struct sp_device *sp, pm_message_t state) >> +{ >> + return 0; >> +} > > Those last two are completely useless. Delete them pls. > We don't have any PM support, I agree will delete it. ... >> +int psp_request_sev_irq(struct psp_device *psp, irq_handler_t handler, >> + void *data) >> +{ >> + psp->sev_irq_data = data; >> + psp->sev_irq_handler = handler; >> + >> + return 0; >> +} >> + >> +int psp_free_sev_irq(struct psp_device *psp, void *data) >> +{ >> + if (psp->sev_irq_handler) { >> + psp->sev_irq_data = NULL; >> + psp->sev_irq_handler = NULL; >> + } >> + >> + return 0; >> +} > > Both void. Please do not return values from functions which are simply > void functions by design. > thanks, will fix it. ... >> +int psp_request_sev_irq(struct psp_device *psp, irq_handler_t handler, >> + void *data); >> +int psp_free_sev_irq(struct psp_device *psp, void *data); >> + >> +int psp_request_tee_irq(struct psp_device *psp, irq_handler_t handler, >> + void *data); > > Let them stick out. okay ... > >> +int psp_free_tee_irq(struct psp_device *psp, void *data); >> + >> +struct psp_device *psp_get_master_device(void); >> + >> +extern const struct psp_vdata psp_entry; >> + >> +#endif /* __PSP_DEV_H */ >> diff --git a/drivers/crypto/ccp/sp-dev.c b/drivers/crypto/ccp/sp-dev.c > > So this file is called sp-dev and the other psp-dev. Confusing. > > And in general, why isn't the whole thing a single psp-dev and you can > save yourself all the registering blabla and have a single driver for > the whole PSP functionality? > > Distros will have to enable everything anyway and the whole CCP/PSP code > is only a couple of KBs so you can just as well put it all into a single > driver. Hm. > PSP provides the interface for communicating with SEV and TEE FWs. I choose to add generic PSP interface first then plug the SEV FW support. The TEE commands may be totally different from SEV FW commands hence I tried to put all the SEV specific changes into one place and adhere to current ccp file naming convention. 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 -Brijesh
On 09/07/2017 05:19 PM, Brijesh Singh wrote: > Hi Boris, > > On 09/07/2017 09:27 AM, Borislav Petkov wrote: > > ... > >> >> The commit message above reads better to me as the help text than what >> you have here. >> >> Also, in order to make it easier for the user, I think we'll need a >> CONFIG_AMD_MEM_ENCRYPT_SEV or so and make that depend on CONFIG_KVM_AMD, >> this above and all the other pieces that are needed. Just so that when >> the user builds such a kernel, all is enabled and not her having to go >> look for what else is needed. >> >> And then put the sev code behind that config option. Depending on how >> ugly it gets... >> > > I will add more detail in the help text. I will look into adding some > depends. > > ... > >>> + >>> +void psp_add_device(struct psp_device *psp) >> >> That function is needlessly global and should be static, AFAICT. >> >> Better yet, it is called only once and its body is trivial so you can >> completely get rid of it and meld it into the callsite. >> > > Agreed, will do. > > ..... > >>> + >>> +static struct psp_device *psp_alloc_struct(struct sp_device *sp) >> >> "psp_alloc()" is enough I guess. >> > > I was trying to adhere to the existing ccp-dev.c function naming > conversion. I would prefer that we not shorten this. The prior incarnation, ccp_alloc_struct(), has/had been around for a while. And there are a number of similarly named allocation functions in the driver that we like to keep sorted. If anything, it should be more explanatory, IMO. > > .... > >> >> static. >> >> Please audit all your functions in the psp pile and make them static if >> not needed outside of their compilation unit. >> > > Will do. > >>> +{ >>> + unsigned int status; >>> + irqreturn_t ret = IRQ_HANDLED; >>> + struct psp_device *psp = data; >> >> Please sort function local variables declaration in a reverse christmas >> tree order: >> >> <type> longest_variable_name; >> <type> shorter_var_name; >> <type> even_shorter; >> <type> i; >> > > Got it, will do > > >>> + >>> + /* read the interrupt status */ >>> + status = ioread32(psp->io_regs + PSP_P2CMSG_INTSTS); >>> + >>> + /* invoke subdevice interrupt handlers */ >>> + if (status) { >>> + if (psp->sev_irq_handler) >>> + ret = psp->sev_irq_handler(irq, psp->sev_irq_data); >>> + if (psp->tee_irq_handler) >>> + ret = psp->tee_irq_handler(irq, psp->tee_irq_data); >>> + } >>> + >>> + /* clear the interrupt status */ >>> + iowrite32(status, psp->io_regs + PSP_P2CMSG_INTSTS); >> >> We're clearing the status by writing the same value back?!? Shouldn't >> that be: >> >> iowrite32(0, psp->io_regs + PSP_P2CMSG_INTSTS); >> > > Actually the SW should write "1" to clear the bit. To make it clear, I > can use value 1 and add comment. > > > >> Below I see >> >> iowrite32(0xffffffff, psp->io_regs + PSP_P2CMSG_INTSTS); >> >> which is supposed to clear IRQs. Btw, you can write that: >> >> iowrite32(-1, psp->io_regs + PSP_P2CMSG_INTSTS); >> > > Sure, I will do that > > ... > > ... > >>> + >>> + sp_set_psp_master(sp); >> >> So this function is called only once and declared somewhere else. You >> could simply do here: >> >> if (sp->set_psp_master_device) >> sp->set_psp_master_device(sp); >> >> and get rid of one more global function. > > > Sure I can do that. > > .... > >>> + /* Enable interrupt */ >>> + dev_dbg(dev, "Enabling interrupts ...\n"); >>> + iowrite32(7, psp->io_regs + PSP_P2CMSG_INTEN); >> >> Uh, a magic "7"! Exciting! >> >> I wonder what that means and whether it could be a define with an >> explanatory name instead. Ditto for the other values... >> > > > I will try to define some macro instead of hard coded values. > > .... > >>> + >>> +int psp_dev_resume(struct sp_device *sp) >>> +{ >>> + return 0; >>> +} >>> + >>> +int psp_dev_suspend(struct sp_device *sp, pm_message_t state) >>> +{ >>> + return 0; >>> +} >> >> Those last two are completely useless. Delete them pls. >> > > We don't have any PM support, I agree will delete it. > > ... > >>> +int psp_request_sev_irq(struct psp_device *psp, irq_handler_t handler, >>> + void *data) >>> +{ >>> + psp->sev_irq_data = data; >>> + psp->sev_irq_handler = handler; >>> + >>> + return 0; >>> +} >>> + >>> +int psp_free_sev_irq(struct psp_device *psp, void *data) >>> +{ >>> + if (psp->sev_irq_handler) { >>> + psp->sev_irq_data = NULL; >>> + psp->sev_irq_handler = NULL; >>> + } >>> + >>> + return 0; >>> +} >> >> Both void. Please do not return values from functions which are simply >> void functions by design. >> > > thanks, will fix it. > > ... > >>> +int psp_request_sev_irq(struct psp_device *psp, irq_handler_t handler, >>> + void *data); >>> +int psp_free_sev_irq(struct psp_device *psp, void *data); >>> + >>> +int psp_request_tee_irq(struct psp_device *psp, irq_handler_t handler, >>> + void *data); >> >> Let them stick out. > > okay > > ... > >> >>> +int psp_free_tee_irq(struct psp_device *psp, void *data); >>> + >>> +struct psp_device *psp_get_master_device(void); >>> + >>> +extern const struct psp_vdata psp_entry; >>> + >>> +#endif /* __PSP_DEV_H */ >>> diff --git a/drivers/crypto/ccp/sp-dev.c b/drivers/crypto/ccp/sp-dev.c >> >> So this file is called sp-dev and the other psp-dev. Confusing. >> >> And in general, why isn't the whole thing a single psp-dev and you can >> save yourself all the registering blabla and have a single driver for >> the whole PSP functionality? >> >> Distros will have to enable everything anyway and the whole CCP/PSP code >> is only a couple of KBs so you can just as well put it all into a single >> driver. Hm. >> > > PSP provides the interface for communicating with SEV and TEE FWs. I choose > to add generic PSP interface first then plug the SEV FW support. The TEE > commands may be totally different from SEV FW commands hence I tried to put > all the SEV specific changes into one place and adhere to current ccp file > naming convention. > > 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 > > -Brijesh
On Thu, Sep 07, 2017 at 06:15:55PM -0500, Gary R Hook wrote: > I would prefer that we not shorten this. The prior incarnation, > ccp_alloc_struct(), has/had been around for a while. And there are a > number of similarly named allocation functions in the driver that we > like to keep sorted. If anything, it should be more explanatory, IMO. Well, looks like an AMD practice: $ git grep --name-only alloc_struct drivers/crypto/ccp/ccp-dev.c drivers/crypto/ccp/ccp-dev.h drivers/crypto/ccp/psp-dev.c drivers/crypto/ccp/sev-dev.c drivers/crypto/ccp/sp-dev.c drivers/crypto/ccp/sp-dev.h drivers/crypto/ccp/sp-pci.c drivers/crypto/ccp/sp-platform.c drivers/gpu/drm/amd/amdkfd/kfd_dbgmgr.c drivers/gpu/drm/amd/amdkfd/kfd_priv.h drivers/gpu/drm/amd/amdkfd/kfd_topology.c But whatever, if you like it more that way... :)
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? 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. And where are the psp_request_tee_irq() et al callers? Nothing calls those functions. So you can save yourself all that needless glue if you put them in a single psp-dev and have that functionality always present when you enable the PSP. Because this is what the PSP does - SEV and TEE services. Why would you have CRYPTO_DEV_PSP_SEV depend on CRYPTO_DEV_SP_PSP where the SEV and TEE functionality are integral part of it? And so on and so on...
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
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
diff --git a/drivers/crypto/ccp/Kconfig b/drivers/crypto/ccp/Kconfig index 15b63fd..41c0ff5 100644 --- a/drivers/crypto/ccp/Kconfig +++ b/drivers/crypto/ccp/Kconfig @@ -31,3 +31,12 @@ config CRYPTO_DEV_CCP_CRYPTO Support for using the cryptographic API with the AMD Cryptographic Coprocessor. This module supports offload of SHA and AES algorithms. If you choose 'M' here, this module will be called ccp_crypto. + +config CRYPTO_DEV_SP_PSP + bool "Platform Security Processor device" + default y + depends on CRYPTO_DEV_CCP_DD + help + Provide the support for AMD Platform Security Processor (PSP) device + which can be used for communicating with Secure Encryption Virtualization + (SEV) firmware. diff --git a/drivers/crypto/ccp/Makefile b/drivers/crypto/ccp/Makefile index 5f2adc5..8aae4ff 100644 --- a/drivers/crypto/ccp/Makefile +++ b/drivers/crypto/ccp/Makefile @@ -7,6 +7,7 @@ ccp-$(CONFIG_CRYPTO_DEV_SP_CCP) += ccp-dev.o \ ccp-dmaengine.o \ ccp-debugfs.o ccp-$(CONFIG_PCI) += sp-pci.o +ccp-$(CONFIG_CRYPTO_DEV_SP_PSP) += psp-dev.o obj-$(CONFIG_CRYPTO_DEV_CCP_CRYPTO) += ccp-crypto.o ccp-crypto-objs := ccp-crypto-main.o \ diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c new file mode 100644 index 0000000..bb0ea9a --- /dev/null +++ b/drivers/crypto/ccp/psp-dev.c @@ -0,0 +1,226 @@ +/* + * AMD Platform Security Processor (PSP) interface + * + * Copyright (C) 2016 Advanced Micro Devices, Inc. + * + * Author: Brijesh Singh <brijesh.singh@amd.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#include <linux/module.h> +#include <linux/kernel.h> +#include <linux/kthread.h> +#include <linux/sched.h> +#include <linux/interrupt.h> +#include <linux/spinlock.h> +#include <linux/spinlock_types.h> +#include <linux/types.h> +#include <linux/mutex.h> +#include <linux/delay.h> +#include <linux/hw_random.h> +#include <linux/ccp.h> + +#include "sp-dev.h" +#include "psp-dev.h" + +static LIST_HEAD(psp_devs); +static DEFINE_SPINLOCK(psp_devs_lock); + +const struct psp_vdata psp_entry = { + .offset = 0x10500, +}; + +void psp_add_device(struct psp_device *psp) +{ + unsigned long flags; + + spin_lock_irqsave(&psp_devs_lock, flags); + + list_add_tail(&psp->entry, &psp_devs); + + spin_unlock_irqrestore(&psp_devs_lock, flags); +} + +void psp_del_device(struct psp_device *psp) +{ + unsigned long flags; + + spin_lock_irqsave(&psp_devs_lock, flags); + + list_del(&psp->entry); + spin_unlock_irqrestore(&psp_devs_lock, flags); +} + +static struct psp_device *psp_alloc_struct(struct sp_device *sp) +{ + struct device *dev = sp->dev; + struct psp_device *psp; + + psp = devm_kzalloc(dev, sizeof(*psp), GFP_KERNEL); + if (!psp) + return NULL; + + psp->dev = dev; + psp->sp = sp; + + snprintf(psp->name, sizeof(psp->name), "psp-%u", sp->ord); + + return psp; +} + +irqreturn_t psp_irq_handler(int irq, void *data) +{ + unsigned int status; + irqreturn_t ret = IRQ_HANDLED; + struct psp_device *psp = data; + + /* read the interrupt status */ + status = ioread32(psp->io_regs + PSP_P2CMSG_INTSTS); + + /* invoke subdevice interrupt handlers */ + if (status) { + if (psp->sev_irq_handler) + ret = psp->sev_irq_handler(irq, psp->sev_irq_data); + if (psp->tee_irq_handler) + ret = psp->tee_irq_handler(irq, psp->tee_irq_data); + } + + /* clear the interrupt status */ + iowrite32(status, psp->io_regs + PSP_P2CMSG_INTSTS); + + return ret; +} + +static int psp_init(struct psp_device *psp) +{ + psp_add_device(psp); + + return 0; +} + +int psp_dev_init(struct sp_device *sp) +{ + struct device *dev = sp->dev; + struct psp_device *psp; + int ret; + + ret = -ENOMEM; + psp = psp_alloc_struct(sp); + if (!psp) + goto e_err; + sp->psp_data = psp; + + psp->vdata = (struct psp_vdata *)sp->dev_vdata->psp_vdata; + if (!psp->vdata) { + ret = -ENODEV; + dev_err(dev, "missing driver data\n"); + goto e_err; + } + + psp->io_regs = sp->io_map + psp->vdata->offset; + + /* Disable and clear interrupts until ready */ + iowrite32(0, psp->io_regs + PSP_P2CMSG_INTEN); + iowrite32(0xffffffff, psp->io_regs + PSP_P2CMSG_INTSTS); + + dev_dbg(dev, "requesting an IRQ ...\n"); + /* Request an irq */ + ret = sp_request_psp_irq(psp->sp, psp_irq_handler, psp->name, psp); + if (ret) { + dev_err(dev, "psp: unable to allocate an IRQ\n"); + goto e_err; + } + + sp_set_psp_master(sp); + + dev_dbg(dev, "initializing psp\n"); + ret = psp_init(psp); + if (ret) { + dev_err(dev, "failed to init psp\n"); + goto e_irq; + } + + /* Enable interrupt */ + dev_dbg(dev, "Enabling interrupts ...\n"); + iowrite32(7, psp->io_regs + PSP_P2CMSG_INTEN); + + dev_notice(dev, "psp enabled\n"); + + return 0; + +e_irq: + sp_free_psp_irq(psp->sp, psp); +e_err: + sp->psp_data = NULL; + + dev_notice(dev, "psp initialization failed\n"); + + return ret; +} + +void psp_dev_destroy(struct sp_device *sp) +{ + struct psp_device *psp = sp->psp_data; + + sp_free_psp_irq(sp, psp); + + psp_del_device(psp); +} + +int psp_dev_resume(struct sp_device *sp) +{ + return 0; +} + +int psp_dev_suspend(struct sp_device *sp, pm_message_t state) +{ + return 0; +} + +int psp_request_tee_irq(struct psp_device *psp, irq_handler_t handler, + void *data) +{ + psp->tee_irq_data = data; + psp->tee_irq_handler = handler; + + return 0; +} + +int psp_free_tee_irq(struct psp_device *psp, void *data) +{ + if (psp->tee_irq_handler) { + psp->tee_irq_data = NULL; + psp->tee_irq_handler = NULL; + } + + return 0; +} + +int psp_request_sev_irq(struct psp_device *psp, irq_handler_t handler, + void *data) +{ + psp->sev_irq_data = data; + psp->sev_irq_handler = handler; + + return 0; +} + +int psp_free_sev_irq(struct psp_device *psp, void *data) +{ + if (psp->sev_irq_handler) { + psp->sev_irq_data = NULL; + psp->sev_irq_handler = NULL; + } + + return 0; +} + +struct psp_device *psp_get_master_device(void) +{ + struct sp_device *sp = sp_get_psp_master_device(); + + return sp ? sp->psp_data : NULL; +} diff --git a/drivers/crypto/ccp/psp-dev.h b/drivers/crypto/ccp/psp-dev.h new file mode 100644 index 0000000..6e167b8 --- /dev/null +++ b/drivers/crypto/ccp/psp-dev.h @@ -0,0 +1,82 @@ +/* + * AMD Platform Security Processor (PSP) interface driver + * + * Copyright (C) 2017 Advanced Micro Devices, Inc. + * + * Author: Brijesh Singh <brijesh.singh@amd.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#ifndef __PSP_DEV_H__ +#define __PSP_DEV_H__ + +#include <linux/device.h> +#include <linux/pci.h> +#include <linux/spinlock.h> +#include <linux/mutex.h> +#include <linux/list.h> +#include <linux/wait.h> +#include <linux/dmapool.h> +#include <linux/hw_random.h> +#include <linux/bitops.h> +#include <linux/interrupt.h> +#include <linux/irqreturn.h> +#include <linux/dmaengine.h> + +#include "sp-dev.h" + +#define PSP_P2CMSG_INTEN 0x0110 +#define PSP_P2CMSG_INTSTS 0x0114 + +#define PSP_C2PMSG_ATTR_0 0x0118 +#define PSP_C2PMSG_ATTR_1 0x011c +#define PSP_C2PMSG_ATTR_2 0x0120 +#define PSP_C2PMSG_ATTR_3 0x0124 +#define PSP_P2CMSG_ATTR_0 0x0128 + +#define PSP_CMDRESP_CMD_SHIFT 16 +#define PSP_CMDRESP_IOC BIT(0) +#define PSP_CMDRESP_RESP BIT(31) +#define PSP_CMDRESP_ERR_MASK 0xffff + +#define MAX_PSP_NAME_LEN 16 + +struct psp_device { + struct list_head entry; + + struct psp_vdata *vdata; + char name[MAX_PSP_NAME_LEN]; + + struct device *dev; + struct sp_device *sp; + + void __iomem *io_regs; + + irq_handler_t sev_irq_handler; + void *sev_irq_data; + irq_handler_t tee_irq_handler; + void *tee_irq_data; + + void *sev_data; + void *tee_data; +}; + +void psp_add_device(struct psp_device *psp); +void psp_del_device(struct psp_device *psp); + +int psp_request_sev_irq(struct psp_device *psp, irq_handler_t handler, + void *data); +int psp_free_sev_irq(struct psp_device *psp, void *data); + +int psp_request_tee_irq(struct psp_device *psp, irq_handler_t handler, + void *data); +int psp_free_tee_irq(struct psp_device *psp, void *data); + +struct psp_device *psp_get_master_device(void); + +extern const struct psp_vdata psp_entry; + +#endif /* __PSP_DEV_H */ diff --git a/drivers/crypto/ccp/sp-dev.c b/drivers/crypto/ccp/sp-dev.c index a017233..d263ba4 100644 --- a/drivers/crypto/ccp/sp-dev.c +++ b/drivers/crypto/ccp/sp-dev.c @@ -198,6 +198,8 @@ int sp_init(struct sp_device *sp) if (sp->dev_vdata->ccp_vdata) ccp_dev_init(sp); + if (sp->dev_vdata->psp_vdata) + psp_dev_init(sp); return 0; } @@ -206,6 +208,9 @@ void sp_destroy(struct sp_device *sp) if (sp->dev_vdata->ccp_vdata) ccp_dev_destroy(sp); + if (sp->dev_vdata->psp_vdata) + psp_dev_destroy(sp); + sp_del_device(sp); } @@ -219,6 +224,12 @@ int sp_suspend(struct sp_device *sp, pm_message_t state) return ret; } + if (sp->dev_vdata->psp_vdata) { + ret = psp_dev_suspend(sp, state); + if (ret) + return ret; + } + return 0; } @@ -232,9 +243,41 @@ int sp_resume(struct sp_device *sp) return ret; } + if (sp->dev_vdata->psp_vdata) { + ret = psp_dev_resume(sp); + if (ret) + return ret; + } return 0; } +struct sp_device *sp_get_psp_master_device(void) +{ + unsigned long flags; + struct sp_device *i, *ret = NULL; + + write_lock_irqsave(&sp_unit_lock, flags); + if (list_empty(&sp_units)) + goto unlock; + + list_for_each_entry(i, &sp_units, entry) { + if (i->psp_data) + break; + } + + if (i->get_psp_master_device) + ret = i->get_psp_master_device(); +unlock: + write_unlock_irqrestore(&sp_unit_lock, flags); + return ret; +} + +void sp_set_psp_master(struct sp_device *sp) +{ + if (sp->set_psp_master_device) + sp->set_psp_master_device(sp); +} + static int __init sp_mod_init(void) { #ifdef CONFIG_X86 diff --git a/drivers/crypto/ccp/sp-dev.h b/drivers/crypto/ccp/sp-dev.h index 3520da4..f98a3f9 100644 --- a/drivers/crypto/ccp/sp-dev.h +++ b/drivers/crypto/ccp/sp-dev.h @@ -41,12 +41,19 @@ struct ccp_vdata { const struct ccp_actions *perform; const unsigned int offset; }; + +struct psp_vdata { + const unsigned int version; + const struct psp_actions *perform; + const unsigned int offset; +}; + /* Structure to hold SP device data */ struct sp_dev_vdata { const unsigned int bar; const struct ccp_vdata *ccp_vdata; - void *psp_vdata; + const struct psp_vdata *psp_vdata; }; struct sp_device { @@ -67,6 +74,10 @@ struct sp_device { /* DMA caching attribute support */ unsigned int axcache; + /* get and set master device */ + struct sp_device*(*get_psp_master_device)(void); + void(*set_psp_master_device)(struct sp_device *); + bool irq_registered; bool use_tasklet; @@ -102,6 +113,8 @@ void sp_free_ccp_irq(struct sp_device *sp, void *data); int sp_request_psp_irq(struct sp_device *sp, irq_handler_t handler, const char *name, void *data); void sp_free_psp_irq(struct sp_device *sp, void *data); +void sp_set_psp_master(struct sp_device *sp); +struct sp_device *sp_get_psp_master_device(void); #ifdef CONFIG_CRYPTO_DEV_SP_CCP @@ -129,4 +142,30 @@ static inline int ccp_dev_resume(struct sp_device *sp) } #endif /* CONFIG_CRYPTO_DEV_SP_CCP */ +#ifdef CONFIG_CRYPTO_DEV_SP_PSP + +int psp_dev_init(struct sp_device *sp); +void psp_dev_destroy(struct sp_device *sp); + +int psp_dev_suspend(struct sp_device *sp, pm_message_t state); +int psp_dev_resume(struct sp_device *sp); +#else /* !CONFIG_CRYPTO_DEV_SP_PSP */ + +static inline int psp_dev_init(struct sp_device *sp) +{ + return 0; +} +static inline void psp_dev_destroy(struct sp_device *sp) { } + +static inline int psp_dev_suspend(struct sp_device *sp, pm_message_t state) +{ + return 0; +} +static inline int psp_dev_resume(struct sp_device *sp) +{ + return 0; +} + +#endif /* CONFIG_CRYPTO_DEV_SP_PSP */ + #endif diff --git a/drivers/crypto/ccp/sp-pci.c b/drivers/crypto/ccp/sp-pci.c index 9859aa6..e58b3ad 100644 --- a/drivers/crypto/ccp/sp-pci.c +++ b/drivers/crypto/ccp/sp-pci.c @@ -25,6 +25,7 @@ #include <linux/ccp.h> #include "ccp-dev.h" +#include "psp-dev.h" #define MSIX_VECTORS 2 @@ -32,6 +33,7 @@ struct sp_pci { int msix_count; struct msix_entry msix_entry[MSIX_VECTORS]; }; +static struct sp_device *sp_dev_master; static int sp_get_msix_irqs(struct sp_device *sp) { @@ -108,6 +110,45 @@ static void sp_free_irqs(struct sp_device *sp) sp->psp_irq = 0; } +static bool sp_pci_is_master(struct sp_device *sp) +{ + struct device *dev_cur, *dev_new; + struct pci_dev *pdev_cur, *pdev_new; + + dev_new = sp->dev; + dev_cur = sp_dev_master->dev; + + pdev_new = to_pci_dev(dev_new); + pdev_cur = to_pci_dev(dev_cur); + + if (pdev_new->bus->number < pdev_cur->bus->number) + return true; + + if (PCI_SLOT(pdev_new->devfn) < PCI_SLOT(pdev_cur->devfn)) + return true; + + if (PCI_FUNC(pdev_new->devfn) < PCI_FUNC(pdev_cur->devfn)) + return true; + + return false; +} + +static void psp_set_master(struct sp_device *sp) +{ + if (!sp_dev_master) { + sp_dev_master = sp; + return; + } + + if (sp_pci_is_master(sp)) + sp_dev_master = sp; +} + +static struct sp_device *psp_get_master(void) +{ + return sp_dev_master; +} + static int sp_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) { struct sp_device *sp; @@ -166,6 +207,8 @@ static int sp_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) goto e_err; pci_set_master(pdev); + sp->set_psp_master_device = psp_set_master; + sp->get_psp_master_device = psp_get_master; ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(48)); if (ret) { @@ -237,6 +280,9 @@ static const struct sp_dev_vdata dev_vdata[] = { #ifdef CONFIG_CRYPTO_DEV_SP_CCP .ccp_vdata = &ccpv5a, #endif +#ifdef CONFIG_CRYPTO_DEV_PSP + .psp_vdata = &psp_entry +#endif }, { .bar = 2,
Platform Security Processor (PSP) is part of AMD Secure Processor (AMD-SP), PSP is a dedicated processor that provides the support for key management commands in a Secure Encrypted Virtualiztion (SEV) mode, along with software-based Tursted Executation Environment (TEE) to enable the third-party tursted applications. Cc: Herbert Xu <herbert@gondor.apana.org.au> Cc: David S. Miller <davem@davemloft.net> Cc: Gary Hook <gary.hook@amd.com> Cc: linux-crypto@vger.kernel.org Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- drivers/crypto/ccp/Kconfig | 9 ++ drivers/crypto/ccp/Makefile | 1 + drivers/crypto/ccp/psp-dev.c | 226 +++++++++++++++++++++++++++++++++++++++++++ drivers/crypto/ccp/psp-dev.h | 82 ++++++++++++++++ drivers/crypto/ccp/sp-dev.c | 43 ++++++++ drivers/crypto/ccp/sp-dev.h | 41 +++++++- drivers/crypto/ccp/sp-pci.c | 46 +++++++++ 7 files changed, 447 insertions(+), 1 deletion(-) create mode 100644 drivers/crypto/ccp/psp-dev.c create mode 100644 drivers/crypto/ccp/psp-dev.h