* [RFC Part2 PATCH v3 00/26] x86: Secure Encrypted Virtualization (AMD)
@ 2017-07-24 20:02 Brijesh Singh
2017-07-24 20:02 ` [RFC Part2 PATCH v3 02/26] crypto: ccp: Add Platform Security Processor (PSP) device support Brijesh Singh
2017-07-24 20:02 ` [RFC Part2 PATCH v3 03/26] crypto: ccp: Add Secure Encrypted Virtualization (SEV) " Brijesh Singh
0 siblings, 2 replies; 22+ messages in thread
From: Brijesh Singh @ 2017-07-24 20:02 UTC (permalink / raw)
To: linux-kernel, x86, kvm
Cc: Thomas Gleixner, Borislav Petkov, Joerg Roedel,
Michael S . Tsirkin, Paolo Bonzini,
\"Radim Krčmář\",
Tom Lendacky, Brijesh Singh, Herbert Xu, David S . Miller,
Gary Hook, linux-crypto
This part of Secure Encryted Virtualization (SEV) patch series focuses on KVM
changes required to create and manage SEV guests.
SEV is an extension to the AMD-V architecture which supports running encrypted
virtual machine (VMs) under the control of a hypervisor. Encrypted VMs have their
pages (code and data) secured such that only the guest itself has access to
unencrypted version. Each encrypted VM is associated with a unique encryption key;
if its data is accessed to a different entity using a different key the encrypted
guest's data will be incorrectly decrypted, leading to unintelligible data.
This security model ensures that hypervisor will no longer able to inspect or
alter any guest code or data.
The key management of this feature is handled by a separate processor known as
the AMD Secure Processor (AMD-SP) which is present on AMD SOCs. The SEV Key
Management Specification (see below) provides a set of commands which can be
used by hypervisor to load virtual machine keys through the AMD-SP driver.
The patch series adds a new ioctl in KVM driver (KVM_MEMORY_ENCRYPTION_OP). The
ioctl will be used by qemu to issue SEV guest-specific commands defined in Key
Management Specification.
The following links provide additional details:
AMD Memory Encryption whitepaper:
http://amd-dev.wpengine.netdna-cdn.com/wordpress/media/2013/12/AMD_Memory_Encryption_Whitepaper_v7-Public.pdf
AMD64 Architecture Programmer's Manual:
http://support.amd.com/TechDocs/24593.pdf
SME is section 7.10
SEV is section 15.34
Secure Encrypted Virutualization Key Management:
http://support.amd.com/TechDocs/55766_SEV-KM API_Specification.pdf
KVM Forum Presentation:
http://www.linux-kvm.org/images/7/74/02x08A-Thomas_Lendacky-AMDs_Virtualizatoin_Memory_Encryption_Technology.pdf
SEV Guest BIOS support:
SEV support has been interated into EDKII/OVMF BIOS
https://github.com/tianocore/edk2
RFC part1:
http://marc.info/?l=kvm&m=150092330804060&w=2
---
This RFC is based on tip/master commit : 22db3de (Merge branch 'x86/mm').
Complete git tree is available: https://github.com/codomania/tip/tree/sev-rfc-3
TODO:
* Add SEV guest migration command support
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
Changes since v2:
* Add KVM_MEMORY_ENCRYPT_REGISTER/UNREGISTER_RAM ioct to register encrypted
memory ranges (recommend by Paolo)
* Extend kvm_x86_ops to provide new memory_encryption_enabled ops
* Enhance DEBUG DECRYPT/ENCRYPT commands to work with more than one page (recommended by Paolo)
* Optimize LAUNCH_UPDATE command to reduce the number of calls to AMD-SP driver
* Changes to address v2 feedbacks
Brijesh Singh (24):
Documentation/virtual/kvm: Add AMD Secure Encrypted Virtualization
(SEV)
crypto: ccp: Add Platform Security Processor (PSP) device support
crypto: ccp: Add Secure Encrypted Virtualization (SEV) device support
KVM: SVM: Prepare to reserve asid for SEV guest
KVM: SVM: Reserve ASID range for SEV guest
KVM: X86: Extend CPUID range to include new leaf
KVM: Introduce KVM_MEMORY_ENCRYPT_OP ioctl
KVM: Introduce KVM_MEMORY_ENCRYPT_REGISTER/UNREGISTER_RAM ioctl
KVM: X86: Extend struct kvm_arch to include SEV information
KVM: Define SEV key management command id
KVM: SVM: Add KVM_SEV_INIT command
KVM: SVM: VMRUN should use assosiated ASID when SEV is enabled
KVM: SVM: Add support for SEV LAUNCH_START command
KVM: SVM: Add support for SEV LAUNCH_UPDATE_DATA command
KVM: SVM: Add support for SEV LAUNCH_MEASURE command
KVM: SVM: Add support for SEV LAUNCH_FINISH command
KVM: svm: Add support for SEV GUEST_STATUS command
KVM: SVM: Add support for SEV DEBUG_DECRYPT command
KVM: SVM: Add support for SEV DEBUG_ENCRYPT command
KVM: SVM: Pin guest memory when SEV is active
KVM: X86: Add memory encryption enabled ops
KVM: SVM: Clear C-bit from the page fault address
KVM: SVM: Do not install #UD intercept when SEV is enabled
KVM: X86: Restart the guest when insn_len is zero and SEV is enabled
Tom Lendacky (2):
KVM: SVM: Prepare for new bit definition in nested_ctl
KVM: SVM: Add SEV feature definitions to KVM
.../virtual/kvm/amd-memory-encryption.txt | 328 ++++++
arch/x86/include/asm/kvm_host.h | 17 +
arch/x86/include/asm/svm.h | 3 +
arch/x86/kvm/cpuid.c | 2 +-
arch/x86/kvm/mmu.c | 17 +
arch/x86/kvm/svm.c | 1221 +++++++++++++++++++-
arch/x86/kvm/x86.c | 48 +
drivers/crypto/ccp/Kconfig | 19 +
drivers/crypto/ccp/Makefile | 2 +
drivers/crypto/ccp/psp-dev.c | 230 ++++
drivers/crypto/ccp/psp-dev.h | 109 ++
drivers/crypto/ccp/sev-dev.c | 416 +++++++
drivers/crypto/ccp/sev-dev.h | 67 ++
drivers/crypto/ccp/sev-ops.c | 457 ++++++++
drivers/crypto/ccp/sp-dev.c | 43 +
drivers/crypto/ccp/sp-dev.h | 41 +-
drivers/crypto/ccp/sp-pci.c | 46 +
include/linux/psp-sev.h | 683 +++++++++++
include/uapi/linux/kvm.h | 159 +++
include/uapi/linux/psp-sev.h | 110 ++
20 files changed, 4010 insertions(+), 8 deletions(-)
create mode 100644 Documentation/virtual/kvm/amd-memory-encryption.txt
create mode 100644 drivers/crypto/ccp/psp-dev.c
create mode 100644 drivers/crypto/ccp/psp-dev.h
create mode 100644 drivers/crypto/ccp/sev-dev.c
create mode 100644 drivers/crypto/ccp/sev-dev.h
create mode 100644 drivers/crypto/ccp/sev-ops.c
create mode 100644 include/linux/psp-sev.h
create mode 100644 include/uapi/linux/psp-sev.h
--
Brijesh Singh
2.9.4
^ permalink raw reply [flat|nested] 22+ messages in thread
* [RFC Part2 PATCH v3 02/26] crypto: ccp: Add Platform Security Processor (PSP) device support
2017-07-24 20:02 [RFC Part2 PATCH v3 00/26] x86: Secure Encrypted Virtualization (AMD) Brijesh Singh
@ 2017-07-24 20:02 ` Brijesh Singh
2017-07-25 8:29 ` Kamil Konieczny
` (2 more replies)
2017-07-24 20:02 ` [RFC Part2 PATCH v3 03/26] crypto: ccp: Add Secure Encrypted Virtualization (SEV) " Brijesh Singh
1 sibling, 3 replies; 22+ messages in thread
From: Brijesh Singh @ 2017-07-24 20:02 UTC (permalink / raw)
To: linux-kernel, x86, kvm
Cc: Thomas Gleixner, Borislav Petkov, Joerg Roedel,
Michael S . Tsirkin, Paolo Bonzini,
\"Radim Krčmář\",
Tom Lendacky, Brijesh Singh, Herbert Xu, David S . Miller,
Gary Hook, linux-crypto
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/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,
--
2.9.4
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [RFC Part2 PATCH v3 03/26] crypto: ccp: Add Secure Encrypted Virtualization (SEV) device support
2017-07-24 20:02 [RFC Part2 PATCH v3 00/26] x86: Secure Encrypted Virtualization (AMD) Brijesh Singh
2017-07-24 20:02 ` [RFC Part2 PATCH v3 02/26] crypto: ccp: Add Platform Security Processor (PSP) device support Brijesh Singh
@ 2017-07-24 20:02 ` Brijesh Singh
2017-09-12 14:02 ` Borislav Petkov
2017-09-13 14:17 ` Borislav Petkov
1 sibling, 2 replies; 22+ messages in thread
From: Brijesh Singh @ 2017-07-24 20:02 UTC (permalink / raw)
To: linux-kernel, x86, kvm
Cc: Thomas Gleixner, Borislav Petkov, Joerg Roedel,
Michael S . Tsirkin, Paolo Bonzini,
\"Radim Krčmář\",
Tom Lendacky, Brijesh Singh, Herbert Xu, David S . Miller,
Gary Hook, linux-crypto
AMDs new Secure Encrypted Virtualization (SEV) feature allows the memory
contents of a virtual machine to be transparently encrypted with a key
unique to the guest VM. The programming and management of the encryption
keys are handled by the AMD Secure Processor (AMD-SP), which exposes the
commands for these tasks. The complete spec for various commands are
available at:
http://support.amd.com/TechDocs/55766_SEV-KM%20API_Specification.pdf
This patch extends AMD-SP driver to provide:
- a in-kernel APIs to communicate with SEV device. The APIs can be used
by the hypervisor to create encryption context for the SEV guests.
- a userspace IOCTL to manage the platform certificates etc
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 | 10 +
drivers/crypto/ccp/Makefile | 1 +
drivers/crypto/ccp/psp-dev.c | 4 +
drivers/crypto/ccp/psp-dev.h | 27 ++
drivers/crypto/ccp/sev-dev.c | 416 ++++++++++++++++++++++++++
drivers/crypto/ccp/sev-dev.h | 67 +++++
drivers/crypto/ccp/sev-ops.c | 457 +++++++++++++++++++++++++++++
drivers/crypto/ccp/sp-pci.c | 2 +-
include/linux/psp-sev.h | 683 +++++++++++++++++++++++++++++++++++++++++++
include/uapi/linux/psp-sev.h | 110 +++++++
10 files changed, 1776 insertions(+), 1 deletion(-)
create mode 100644 drivers/crypto/ccp/sev-dev.c
create mode 100644 drivers/crypto/ccp/sev-dev.h
create mode 100644 drivers/crypto/ccp/sev-ops.c
create mode 100644 include/linux/psp-sev.h
create mode 100644 include/uapi/linux/psp-sev.h
diff --git a/drivers/crypto/ccp/Kconfig b/drivers/crypto/ccp/Kconfig
index 41c0ff5..ae0ff1c 100644
--- a/drivers/crypto/ccp/Kconfig
+++ b/drivers/crypto/ccp/Kconfig
@@ -40,3 +40,13 @@ config CRYPTO_DEV_SP_PSP
Provide the support for AMD Platform Security Processor (PSP) device
which can be used for communicating with Secure Encryption Virtualization
(SEV) firmware.
+
+config CRYPTO_DEV_PSP_SEV
+ bool "Secure Encrypted Virtualization (SEV) interface"
+ default y
+ depends on CRYPTO_DEV_CCP_DD
+ depends on CRYPTO_DEV_SP_PSP
+ help
+ Provide the kernel and userspace (/dev/sev) interface to communicate with
+ Secure Encrypted Virtualization (SEV) firmware running inside AMD Platform
+ Security Processor (PSP)
diff --git a/drivers/crypto/ccp/Makefile b/drivers/crypto/ccp/Makefile
index 8aae4ff..94ca748 100644
--- a/drivers/crypto/ccp/Makefile
+++ b/drivers/crypto/ccp/Makefile
@@ -8,6 +8,7 @@ ccp-$(CONFIG_CRYPTO_DEV_SP_CCP) += ccp-dev.o \
ccp-debugfs.o
ccp-$(CONFIG_PCI) += sp-pci.o
ccp-$(CONFIG_CRYPTO_DEV_SP_PSP) += psp-dev.o
+ccp-$(CONFIG_CRYPTO_DEV_PSP_SEV) += sev-dev.o sev-ops.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
index bb0ea9a..0c9d25c 100644
--- a/drivers/crypto/ccp/psp-dev.c
+++ b/drivers/crypto/ccp/psp-dev.c
@@ -97,6 +97,7 @@ irqreturn_t psp_irq_handler(int irq, void *data)
static int psp_init(struct psp_device *psp)
{
psp_add_device(psp);
+ sev_dev_init(psp);
return 0;
}
@@ -166,17 +167,20 @@ void psp_dev_destroy(struct sp_device *sp)
struct psp_device *psp = sp->psp_data;
sp_free_psp_irq(sp, psp);
+ sev_dev_destroy(psp);
psp_del_device(psp);
}
int psp_dev_resume(struct sp_device *sp)
{
+ sev_dev_resume(sp->psp_data);
return 0;
}
int psp_dev_suspend(struct sp_device *sp, pm_message_t state)
{
+ sev_dev_suspend(sp->psp_data, state);
return 0;
}
diff --git a/drivers/crypto/ccp/psp-dev.h b/drivers/crypto/ccp/psp-dev.h
index 6e167b8..9334d87 100644
--- a/drivers/crypto/ccp/psp-dev.h
+++ b/drivers/crypto/ccp/psp-dev.h
@@ -78,5 +78,32 @@ 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;
+#ifdef CONFIG_CRYPTO_DEV_PSP_SEV
+
+int sev_dev_init(struct psp_device *psp);
+void sev_dev_destroy(struct psp_device *psp);
+int sev_dev_resume(struct psp_device *psp);
+int sev_dev_suspend(struct psp_device *psp, pm_message_t state);
+
+#else /* !CONFIG_CRYPTO_DEV_PSP_SEV */
+
+static inline int sev_dev_init(struct psp_device *psp)
+{
+ return -ENODEV;
+}
+
+static inline void sev_dev_destroy(struct psp_device *psp) { }
+
+static inline int sev_dev_resume(struct psp_device *psp)
+{
+ return -ENODEV;
+}
+
+static inline int sev_dev_suspend(struct psp_device *psp, pm_message_t state)
+{
+ return -ENODEV;
+}
+
+#endif /* CONFIG_CRYPTO_DEV_PSP_SEV */
#endif /* __PSP_DEV_H */
diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
new file mode 100644
index 0000000..a2b41dd
--- /dev/null
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -0,0 +1,416 @@
+/*
+ * AMD Secure Encrypted Virtualization (SEV) interface
+ *
+ * Copyright (C) 2016-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.
+ */
+
+#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/wait.h>
+#include <linux/jiffies.h>
+
+#include "psp-dev.h"
+#include "sev-dev.h"
+
+extern const struct file_operations sev_fops;
+
+static LIST_HEAD(sev_devs);
+static DEFINE_SPINLOCK(sev_devs_lock);
+static atomic_t sev_id;
+
+static unsigned int sev_poll;
+module_param(sev_poll, uint, 0444);
+MODULE_PARM_DESC(sev_poll, "Poll for sev command completion - any non-zero value");
+
+DEFINE_MUTEX(sev_cmd_mutex);
+
+void sev_add_device(struct sev_device *sev)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&sev_devs_lock, flags);
+
+ list_add_tail(&sev->entry, &sev_devs);
+
+ spin_unlock_irqrestore(&sev_devs_lock, flags);
+}
+
+void sev_del_device(struct sev_device *sev)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&sev_devs_lock, flags);
+
+ list_del(&sev->entry);
+ spin_unlock_irqrestore(&sev_devs_lock, flags);
+}
+
+static struct sev_device *get_sev_master_device(void)
+{
+ struct psp_device *psp = psp_get_master_device();
+
+ return psp ? psp->sev_data : NULL;
+}
+
+static int sev_wait_cmd_poll(struct sev_device *sev, unsigned int timeout,
+ unsigned int *reg)
+{
+ int wait = timeout * 10; /* 100ms sleep => timeout * 10 */
+
+ while (--wait) {
+ msleep(100);
+
+ *reg = ioread32(sev->io_regs + PSP_CMDRESP);
+ if (*reg & PSP_CMDRESP_RESP)
+ break;
+ }
+
+ if (!wait) {
+ dev_err(sev->dev, "sev command timed out\n");
+ return -ETIMEDOUT;
+ }
+
+ return 0;
+}
+
+static int sev_wait_cmd_ioc(struct sev_device *sev, unsigned int *reg)
+{
+ sev->int_rcvd = 0;
+
+ wait_event(sev->int_queue, sev->int_rcvd);
+ *reg = ioread32(sev->io_regs + PSP_CMDRESP);
+
+ return 0;
+}
+
+static int sev_wait_cmd(struct sev_device *sev, unsigned int *reg)
+{
+ return (*reg & PSP_CMDRESP_IOC) ? sev_wait_cmd_ioc(sev, reg)
+ : sev_wait_cmd_poll(sev, 10, reg);
+}
+
+static struct sev_device *sev_alloc_struct(struct psp_device *psp)
+{
+ struct device *dev = psp->dev;
+ struct sev_device *sev;
+
+ sev = devm_kzalloc(dev, sizeof(*sev), GFP_KERNEL);
+ if (!sev)
+ return NULL;
+
+ sev->dev = dev;
+ sev->psp = psp;
+ sev->id = atomic_inc_return(&sev_id);
+
+ snprintf(sev->name, sizeof(sev->name), "sev%u", sev->id);
+ init_waitqueue_head(&sev->int_queue);
+
+ return sev;
+}
+
+irqreturn_t sev_irq_handler(int irq, void *data)
+{
+ struct sev_device *sev = data;
+ unsigned int status;
+
+ status = ioread32(sev->io_regs + PSP_P2CMSG_INTSTS);
+ if (status & (1 << PSP_CMD_COMPLETE_REG)) {
+ int reg;
+
+ reg = ioread32(sev->io_regs + PSP_CMDRESP);
+ if (reg & PSP_CMDRESP_RESP) {
+ sev->int_rcvd = 1;
+ wake_up(&sev->int_queue);
+ }
+ }
+
+ return IRQ_HANDLED;
+}
+
+static bool check_sev_support(struct sev_device *sev)
+{
+ /* Bit 0 in PSP_FEATURE_REG is set then SEV is support in PSP */
+ if (ioread32(sev->io_regs + PSP_FEATURE_REG) & 1)
+ return true;
+
+ return false;
+}
+
+int sev_dev_init(struct psp_device *psp)
+{
+ struct device *dev = psp->dev;
+ struct sev_device *sev;
+ int ret;
+
+ ret = -ENOMEM;
+ sev = sev_alloc_struct(psp);
+ if (!sev)
+ goto e_err;
+ psp->sev_data = sev;
+
+ sev->io_regs = psp->io_regs;
+
+ dev_dbg(dev, "checking SEV support ...\n");
+ /* check SEV support */
+ if (!check_sev_support(sev)) {
+ dev_dbg(dev, "device does not support SEV\n");
+ goto e_err;
+ }
+
+ dev_dbg(dev, "requesting an IRQ ...\n");
+ /* Request an irq */
+ ret = psp_request_sev_irq(sev->psp, sev_irq_handler, sev);
+ if (ret) {
+ dev_err(dev, "unable to allocate an IRQ\n");
+ goto e_err;
+ }
+
+ /* initialize SEV ops */
+ dev_dbg(dev, "init sev ops\n");
+ ret = sev_ops_init(sev);
+ if (ret) {
+ dev_err(dev, "failed to init sev ops\n");
+ goto e_irq;
+ }
+
+ sev_add_device(sev);
+
+ dev_notice(dev, "sev enabled\n");
+
+ return 0;
+
+e_irq:
+ psp_free_sev_irq(psp, sev);
+e_err:
+ psp->sev_data = NULL;
+
+ dev_notice(dev, "sev initialization failed\n");
+
+ return ret;
+}
+
+void sev_dev_destroy(struct psp_device *psp)
+{
+ struct sev_device *sev = psp->sev_data;
+
+ psp_free_sev_irq(psp, sev);
+
+ sev_ops_destroy(sev);
+
+ sev_del_device(sev);
+}
+
+int sev_dev_resume(struct psp_device *psp)
+{
+ return 0;
+}
+
+int sev_dev_suspend(struct psp_device *psp, pm_message_t state)
+{
+ return 0;
+}
+
+static int sev_cmd_buffer_len(int cmd)
+{
+ int size;
+
+ switch (cmd) {
+ case SEV_CMD_INIT:
+ size = sizeof(struct sev_data_init);
+ break;
+ case SEV_CMD_PLATFORM_STATUS:
+ size = sizeof(struct sev_data_status);
+ break;
+ case SEV_CMD_PEK_CSR:
+ size = sizeof(struct sev_data_pek_csr);
+ break;
+ case SEV_CMD_PEK_CERT_IMPORT:
+ size = sizeof(struct sev_data_pek_cert_import);
+ break;
+ case SEV_CMD_PDH_CERT_EXPORT:
+ size = sizeof(struct sev_data_pdh_cert_export);
+ break;
+ case SEV_CMD_LAUNCH_START:
+ size = sizeof(struct sev_data_launch_start);
+ break;
+ case SEV_CMD_LAUNCH_UPDATE_DATA:
+ size = sizeof(struct sev_data_launch_update_data);
+ break;
+ case SEV_CMD_LAUNCH_UPDATE_VMSA:
+ size = sizeof(struct sev_data_launch_update_vmsa);
+ break;
+ case SEV_CMD_LAUNCH_FINISH:
+ size = sizeof(struct sev_data_launch_finish);
+ break;
+ case SEV_CMD_LAUNCH_UPDATE_SECRET:
+ size = sizeof(struct sev_data_launch_secret);
+ break;
+ case SEV_CMD_LAUNCH_MEASURE:
+ size = sizeof(struct sev_data_launch_measure);
+ break;
+ case SEV_CMD_ACTIVATE:
+ size = sizeof(struct sev_data_activate);
+ break;
+ case SEV_CMD_DEACTIVATE:
+ size = sizeof(struct sev_data_deactivate);
+ break;
+ case SEV_CMD_DECOMMISSION:
+ size = sizeof(struct sev_data_decommission);
+ break;
+ case SEV_CMD_GUEST_STATUS:
+ size = sizeof(struct sev_data_guest_status);
+ break;
+ case SEV_CMD_DBG_DECRYPT:
+ case SEV_CMD_DBG_ENCRYPT:
+ size = sizeof(struct sev_data_dbg);
+ break;
+ case SEV_CMD_SEND_START:
+ size = sizeof(struct sev_data_send_start);
+ break;
+ case SEV_CMD_SEND_UPDATE_DATA:
+ size = sizeof(struct sev_data_send_update_data);
+ break;
+ case SEV_CMD_SEND_UPDATE_VMSA:
+ size = sizeof(struct sev_data_send_update_vmsa);
+ break;
+ case SEV_CMD_SEND_FINISH:
+ size = sizeof(struct sev_data_send_finish);
+ break;
+ case SEV_CMD_RECEIVE_START:
+ size = sizeof(struct sev_data_receive_start);
+ break;
+ case SEV_CMD_RECEIVE_UPDATE_DATA:
+ size = sizeof(struct sev_data_receive_update_data);
+ break;
+ case SEV_CMD_RECEIVE_UPDATE_VMSA:
+ size = sizeof(struct sev_data_receive_update_vmsa);
+ break;
+ case SEV_CMD_RECEIVE_FINISH:
+ size = sizeof(struct sev_data_receive_finish);
+ break;
+ default:
+ size = 0;
+ break;
+ }
+
+ return size;
+}
+
+int sev_issue_cmd(int cmd, void *data, int *psp_ret)
+{
+ struct sev_device *sev = get_sev_master_device();
+ unsigned int phys_lsb, phys_msb;
+ unsigned int reg, ret;
+
+ if (!sev)
+ return -ENODEV;
+
+ if (psp_ret)
+ *psp_ret = 0;
+
+ /* Set the physical address for the PSP */
+ phys_lsb = data ? lower_32_bits(__psp_pa(data)) : 0;
+ phys_msb = data ? upper_32_bits(__psp_pa(data)) : 0;
+
+ dev_dbg(sev->dev, "sev command id %#x buffer 0x%08x%08x\n",
+ cmd, phys_msb, phys_lsb);
+ print_hex_dump_debug("(in): ", DUMP_PREFIX_OFFSET, 16, 2, data,
+ sev_cmd_buffer_len(cmd), false);
+
+ /* Only one command at a time... */
+ mutex_lock(&sev_cmd_mutex);
+
+ iowrite32(phys_lsb, sev->io_regs + PSP_CMDBUFF_ADDR_LO);
+ iowrite32(phys_msb, sev->io_regs + PSP_CMDBUFF_ADDR_HI);
+ wmb();
+
+ reg = cmd;
+ reg <<= PSP_CMDRESP_CMD_SHIFT;
+ reg |= sev_poll ? 0 : PSP_CMDRESP_IOC;
+ iowrite32(reg, sev->io_regs + PSP_CMDRESP);
+
+ ret = sev_wait_cmd(sev, ®);
+ if (ret)
+ goto unlock;
+
+ if (psp_ret)
+ *psp_ret = reg & PSP_CMDRESP_ERR_MASK;
+
+ if (reg & PSP_CMDRESP_ERR_MASK) {
+ dev_dbg(sev->dev, "sev command %u failed (%#010x)\n",
+ cmd, reg & PSP_CMDRESP_ERR_MASK);
+ ret = -EIO;
+ }
+
+unlock:
+ mutex_unlock(&sev_cmd_mutex);
+ print_hex_dump_debug("(out): ", DUMP_PREFIX_OFFSET, 16, 2, data,
+ sev_cmd_buffer_len(cmd), false);
+ return ret;
+}
+
+int sev_platform_init(struct sev_data_init *data, int *error)
+{
+ return sev_issue_cmd(SEV_CMD_INIT, data, error);
+}
+EXPORT_SYMBOL_GPL(sev_platform_init);
+
+int sev_platform_shutdown(int *error)
+{
+ return sev_issue_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_issue_cmd(SEV_CMD_PLATFORM_STATUS, data, error);
+}
+EXPORT_SYMBOL_GPL(sev_platform_status);
+
+int sev_issue_cmd_external_user(struct file *filep, unsigned int cmd,
+ void *data, int *error)
+{
+ if (!filep || filep->f_op != &sev_fops)
+ return -EBADF;
+
+ return sev_issue_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_issue_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_issue_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_issue_cmd(SEV_CMD_DECOMMISSION, data, error);
+}
+EXPORT_SYMBOL_GPL(sev_guest_decommission);
+
+int sev_guest_df_flush(int *error)
+{
+ return sev_issue_cmd(SEV_CMD_DF_FLUSH, 0, error);
+}
+EXPORT_SYMBOL_GPL(sev_guest_df_flush);
diff --git a/drivers/crypto/ccp/sev-dev.h b/drivers/crypto/ccp/sev-dev.h
new file mode 100644
index 0000000..0a8ce08
--- /dev/null
+++ b/drivers/crypto/ccp/sev-dev.h
@@ -0,0 +1,67 @@
+/*
+ * AMD Secure Encrypted Virtualization (SEV) interface
+ *
+ * Copyright (C) 2016-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 __SEV_DEV_H__
+#define __SEV_DEV_H__
+
+#include <linux/device.h>
+#include <linux/spinlock.h>
+#include <linux/mutex.h>
+#include <linux/list.h>
+#include <linux/wait.h>
+#include <linux/interrupt.h>
+#include <linux/irqreturn.h>
+#include <linux/miscdevice.h>
+
+#include <linux/psp-sev.h>
+
+#define PSP_C2PMSG(_num) ((_num) << 2)
+#define PSP_CMDRESP PSP_C2PMSG(32)
+#define PSP_CMDBUFF_ADDR_LO PSP_C2PMSG(56)
+#define PSP_CMDBUFF_ADDR_HI PSP_C2PMSG(57)
+#define PSP_FEATURE_REG PSP_C2PMSG(63)
+
+#define PSP_P2CMSG(_num) (_num << 2)
+#define PSP_CMD_COMPLETE_REG 1
+#define PSP_CMD_COMPLETE PSP_P2CMSG(PSP_CMD_COMPLETE_REG)
+
+#define MAX_PSP_NAME_LEN 16
+#define SEV_DEFAULT_TIMEOUT 5
+
+struct sev_device {
+ struct list_head entry;
+
+ struct dentry *debugfs;
+ struct miscdevice misc;
+
+ unsigned int id;
+ char name[MAX_PSP_NAME_LEN];
+
+ struct device *dev;
+ struct sp_device *sp;
+ struct psp_device *psp;
+
+ void __iomem *io_regs;
+
+ unsigned int int_rcvd;
+ wait_queue_head_t int_queue;
+};
+
+void sev_add_device(struct sev_device *sev);
+void sev_del_device(struct sev_device *sev);
+
+int sev_ops_init(struct sev_device *sev);
+void sev_ops_destroy(struct sev_device *sev);
+
+int sev_issue_cmd(int cmd, void *data, int *error);
+
+#endif /* __SEV_DEV_H */
diff --git a/drivers/crypto/ccp/sev-ops.c b/drivers/crypto/ccp/sev-ops.c
new file mode 100644
index 0000000..a13d857
--- /dev/null
+++ b/drivers/crypto/ccp/sev-ops.c
@@ -0,0 +1,457 @@
+/*
+ * AMD Secure Encrypted Virtualization (SEV) command interface
+ *
+ * Copyright (C) 2016-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.
+ */
+
+#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/uaccess.h>
+
+#include <uapi/linux/psp-sev.h>
+
+#include "psp-dev.h"
+#include "sev-dev.h"
+
+static bool sev_initialized;
+static int sev_platform_get_state(int *state, int *error)
+{
+ int ret;
+ struct sev_data_status *data;
+
+ data = kzalloc(sizeof(*data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ ret = sev_platform_status(data, error);
+ *state = data->state;
+
+ kfree(data);
+ return ret;
+}
+
+static int __sev_platform_init(int *error)
+{
+ int ret;
+ struct sev_data_init *data;
+
+ data = kzalloc(sizeof(*data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ ret = sev_platform_init(data, error);
+
+ kfree(data);
+ return ret;
+}
+
+static int sev_ioctl_factory_reset(struct sev_issue_cmd *argp)
+{
+ return sev_issue_cmd(SEV_CMD_FACTORY_RESET, 0, &argp->error);
+}
+
+static int sev_ioctl_platform_status(struct sev_issue_cmd *argp)
+{
+ int ret;
+ struct sev_data_status *data;
+
+ data = kzalloc(sizeof(*data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ ret = sev_platform_status(data, &argp->error);
+
+ if (copy_to_user((void *)argp->data, data, sizeof(*data)))
+ ret = -EFAULT;
+
+ kfree(data);
+ return ret;
+}
+
+static int sev_ioctl_pek_csr(struct sev_issue_cmd *argp)
+{
+ int do_shutdown = 0;
+ int ret, state, error;
+ void *csr_addr = NULL;
+ struct sev_data_pek_csr *data;
+ struct sev_user_data_pek_csr input;
+
+ if (copy_from_user(&input, (void *)argp->data,
+ sizeof(struct sev_user_data_pek_csr)))
+ return -EFAULT;
+
+ data = kzalloc(sizeof(*data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ /*
+ * PEK_CSR command can be issued when firmware is in INIT or WORKING
+ * state. If firmware is in UNINIT state then we transition into INIT
+ * state and issue the command.
+ */
+ ret = sev_platform_get_state(&state, &argp->error);
+ if (ret)
+ return ret;
+
+ if (state == SEV_STATE_UNINIT) {
+ /* transition the plaform into INIT state */
+ ret = __sev_platform_init(&argp->error);
+ if (ret)
+ return ret;
+ do_shutdown = 1;
+ }
+
+ if (input.address) {
+ csr_addr = kmalloc(input.length, GFP_KERNEL);
+ if (!csr_addr) {
+ ret = -ENOMEM;
+ goto e_free;
+ }
+ data->address = __psp_pa(csr_addr);
+ data->length = input.length;
+ }
+
+ ret = sev_issue_cmd(SEV_CMD_PEK_CSR, data, &argp->error);
+
+ if (csr_addr) {
+ if (copy_to_user((void *)input.address, csr_addr,
+ input.length)) {
+ ret = -EFAULT;
+ goto e_free;
+ }
+ }
+
+ input.length = data->length;
+ if (copy_to_user((void *)argp->data, &input,
+ sizeof(struct sev_user_data_pek_csr)))
+ ret = -EFAULT;
+e_free:
+ if (do_shutdown)
+ sev_platform_shutdown(&error);
+ kfree(csr_addr);
+ kfree(data);
+ return ret;
+}
+
+static int sev_ioctl_pdh_gen(struct sev_issue_cmd *argp)
+{
+ int ret, state, error, do_shutdown = 0;
+
+ /*
+ * PDH_GEN command can be issued when platform is in INIT or WORKING
+ * state. If we are in UNINIT state then transition into INIT.
+ */
+ ret = sev_platform_get_state(&state, &argp->error);
+ if (ret)
+ return ret;
+
+ if (state == SEV_STATE_UNINIT) {
+ /* transition the plaform into INIT state */
+ ret = __sev_platform_init(&argp->error);
+ if (ret)
+ return ret;
+ do_shutdown = 1;
+ }
+
+ ret = sev_issue_cmd(SEV_CMD_PDH_GEN, 0, &argp->error);
+ if (do_shutdown)
+ sev_platform_shutdown(&error);
+ return ret;
+}
+
+static int sev_ioctl_pek_gen(struct sev_issue_cmd *argp)
+{
+ int do_shutdown = 0;
+ int error, ret, state;
+
+ /*
+ * PEK_GEN command can be issued only when firmware is in INIT state.
+ * If firmware is in UNINIT state then we transition into INIT state
+ * and issue the command and then shutdown.
+ */
+ ret = sev_platform_get_state(&state, &argp->error);
+ if (ret)
+ return ret;
+
+ if (state == SEV_STATE_UNINIT) {
+ /* transition the plaform into INIT state */
+ ret = __sev_platform_init(&argp->error);
+ if (ret)
+ return ret;
+
+ do_shutdown = 1;
+ }
+
+ ret = sev_issue_cmd(SEV_CMD_PEK_GEN, 0, &argp->error);
+
+ if (do_shutdown)
+ sev_platform_shutdown(&error);
+ return ret;
+}
+
+static int sev_ioctl_pek_cert_import(struct sev_issue_cmd *argp)
+{
+ int ret, state, error, do_shutdown = 0;
+ struct sev_data_pek_cert_import *data;
+ struct sev_user_data_pek_cert_import input;
+ void *pek_cert = NULL, *oca_cert = NULL;
+
+ if (copy_from_user(&input, (void *)argp->data, sizeof(*data)))
+ return -EFAULT;
+
+ if (!input.pek_cert_address || !input.pek_cert_length ||
+ !input.oca_cert_address || !input.oca_cert_length)
+ return -EINVAL;
+
+ ret = sev_platform_get_state(&state, &argp->error);
+ if (ret)
+ return ret;
+
+ /*
+ * CERT_IMPORT command can be issued only when platform is in INIT
+ * state. If we are in UNINIT state then transition into INIT state
+ * and issue the command.
+ */
+ if (state == SEV_STATE_UNINIT) {
+ /* transition platform init INIT state */
+ ret = __sev_platform_init(&argp->error);
+ if (ret)
+ return ret;
+ do_shutdown = 1;
+ }
+
+ data = kzalloc(sizeof(*data), GFP_KERNEL);
+ if (!data) {
+ ret = -ENOMEM;
+ goto e_free;
+ }
+
+ pek_cert = kmalloc(input.pek_cert_length, GFP_KERNEL);
+ if (!pek_cert) {
+ ret = -ENOMEM;
+ goto e_free;
+ }
+
+ /* copy PEK certificate from userspace */
+ if (copy_from_user(pek_cert, (void *)input.pek_cert_address,
+ input.pek_cert_length)) {
+ ret = -EFAULT;
+ goto e_free;
+ }
+
+ data->pek_cert_address = __psp_pa(pek_cert);
+ data->pek_cert_length = input.pek_cert_length;
+
+ oca_cert = kmalloc(input.oca_cert_length, GFP_KERNEL);
+ if (!oca_cert) {
+ ret = -ENOMEM;
+ goto e_free;
+ }
+
+ /* copy OCA certificate from userspace */
+ if (copy_from_user(oca_cert, (void *)input.oca_cert_address,
+ input.oca_cert_length)) {
+ ret = -EFAULT;
+ goto e_free;
+ }
+
+ data->oca_cert_address = __psp_pa(oca_cert);
+ data->oca_cert_length = input.oca_cert_length;
+
+ ret = sev_issue_cmd(SEV_CMD_PEK_CERT_IMPORT, data, &argp->error);
+e_free:
+ if (do_shutdown)
+ sev_platform_shutdown(&error);
+ kfree(oca_cert);
+ kfree(pek_cert);
+ kfree(data);
+ return ret;
+}
+
+static int sev_ioctl_pdh_cert_export(struct sev_issue_cmd *argp)
+{
+ int ret, state, error, need_shutdown = 0;
+ struct sev_data_pdh_cert_export *data;
+ struct sev_user_data_pdh_cert_export input;
+ void *pdh_cert = NULL, *cert_chain = NULL;
+
+ if (copy_from_user(&input, (void *)argp->data, sizeof(*data)))
+ return -EFAULT;
+
+ /*
+ * CERT_EXPORT command can be issued in INIT or WORKING state.
+ * If we are in UNINIT state then transition into INIT state and
+ * shutdown before exiting. But if platform is in WORKING state
+ * then EXPORT the certificate but do not shutdown the platform.
+ */
+ ret = sev_platform_get_state(&state, &argp->error);
+ if (ret)
+ return ret;
+
+ if (state == SEV_STATE_UNINIT) {
+ ret = __sev_platform_init(&argp->error);
+ if (ret)
+ return ret;
+ need_shutdown = 1;
+ }
+
+ data = kzalloc(sizeof(*data), GFP_KERNEL);
+ if (!data) {
+ ret = -ENOMEM;
+ goto e_free;
+ }
+
+ if (input.pdh_cert_address) {
+ pdh_cert = kmalloc(input.pdh_cert_length, GFP_KERNEL);
+ if (!pdh_cert) {
+ ret = -ENOMEM;
+ goto e_free;
+ }
+
+ data->pdh_cert_address = __psp_pa(pdh_cert);
+ data->pdh_cert_length = input.pdh_cert_length;
+ }
+
+ if (input.cert_chain_address) {
+ cert_chain = kmalloc(input.cert_chain_length, GFP_KERNEL);
+ if (!cert_chain) {
+ ret = -ENOMEM;
+ goto e_free;
+ }
+
+ data->cert_chain_address = __psp_pa(cert_chain);
+ data->cert_chain_length = input.cert_chain_length;
+ }
+
+ ret = sev_issue_cmd(SEV_CMD_PDH_CERT_EXPORT, data, &argp->error);
+
+ input.cert_chain_length = data->cert_chain_length;
+ input.pdh_cert_length = data->pdh_cert_length;
+
+ /* copy PDH certificate to userspace */
+ if (pdh_cert) {
+ if (copy_to_user((void *)input.pdh_cert_address,
+ pdh_cert, input.pdh_cert_length)) {
+ ret = -EFAULT;
+ goto e_free;
+ }
+ }
+
+ /* copy certificate chain to userspace */
+ if (cert_chain) {
+ if (copy_to_user((void *)input.cert_chain_address,
+ cert_chain, input.cert_chain_length)) {
+ ret = -EFAULT;
+ goto e_free;
+ }
+ }
+
+ /* copy certificate length to userspace */
+ if (copy_to_user((void *)argp->data, &input,
+ sizeof(struct sev_user_data_pdh_cert_export)))
+ ret = -EFAULT;
+
+e_free:
+ if (need_shutdown)
+ sev_platform_shutdown(&error);
+
+ kfree(cert_chain);
+ kfree(pdh_cert);
+ kfree(data);
+ return ret;
+}
+
+static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
+{
+ int ret = -EFAULT;
+ void __user *argp = (void __user *)arg;
+ struct sev_issue_cmd input;
+
+ if (ioctl != SEV_ISSUE_CMD)
+ return -EINVAL;
+
+ if (copy_from_user(&input, argp, sizeof(struct sev_issue_cmd)))
+ return -EFAULT;
+
+ if (input.cmd > SEV_CMD_MAX)
+ return -EINVAL;
+
+ switch (input.cmd) {
+
+ case SEV_USER_CMD_FACTORY_RESET: {
+ ret = sev_ioctl_factory_reset(&input);
+ break;
+ }
+ case SEV_USER_CMD_PLATFORM_STATUS: {
+ ret = sev_ioctl_platform_status(&input);
+ break;
+ }
+ case SEV_USER_CMD_PEK_GEN: {
+ ret = sev_ioctl_pek_gen(&input);
+ break;
+ }
+ case SEV_USER_CMD_PDH_GEN: {
+ ret = sev_ioctl_pdh_gen(&input);
+ break;
+ }
+ case SEV_USER_CMD_PEK_CSR: {
+ ret = sev_ioctl_pek_csr(&input);
+ break;
+ }
+ case SEV_USER_CMD_PEK_CERT_IMPORT: {
+ ret = sev_ioctl_pek_cert_import(&input);
+ break;
+ }
+ case SEV_USER_CMD_PDH_CERT_EXPORT: {
+ ret = sev_ioctl_pdh_cert_export(&input);
+ break;
+ }
+ default:
+ ret = -EINVAL;
+ break;
+ }
+
+ if (copy_to_user(argp, &input, sizeof(struct sev_issue_cmd)))
+ ret = -EFAULT;
+
+ return ret;
+}
+
+const struct file_operations sev_fops = {
+ .owner = THIS_MODULE,
+ .unlocked_ioctl = sev_ioctl,
+};
+
+int sev_ops_init(struct sev_device *sev)
+{
+ struct miscdevice *misc = &sev->misc;
+
+ /* if sev device is already registered then do nothing */
+ if (sev_initialized)
+ return 0;
+
+ misc->minor = MISC_DYNAMIC_MINOR;
+ misc->name = sev->name;
+ misc->fops = &sev_fops;
+ sev_initialized = true;
+
+ return misc_register(misc);
+}
+
+void sev_ops_destroy(struct sev_device *sev)
+{
+ misc_deregister(&sev->misc);
+}
diff --git a/drivers/crypto/ccp/sp-pci.c b/drivers/crypto/ccp/sp-pci.c
index e58b3ad..20a0f35 100644
--- a/drivers/crypto/ccp/sp-pci.c
+++ b/drivers/crypto/ccp/sp-pci.c
@@ -280,7 +280,7 @@ static const struct sp_dev_vdata dev_vdata[] = {
#ifdef CONFIG_CRYPTO_DEV_SP_CCP
.ccp_vdata = &ccpv5a,
#endif
-#ifdef CONFIG_CRYPTO_DEV_PSP
+#ifdef CONFIG_CRYPTO_DEV_SP_PSP
.psp_vdata = &psp_entry
#endif
},
diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
new file mode 100644
index 0000000..1736880
--- /dev/null
+++ b/include/linux/psp-sev.h
@@ -0,0 +1,683 @@
+/*
+ * AMD Secure Encrypted Virtualization (SEV) driver interface
+ *
+ * Copyright (C) 2016-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_SEV_H__
+#define __PSP_SEV_H__
+
+#ifdef CONFIG_X86
+#include <linux/mem_encrypt.h>
+
+#define __psp_pa(x) __sme_pa(x)
+#else
+#define __psp_pa(x) __pa(x)
+#endif
+
+/**
+ * SEV platform state
+ */
+enum sev_state {
+ SEV_STATE_UNINIT = 0x0,
+ SEV_STATE_INIT = 0x1,
+ SEV_STATE_WORKING = 0x2,
+
+ SEV_STATE_MAX
+};
+
+/**
+ * SEV platform and guest management commands
+ */
+enum sev_cmd {
+ /* platform commands */
+ SEV_CMD_INIT = 0x001,
+ SEV_CMD_SHUTDOWN = 0x002,
+ SEV_CMD_FACTORY_RESET = 0x003,
+ SEV_CMD_PLATFORM_STATUS = 0x004,
+ SEV_CMD_PEK_GEN = 0x005,
+ SEV_CMD_PEK_CSR = 0x006,
+ SEV_CMD_PEK_CERT_IMPORT = 0x007,
+ SEV_CMD_PDH_CERT_EXPORT = 0x008,
+ SEV_CMD_PDH_GEN = 0x009,
+ SEV_CMD_DF_FLUSH = 0x00A,
+
+ /* Guest commands */
+ SEV_CMD_DECOMMISSION = 0x020,
+ SEV_CMD_ACTIVATE = 0x021,
+ SEV_CMD_DEACTIVATE = 0x022,
+ SEV_CMD_GUEST_STATUS = 0x023,
+
+ /* Guest launch commands */
+ SEV_CMD_LAUNCH_START = 0x030,
+ SEV_CMD_LAUNCH_UPDATE_DATA = 0x031,
+ SEV_CMD_LAUNCH_UPDATE_VMSA = 0x032,
+ SEV_CMD_LAUNCH_MEASURE = 0x033,
+ SEV_CMD_LAUNCH_UPDATE_SECRET = 0x034,
+ SEV_CMD_LAUNCH_FINISH = 0x035,
+
+ /* Guest migration commands (outgoing) */
+ SEV_CMD_SEND_START = 0x040,
+ SEV_CMD_SEND_UPDATE_DATA = 0x041,
+ SEV_CMD_SEND_UPDATE_VMSA = 0x042,
+ SEV_CMD_SEND_FINISH = 0x043,
+
+ /* Guest migration commands (incoming) */
+ SEV_CMD_RECEIVE_START = 0x050,
+ SEV_CMD_RECEIVE_UPDATE_DATA = 0x051,
+ SEV_CMD_RECEIVE_UPDATE_VMSA = 0x052,
+ SEV_CMD_RECEIVE_FINISH = 0x053,
+
+ /* Guest debug commands */
+ SEV_CMD_DBG_DECRYPT = 0x060,
+ SEV_CMD_DBG_ENCRYPT = 0x061,
+
+ SEV_CMD_MAX,
+};
+
+/**
+ * status code returned by the commands
+ */
+enum psp_ret_code {
+ SEV_RET_SUCCESS = 0,
+ SEV_RET_INVALID_PLATFORM_STATE,
+ SEV_RET_INVALID_GUEST_STATE,
+ SEV_RET_INAVLID_CONFIG,
+ SEV_RET_INVALID_LENGTH,
+ SEV_RET_ALREADY_OWNED,
+ SEV_RET_INVALID_CERTIFICATE,
+ SEV_RET_POLICY_FAILURE,
+ SEV_RET_INACTIVE,
+ SEV_RET_INVALID_ADDRESS,
+ SEV_RET_BAD_SIGNATURE,
+ SEV_RET_BAD_MEASUREMENT,
+ SEV_RET_ASID_OWNED,
+ SEV_RET_INVALID_ASID,
+ SEV_RET_WBINVD_REQUIRED,
+ SEV_RET_DFFLUSH_REQUIRED,
+ SEV_RET_INVALID_GUEST,
+ SEV_RET_INVALID_COMMAND,
+ SEV_RET_ACTIVE,
+ SEV_RET_HWSEV_RET_PLATFORM,
+ SEV_RET_HWSEV_RET_UNSAFE,
+ SEV_RET_UNSUPPORTED,
+ SEV_RET_MAX,
+};
+
+/**
+ * struct sev_data_init - INIT command parameters
+ *
+ * @flags: processing flags
+ * @tmr_address: system physical address used for SEV-ES
+ * @tmr_length: length of tmr_address
+ */
+struct sev_data_init {
+ __u32 flags; /* In */
+ __u32 reserved; /* In */
+ __u64 tmr_address; /* In */
+ __u32 tmr_length; /* In */
+};
+
+/**
+ * struct sev_data_status - PLATFORM_STATUS command parameters
+ *
+ * @major: major API version
+ * @minor: minor API version
+ * @state: platform state
+ * @owner: self-owned or externally owned
+ * @config: platform config flags
+ * @guest_count: number of active guests
+ */
+struct sev_data_status {
+ __u8 api_major; /* Out */
+ __u8 api_minor; /* Out */
+ __u8 state; /* Out */
+ __u8 owner; /* Out */
+ __u32 config; /* Out */
+ __u32 guest_count; /* Out */
+};
+
+/**
+ * struct sev_data_pek_csr - PEK_CSR command parameters
+ *
+ * @address: PEK certificate chain
+ * @length: length of certificate
+ */
+struct sev_data_pek_csr {
+ __u64 address; /* In */
+ __u32 length; /* In/Out */
+};
+
+/**
+ * struct sev_data_cert_import - PEK_CERT_IMPORT command parameters
+ *
+ * @pek_address: PEK certificate chain
+ * @pek_length: length of PEK certificate
+ * @oca_address: OCA certificate chain
+ * @oca_length: length of OCA certificate
+ */
+struct sev_data_pek_cert_import {
+ __u64 pek_cert_address; /* In */
+ __u32 pek_cert_length; /* In */
+ __u32 reserved; /* In */
+ __u64 oca_cert_address; /* In */
+ __u32 oca_cert_length; /* In */
+};
+
+/**
+ * struct sev_data_pdh_cert_export - PDH_CERT_EXPORT command parameters
+ *
+ * @pdh_address: PDH certificate address
+ * @pdh_length: length of PDH certificate
+ * @cert_chain_address: PDH certificate chain
+ * @cert_chain_length: length of PDH certificate chain
+ */
+struct sev_data_pdh_cert_export {
+ __u64 pdh_cert_address; /* In */
+ __u32 pdh_cert_length; /* In/Out */
+ __u32 reserved; /* In */
+ __u64 cert_chain_address; /* In */
+ __u32 cert_chain_length; /* In/Out */
+};
+
+/**
+ * struct sev_data_decommission - DECOMMISSION command parameters
+ *
+ * @handle: handle of the VM to decommission
+ */
+struct sev_data_decommission {
+ u32 handle; /* In */
+};
+
+/**
+ * struct sev_data_activate - ACTIVATE command parameters
+ *
+ * @handle: handle of the VM to activate
+ * @asid: asid assigned to the VM
+ */
+struct sev_data_activate {
+ u32 handle; /* In */
+ u32 asid; /* In */
+};
+
+/**
+ * struct sev_data_deactivate - DEACTIVATE command parameters
+ *
+ * @handle: handle of the VM to deactivate
+ */
+struct sev_data_deactivate {
+ u32 handle; /* In */
+};
+
+/**
+ * struct sev_data_guest_status - SEV GUEST_STATUS command parameters
+ *
+ * @handle: handle of the VM to retrieve status
+ * @policy: policy information for the VM
+ * @asid: current ASID of the VM
+ * @state: current state of the VM
+ */
+struct sev_data_guest_status {
+ u32 handle; /* In */
+ u32 policy; /* Out */
+ u32 asid; /* Out */
+ u8 state; /* Out */
+};
+
+/**
+ * struct sev_data_launch_start - LAUNCH_START command parameters
+ *
+ * @handle: handle assigned to the VM
+ * @policy: guest launch policy
+ * @dh_cert_address: physical address of DH certificate blob
+ * @dh_cert_length: length of DH certificate blob
+ * @session_address: physical address of session parameters
+ * @session_len: length of session parameters
+ */
+struct sev_data_launch_start {
+ u32 handle; /* In/Out */
+ u32 policy; /* In */
+ u64 dh_cert_address; /* In */
+ u32 dh_cert_length; /* In */
+ u32 reserved; /* In */
+ u64 session_address; /* In */
+ u32 session_length; /* In */
+};
+
+/**
+ * struct sev_data_launch_update_data - LAUNCH_UPDATE_DATA command parameter
+ *
+ * @handle: handle of the VM to update
+ * @length: length of memory to be encrypted
+ * @address: physical address of memory region to encrypt
+ */
+struct sev_data_launch_update_data {
+ u32 handle; /* In */
+ u32 reserved;
+ u64 address; /* In */
+ u32 length; /* In */
+};
+
+/**
+ * struct sev_data_launch_update_vmsa - LAUNCH_UPDATE_VMSA command
+ *
+ * @handle: handle of the VM
+ * @address: physical address of memory region to encrypt
+ * @length: length of memory region to encrypt
+ */
+struct sev_data_launch_update_vmsa {
+ u32 handle; /* In */
+ u32 reserved;
+ u64 address; /* In */
+ u32 length; /* In */
+};
+
+/**
+ * struct sev_data_launch_measure - LAUNCH_MEASURE command parameters
+ *
+ * @handle: handle of the VM to process
+ * @address: physical address containing the measurement blob
+ * @length: length of measurement blob
+ */
+struct sev_data_launch_measure {
+ u32 handle; /* In */
+ u32 reserved;
+ u64 address; /* In */
+ u32 length; /* In/Out */
+};
+
+/**
+ * struct sev_data_launch_secret - LAUNCH_SECRET command parameters
+ *
+ * @handle: handle of the VM to process
+ * @hdr_address: physical address containing the packet header
+ * @hdr_length: length of packet header
+ * @guest_address: system physical address of guest memory region
+ * @guest_length: length of guest_paddr
+ * @trans_address: physical address of transport memory buffer
+ * @trans_length: length of transport memory buffer
+ */
+struct sev_data_launch_secret {
+ u32 handle; /* In */
+ u32 reserved1;
+ u64 hdr_address; /* In */
+ u32 hdr_length; /* In */
+ u32 reserved2;
+ u64 guest_address; /* In */
+ u32 guest_length; /* In */
+ u32 reserved3;
+ u64 trans_address; /* In */
+ u32 trans_length; /* In */
+};
+
+/**
+ * struct sev_data_launch_finish - LAUNCH_FINISH command parameters
+ *
+ * @handle: handle of the VM to process
+ */
+struct sev_data_launch_finish {
+ u32 handle; /* In */
+};
+
+/**
+ * struct sev_data_send_start - SEND_START command parameters
+ *
+ * @handle: handle of the VM to process
+ * @policy: policy information for the VM
+ * @pdh_cert_address: physical address containing PDH certificate
+ * @pdh_cert_length: length of PDH certificate
+ * @plat_certs_address: physical address containing platform certificate
+ * @plat_certs_length: length of platform certificate
+ * @amd_certs_address: physical address containing AMD certificate
+ * @amd_certs_length: length of AMD certificate
+ * @session_address: physical address containing Session data
+ * @session_length: length of session data
+ */
+struct sev_data_send_start {
+ u32 handle; /* In */
+ u32 policy; /* Out */
+ u64 pdh_cert_address; /* In */
+ u32 pdh_cert_length; /* In */
+ u32 reserved1;
+ u64 plat_cert_address; /* In */
+ u32 plat_cert_length; /* In */
+ u32 reserved2;
+ u64 amd_cert_address; /* In */
+ u32 amd_cert_length; /* In */
+ u32 reserved3;
+ u64 session_address; /* In */
+ u32 session_length; /* In/Out */
+};
+
+/**
+ * struct sev_data_send_update - SEND_UPDATE_DATA command
+ *
+ * @handle: handle of the VM to process
+ * @hdr_address: physical address containing packet header
+ * @hdr_length: length of packet header
+ * @guest_address: physical address of guest memory region to send
+ * @guest_length: length of guest memory region to send
+ * @trans_address: physical address of host memory region
+ * @trans_length: length of host memory region
+ */
+struct sev_data_send_update_data {
+ u32 handle; /* In */
+ u32 reserved1;
+ u64 hdr_address; /* In */
+ u32 hdr_length; /* In/Out */
+ u32 reserved2;
+ u64 guest_address; /* In */
+ u32 guest_length; /* In */
+ u32 reserved3;
+ u64 trans_address; /* In */
+ u32 trans_length; /* In */
+};
+
+/**
+ * struct sev_data_send_update - SEND_UPDATE_VMSA command
+ *
+ * @handle: handle of the VM to process
+ * @hdr_address: physical address containing packet header
+ * @hdr_length: length of packet header
+ * @guest_address: physical address of guest memory region to send
+ * @guest_length: length of guest memory region to send
+ * @trans_address: physical address of host memory region
+ * @trans_length: length of host memory region
+ */
+struct sev_data_send_update_vmsa {
+ u32 handle; /* In */
+ u64 hdr_address; /* In */
+ u32 hdr_length; /* In/Out */
+ u32 reserved2;
+ u64 guest_address; /* In */
+ u32 guest_length; /* In */
+ u32 reserved3;
+ u64 trans_address; /* In */
+ u32 trans_length; /* In */
+};
+
+/**
+ * struct sev_data_send_finish - SEND_FINISH command parameters
+ *
+ * @handle: handle of the VM to process
+ */
+struct sev_data_send_finish {
+ u32 handle; /* In */
+};
+
+/**
+ * struct sev_data_receive_start - RECEIVE_START command parameters
+ *
+ * @handle: handle of the VM to perform receive operation
+ * @pdh_cert_address: system physical address containing PDH certificate blob
+ * @pdh_cert_length: length of PDH certificate blob
+ * @session_address: system physical address containing session blob
+ * @session_length: length of session blob
+ */
+struct sev_data_receive_start {
+ u32 handle; /* In/Out */
+ u32 policy; /* In */
+ u64 pdh_cert_address; /* In */
+ u32 pdh_cert_length; /* In */
+ u32 reserved1;
+ u64 session_address; /* In */
+ u32 session_length; /* In */
+};
+
+/**
+ * struct sev_data_receive_update_data - RECEIVE_UPDATE_DATA command parameters
+ *
+ * @handle: handle of the VM to update
+ * @hdr_address: physical address containing packet header blob
+ * @hdr_length: length of packet header
+ * @guest_address: system physical address of guest memory region
+ * @guest_length: length of guest memory region
+ * @trans_address: system physical address of transport buffer
+ * @trans_length: length of transport buffer
+ */
+struct sev_data_receive_update_data {
+ u32 handle; /* In */
+ u32 reserved1;
+ u64 hdr_address; /* In */
+ u32 hdr_length; /* In */
+ u32 reserved2;
+ u64 guest_address; /* In */
+ u32 guest_length; /* In */
+ u32 reserved3;
+ u64 trans_address; /* In */
+ u32 trans_length; /* In */
+};
+
+/**
+ * struct sev_data_receive_update_vmsa - RECEIVE_UPDATE_VMSA command parameters
+ *
+ * @handle: handle of the VM to update
+ * @hdr_address: physical address containing packet header blob
+ * @hdr_length: length of packet header
+ * @guest_address: system physical address of guest memory region
+ * @guest_length: length of guest memory region
+ * @trans_address: system physical address of transport buffer
+ * @trans_length: length of transport buffer
+ */
+struct sev_data_receive_update_vmsa {
+ u32 handle; /* In */
+ u32 reserved1;
+ u64 hdr_address; /* In */
+ u32 hdr_length; /* In */
+ u32 reserved2;
+ u64 guest_address; /* In */
+ u32 guest_length; /* In */
+ u32 reserved3;
+ u64 trans_address; /* In */
+ u32 trans_length; /* In */
+};
+
+/**
+ * struct sev_data_receive_finish - RECEIVE_FINISH command parameters
+ *
+ * @handle: handle of the VM to finish
+ */
+struct sev_data_receive_finish {
+ u32 handle; /* In */
+};
+
+/**
+ * struct sev_data_dbg - DBG_ENCRYPT/DBG_DECRYPT command parameters
+ *
+ * @handle: handle of the VM to perform debug operation
+ * @src_addr: source address of data to operate on
+ * @dst_addr: destination address of data to operate on
+ * @length: length of data to operate on
+ */
+struct sev_data_dbg {
+ u32 handle; /* In */
+ u32 reserved;
+ u64 src_addr; /* In */
+ u64 dst_addr; /* In */
+ u32 length; /* In */
+};
+
+#if defined(CONFIG_CRYPTO_DEV_PSP_SEV)
+
+/**
+ * sev_platform_init - perform SEV INIT command
+ *
+ * @init: sev_data_init structure to be processed
+ * @error: SEV command return code
+ *
+ * Returns:
+ * 0 if the SEV successfully processed the command
+ * -%ENODEV if the SEV device is not available
+ * -%ENOTSUPP if the SEV does not support SEV
+ * -%ETIMEDOUT if the SEV command timed out
+ * -%EIO if the SEV returned a non-zero return code
+ */
+int sev_platform_init(struct sev_data_init *init, int *error);
+
+/**
+ * sev_platform_shutdown - perform SEV SHUTDOWN command
+ *
+ * @error: SEV command return code
+ *
+ * Returns:
+ * 0 if the SEV successfully processed the command
+ * -%ENODEV if the SEV device is not available
+ * -%ENOTSUPP if the SEV does not support SEV
+ * -%ETIMEDOUT if the SEV command timed out
+ * -%EIO if the SEV returned a non-zero return code
+ */
+int sev_platform_shutdown(int *error);
+
+/**
+ * sev_platform_status - perform SEV PLATFORM_STATUS command
+ *
+ * @init: sev_data_status structure to be processed
+ * @error: SEV command return code
+ *
+ * Returns:
+ * 0 if the SEV successfully processed the command
+ * -%ENODEV if the SEV device is not available
+ * -%ENOTSUPP if the SEV does not support SEV
+ * -%ETIMEDOUT if the SEV command timed out
+ * -%EIO if the SEV returned a non-zero return code
+ */
+int sev_platform_status(struct sev_data_status *status, int *error);
+
+/**
+ * sev_issue_cmd_external_user - issue SEV command by other driver
+ *
+ * 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.
+ *
+ * @filep - SEV device file pointer
+ * @cmd - command to issue
+ * @data - command buffer
+ * @error: SEV command return code
+ *
+ * Returns:
+ * 0 if the SEV successfully processed the command
+ * -%ENODEV if the SEV device is not available
+ * -%ENOTSUPP if the SEV does not support SEV
+ * -%ETIMEDOUT if the SEV command timed out
+ * -%EIO if the SEV returned a non-zero return code
+ * -%EINVAL if the SEV file descriptor is not valid
+ */
+int sev_issue_cmd_external_user(struct file *filep, unsigned int id,
+ void *data, int *error);
+
+/**
+ * sev_guest_deactivate - perform SEV DEACTIVATE command
+ *
+ * @deactivate: sev_data_deactivate structure to be processed
+ * @sev_ret: sev command return code
+ *
+ * Returns:
+ * 0 if the sev successfully processed the command
+ * -%ENODEV if the sev device is not available
+ * -%ENOTSUPP if the sev does not support SEV
+ * -%ETIMEDOUT if the sev command timed out
+ * -%EIO if the sev returned a non-zero return code
+ */
+int sev_guest_deactivate(struct sev_data_deactivate *data, int *error);
+
+/**
+ * sev_guest_activate - perform SEV ACTIVATE command
+ *
+ * @activate: sev_data_activate structure to be processed
+ * @sev_ret: sev command return code
+ *
+ * Returns:
+ * 0 if the sev successfully processed the command
+ * -%ENODEV if the sev device is not available
+ * -%ENOTSUPP if the sev does not support SEV
+ * -%ETIMEDOUT if the sev command timed out
+ * -%EIO if the sev returned a non-zero return code
+ */
+int sev_guest_activate(struct sev_data_activate *data, int *error);
+
+/**
+ * sev_guest_df_flush - perform SEV DF_FLUSH command
+ *
+ * @sev_ret: sev command return code
+ *
+ * Returns:
+ * 0 if the sev successfully processed the command
+ * -%ENODEV if the sev device is not available
+ * -%ENOTSUPP if the sev does not support SEV
+ * -%ETIMEDOUT if the sev command timed out
+ * -%EIO if the sev returned a non-zero return code
+ */
+int sev_guest_df_flush(int *error);
+
+/**
+ * sev_guest_decommission - perform SEV DECOMMISSION command
+ *
+ * @decommission: sev_data_decommission structure to be processed
+ * @sev_ret: sev command return code
+ *
+ * Returns:
+ * 0 if the sev successfully processed the command
+ * -%ENODEV if the sev device is not available
+ * -%ENOTSUPP if the sev does not support SEV
+ * -%ETIMEDOUT if the sev command timed out
+ * -%EIO if the sev returned a non-zero return code
+ */
+int sev_guest_decommission(struct sev_data_decommission *data, int *error);
+
+#else /* !CONFIG_CRYPTO_DEV_PSP_SEV */
+
+static inline int sev_platform_status(struct sev_data_status *status,
+ int *error)
+{
+ return -ENODEV;
+}
+
+static inline int sev_platform_init(struct sev_data_init *init, int *error)
+{
+ return -ENODEV;
+}
+
+static inline int sev_platform_shutdown(int *error)
+{
+ return -ENODEV;
+}
+
+static inline int sev_issue_cmd_external_user(int fd, unsigned int id,
+ void *data, int *error)
+{
+ return -ENODEV;
+}
+
+static inline int sev_guest_deactivate(struct sev_data_deactivate *data,
+ int *error)
+{
+ return -ENODEV;
+}
+
+static inline int sev_guest_decommission(struct sev_data_decommission *data,
+ int *error)
+{
+ return -ENODEV;
+}
+
+static inline int sev_guest_activate(struct sev_data_activate *data,
+ int *error)
+{
+ return -ENODEV;
+}
+
+static inline int sev_guest_df_flush(int *error)
+{
+ return -ENODEV;
+}
+
+#endif /* CONFIG_CRYPTO_DEV_PSP_SEV */
+
+#endif /* __PSP_SEV_H__ */
diff --git a/include/uapi/linux/psp-sev.h b/include/uapi/linux/psp-sev.h
new file mode 100644
index 0000000..eb802f8
--- /dev/null
+++ b/include/uapi/linux/psp-sev.h
@@ -0,0 +1,110 @@
+
+/*
+ * Userspace interface for AMD Secure Encrypted Virtualization (SEV)
+ *
+ * Copyright (C) 2016-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_SEV_USER_H__
+#define __PSP_SEV_USER_H__
+
+#include <linux/types.h>
+
+/**
+ * SEV platform commands
+ */
+enum {
+ SEV_USER_CMD_FACTORY_RESET = 0,
+ SEV_USER_CMD_PLATFORM_STATUS,
+ SEV_USER_CMD_PEK_GEN,
+ SEV_USER_CMD_PEK_CSR,
+ SEV_USER_CMD_PDH_GEN,
+ SEV_USER_CMD_PDH_CERT_EXPORT,
+ SEV_USER_CMD_PEK_CERT_IMPORT,
+
+ SEV_USER_CMD_MAX,
+};
+
+/**
+ * struct sev_user_data_status - PLATFORM_STATUS command parameters
+ *
+ * @major: major API version
+ * @minor: minor API version
+ * @state: platform state
+ * @owner: self-owned or externally owned
+ * @config: platform config flags
+ * @guest_count: number of active guests
+ */
+struct sev_user_data_status {
+ __u8 api_major; /* Out */
+ __u8 api_minor; /* Out */
+ __u8 state; /* Out */
+ __u8 owner; /* Out */
+ __u32 config; /* Out */
+ __u32 guest_count; /* Out */
+};
+
+/**
+ * struct sev_user_data_pek_csr - PEK_CSR command parameters
+ *
+ * @address: PEK certificate chain
+ * @length: length of certificate
+ */
+struct sev_user_data_pek_csr {
+ __u64 address; /* In */
+ __u32 length; /* In/Out */
+};
+
+/**
+ * struct sev_user_data_cert_import - PEK_CERT_IMPORT command parameters
+ *
+ * @pek_address: PEK certificate chain
+ * @pek_length: length of PEK certificate
+ * @oca_address: OCA certificate chain
+ * @oca_length: length of OCA certificate
+ */
+struct sev_user_data_pek_cert_import {
+ __u64 pek_cert_address; /* In */
+ __u32 pek_cert_length; /* In */
+ __u64 oca_cert_address; /* In */
+ __u32 oca_cert_length; /* In */
+};
+
+/**
+ * struct sev_user_data_pdh_cert_export - PDH_CERT_EXPORT command parameters
+ *
+ * @pdh_address: PDH certificate address
+ * @pdh_length: length of PDH certificate
+ * @cert_chain_address: PDH certificate chain
+ * @cert_chain_length: length of PDH certificate chain
+ */
+struct sev_user_data_pdh_cert_export {
+ __u64 pdh_cert_address; /* In */
+ __u32 pdh_cert_length; /* In/Out */
+ __u64 cert_chain_address; /* In */
+ __u32 cert_chain_length; /* In/Out */
+};
+
+/**
+ * struct sev_issue_cmd - SEV ioctl parameters
+ *
+ * @cmd: SEV commands to execute
+ * @opaque: pointer to the command structure
+ * @error: SEV FW return code on failure
+ */
+struct sev_issue_cmd {
+ __u32 cmd; /* In */
+ __u64 data; /* In */
+ __u32 error; /* Out */
+};
+
+#define SEV_IOC_TYPE 'S'
+#define SEV_ISSUE_CMD _IOWR(SEV_IOC_TYPE, 0x0, struct sev_issue_cmd)
+
+#endif /* __PSP_USER_SEV_H */
--
2.9.4
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [RFC Part2 PATCH v3 02/26] crypto: ccp: Add Platform Security Processor (PSP) device support
2017-07-24 20:02 ` [RFC Part2 PATCH v3 02/26] crypto: ccp: Add Platform Security Processor (PSP) device support Brijesh Singh
@ 2017-07-25 8:29 ` Kamil Konieczny
2017-07-25 15:00 ` Brijesh Singh
2017-09-06 17:00 ` Borislav Petkov
2017-09-07 14:27 ` Borislav Petkov
2 siblings, 1 reply; 22+ messages in thread
From: Kamil Konieczny @ 2017-07-25 8:29 UTC (permalink / raw)
To: Brijesh Singh, linux-kernel, x86, kvm
Cc: Thomas Gleixner, Borislav Petkov, Joerg Roedel,
Michael S . Tsirkin, Paolo Bonzini,
\"Radim Krčmář\",
Tom Lendacky, Herbert Xu, David S . Miller, Gary Hook,
linux-crypto
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
[...]
--
Best regards,
Kamil Konieczny
Samsung R&D Institute Poland
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC Part2 PATCH v3 02/26] crypto: ccp: Add Platform Security Processor (PSP) device support
2017-07-25 8:29 ` Kamil Konieczny
@ 2017-07-25 15:00 ` Brijesh Singh
0 siblings, 0 replies; 22+ messages in thread
From: Brijesh Singh @ 2017-07-25 15:00 UTC (permalink / raw)
To: Kamil Konieczny, linux-kernel, x86, kvm
Cc: brijesh.singh, Thomas Gleixner, Borislav Petkov, Joerg Roedel,
Michael S . Tsirkin, Paolo Bonzini,
\"Radim Krčmář\",
Tom Lendacky, Herbert Xu, David S . Miller, Gary Hook,
linux-crypto
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC Part2 PATCH v3 02/26] crypto: ccp: Add Platform Security Processor (PSP) device support
2017-07-24 20:02 ` [RFC Part2 PATCH v3 02/26] crypto: ccp: Add Platform Security Processor (PSP) device support Brijesh Singh
2017-07-25 8:29 ` Kamil Konieczny
@ 2017-09-06 17:00 ` Borislav Petkov
2017-09-06 20:38 ` Brijesh Singh
2017-09-07 14:27 ` Borislav Petkov
2 siblings, 1 reply; 22+ messages in thread
From: Borislav Petkov @ 2017-09-06 17:00 UTC (permalink / raw)
To: Brijesh Singh
Cc: linux-kernel, x86, kvm, Thomas Gleixner, Joerg Roedel,
Michael S . Tsirkin, Paolo Bonzini,
\"Radim Krčmář\",
Tom Lendacky, Herbert Xu, David S . Miller, Gary Hook,
linux-crypto
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.
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC Part2 PATCH v3 02/26] crypto: ccp: Add Platform Security Processor (PSP) device support
2017-09-06 17:00 ` Borislav Petkov
@ 2017-09-06 20:38 ` Brijesh Singh
2017-09-06 20:46 ` Borislav Petkov
0 siblings, 1 reply; 22+ messages in thread
From: Brijesh Singh @ 2017-09-06 20:38 UTC (permalink / raw)
To: Borislav Petkov
Cc: brijesh.singh, linux-kernel, x86, kvm, Thomas Gleixner,
Joerg Roedel, Michael S . Tsirkin, Paolo Bonzini,
\"Radim Krčmář\",
Tom Lendacky, Herbert Xu, David S . Miller, Gary Hook,
linux-crypto
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.
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC Part2 PATCH v3 02/26] crypto: ccp: Add Platform Security Processor (PSP) device support
2017-09-06 20:38 ` Brijesh Singh
@ 2017-09-06 20:46 ` Borislav Petkov
2017-09-06 21:26 ` Gary R Hook
0 siblings, 1 reply; 22+ messages in thread
From: Borislav Petkov @ 2017-09-06 20:46 UTC (permalink / raw)
To: Brijesh Singh
Cc: linux-kernel, x86, kvm, Thomas Gleixner, Joerg Roedel,
Michael S . Tsirkin, Paolo Bonzini,
\"Radim Krčmář\",
Tom Lendacky, Herbert Xu, David S . Miller, Gary Hook,
linux-crypto
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.
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC Part2 PATCH v3 02/26] crypto: ccp: Add Platform Security Processor (PSP) device support
2017-09-06 20:46 ` Borislav Petkov
@ 2017-09-06 21:26 ` Gary R Hook
2017-09-07 10:34 ` Borislav Petkov
0 siblings, 1 reply; 22+ messages in thread
From: Gary R Hook @ 2017-09-06 21:26 UTC (permalink / raw)
To: Borislav Petkov, Brijesh Singh
Cc: linux-kernel, x86, kvm, Thomas Gleixner, Joerg Roedel,
Michael S . Tsirkin, Paolo Bonzini,
\"Radim Krčmář\",
Tom Lendacky, Herbert Xu, David S . Miller, linux-crypto
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.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC Part2 PATCH v3 02/26] crypto: ccp: Add Platform Security Processor (PSP) device support
2017-09-06 21:26 ` Gary R Hook
@ 2017-09-07 10:34 ` Borislav Petkov
0 siblings, 0 replies; 22+ messages in thread
From: Borislav Petkov @ 2017-09-07 10:34 UTC (permalink / raw)
To: Gary R Hook
Cc: Brijesh Singh, linux-kernel, x86, kvm, Thomas Gleixner,
Joerg Roedel, Michael S . Tsirkin, Paolo Bonzini,
\"Radim Krčmář\",
Tom Lendacky, Herbert Xu, David S . Miller, linux-crypto
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.
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC Part2 PATCH v3 02/26] crypto: ccp: Add Platform Security Processor (PSP) device support
2017-07-24 20:02 ` [RFC Part2 PATCH v3 02/26] crypto: ccp: Add Platform Security Processor (PSP) device support Brijesh Singh
2017-07-25 8:29 ` Kamil Konieczny
2017-09-06 17:00 ` Borislav Petkov
@ 2017-09-07 14:27 ` Borislav Petkov
2017-09-07 22:19 ` Brijesh Singh
2 siblings, 1 reply; 22+ messages in thread
From: Borislav Petkov @ 2017-09-07 14:27 UTC (permalink / raw)
To: Brijesh Singh
Cc: linux-kernel, x86, kvm, Thomas Gleixner, Joerg Roedel,
Michael S . Tsirkin, Paolo Bonzini,
\"Radim Krčmář\",
Tom Lendacky, Herbert Xu, David S . Miller, Gary Hook,
linux-crypto
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.
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC Part2 PATCH v3 02/26] crypto: ccp: Add Platform Security Processor (PSP) device support
2017-09-07 14:27 ` Borislav Petkov
@ 2017-09-07 22:19 ` Brijesh Singh
2017-09-07 23:15 ` Gary R Hook
2017-09-08 8:40 ` Borislav Petkov
0 siblings, 2 replies; 22+ messages in thread
From: Brijesh Singh @ 2017-09-07 22:19 UTC (permalink / raw)
To: Borislav Petkov
Cc: brijesh.singh, linux-kernel, x86, kvm, Thomas Gleixner,
Joerg Roedel, Michael S . Tsirkin, Paolo Bonzini,
\"Radim Krčmář\",
Tom Lendacky, Herbert Xu, David S . Miller, Gary Hook,
linux-crypto
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC Part2 PATCH v3 02/26] crypto: ccp: Add Platform Security Processor (PSP) device support
2017-09-07 22:19 ` Brijesh Singh
@ 2017-09-07 23:15 ` Gary R Hook
2017-09-08 8:22 ` Borislav Petkov
2017-09-08 8:40 ` Borislav Petkov
1 sibling, 1 reply; 22+ messages in thread
From: Gary R Hook @ 2017-09-07 23:15 UTC (permalink / raw)
To: Brijesh Singh, Borislav Petkov
Cc: linux-kernel, x86, kvm, Thomas Gleixner, Joerg Roedel,
Michael S . Tsirkin, Paolo Bonzini,
\"Radim Krčmář\",
Tom Lendacky, Herbert Xu, David S . Miller, linux-crypto
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC Part2 PATCH v3 02/26] crypto: ccp: Add Platform Security Processor (PSP) device support
2017-09-07 23:15 ` Gary R Hook
@ 2017-09-08 8:22 ` Borislav Petkov
0 siblings, 0 replies; 22+ messages in thread
From: Borislav Petkov @ 2017-09-08 8:22 UTC (permalink / raw)
To: Gary R Hook
Cc: Brijesh Singh, linux-kernel, x86, kvm, Thomas Gleixner,
Joerg Roedel, Michael S . Tsirkin, Paolo Bonzini,
\"Radim Krčmář\",
Tom Lendacky, Herbert Xu, David S . Miller, linux-crypto
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... :)
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC Part2 PATCH v3 02/26] crypto: ccp: Add Platform Security Processor (PSP) device support
2017-09-07 22:19 ` Brijesh Singh
2017-09-07 23:15 ` Gary R Hook
@ 2017-09-08 8:40 ` Borislav Petkov
2017-09-08 13:54 ` Brijesh Singh
2017-09-08 16:06 ` Brijesh Singh
1 sibling, 2 replies; 22+ messages in thread
From: Borislav Petkov @ 2017-09-08 8:40 UTC (permalink / raw)
To: Brijesh Singh
Cc: linux-kernel, x86, kvm, Thomas Gleixner, Joerg Roedel,
Michael S . Tsirkin, Paolo Bonzini,
\"Radim Krčmář\",
Tom Lendacky, Herbert Xu, David S . Miller, Gary Hook,
linux-crypto
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...
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC Part2 PATCH v3 02/26] crypto: ccp: Add Platform Security Processor (PSP) device support
2017-09-08 8:40 ` Borislav Petkov
@ 2017-09-08 13:54 ` Brijesh Singh
2017-09-08 16:06 ` Brijesh Singh
1 sibling, 0 replies; 22+ messages in thread
From: Brijesh Singh @ 2017-09-08 13:54 UTC (permalink / raw)
To: Borislav Petkov
Cc: brijesh.singh, linux-kernel, x86, kvm, Thomas Gleixner,
Joerg Roedel, Michael S . Tsirkin, Paolo Bonzini,
\"Radim Krčmář\",
Tom Lendacky, Herbert Xu, David S . Miller, Gary Hook,
linux-crypto
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC Part2 PATCH v3 02/26] crypto: ccp: Add Platform Security Processor (PSP) device support
2017-09-08 8:40 ` Borislav Petkov
2017-09-08 13:54 ` Brijesh Singh
@ 2017-09-08 16:06 ` Brijesh Singh
1 sibling, 0 replies; 22+ messages in thread
From: Brijesh Singh @ 2017-09-08 16:06 UTC (permalink / raw)
To: Borislav Petkov
Cc: brijesh.singh, linux-kernel, x86, kvm, Thomas Gleixner,
Joerg Roedel, Michael S . Tsirkin, Paolo Bonzini,
\"Radim Krčmář\",
Tom Lendacky, Herbert Xu, David S . Miller, Gary Hook,
linux-crypto
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC Part2 PATCH v3 03/26] crypto: ccp: Add Secure Encrypted Virtualization (SEV) device support
2017-07-24 20:02 ` [RFC Part2 PATCH v3 03/26] crypto: ccp: Add Secure Encrypted Virtualization (SEV) " Brijesh Singh
@ 2017-09-12 14:02 ` Borislav Petkov
2017-09-12 15:32 ` Brijesh Singh
2017-09-13 14:17 ` Borislav Petkov
1 sibling, 1 reply; 22+ messages in thread
From: Borislav Petkov @ 2017-09-12 14:02 UTC (permalink / raw)
To: Brijesh Singh
Cc: linux-kernel, x86, kvm, Thomas Gleixner, Joerg Roedel,
Michael S . Tsirkin, Paolo Bonzini,
\"Radim Krčmář\",
Tom Lendacky, Herbert Xu, David S . Miller, Gary Hook,
linux-crypto
Just a cursory review: more indepth after the redesign.
On Mon, Jul 24, 2017 at 03:02:40PM -0500, Brijesh Singh wrote:
> AMDs new Secure Encrypted Virtualization (SEV) feature allows the memory
> contents of a virtual machine to be transparently encrypted with a key
> unique to the guest VM. The programming and management of the encryption
> keys are handled by the AMD Secure Processor (AMD-SP), which exposes the
> commands for these tasks. The complete spec for various commands are
s/ for various commands are/is/
> available at:
> http://support.amd.com/TechDocs/55766_SEV-KM%20API_Specification.pdf
>
> This patch extends AMD-SP driver to provide:
Never say "This patch" in a commit message of a patch. It is
tautologically useless.
> - a in-kernel APIs to communicate with SEV device. The APIs can be used
s/a/an/ with a SEV ...
> by the hypervisor to create encryption context for the SEV guests.
>
> - a userspace IOCTL to manage the platform certificates etc
>
> 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 | 10 +
> drivers/crypto/ccp/Makefile | 1 +
> drivers/crypto/ccp/psp-dev.c | 4 +
> drivers/crypto/ccp/psp-dev.h | 27 ++
> drivers/crypto/ccp/sev-dev.c | 416 ++++++++++++++++++++++++++
> drivers/crypto/ccp/sev-dev.h | 67 +++++
> drivers/crypto/ccp/sev-ops.c | 457 +++++++++++++++++++++++++++++
> drivers/crypto/ccp/sp-pci.c | 2 +-
> include/linux/psp-sev.h | 683 +++++++++++++++++++++++++++++++++++++++++++
> include/uapi/linux/psp-sev.h | 110 +++++++
> 10 files changed, 1776 insertions(+), 1 deletion(-)
> create mode 100644 drivers/crypto/ccp/sev-dev.c
> create mode 100644 drivers/crypto/ccp/sev-dev.h
> create mode 100644 drivers/crypto/ccp/sev-ops.c
> create mode 100644 include/linux/psp-sev.h
> create mode 100644 include/uapi/linux/psp-sev.h
>
> diff --git a/drivers/crypto/ccp/Kconfig b/drivers/crypto/ccp/Kconfig
> index 41c0ff5..ae0ff1c 100644
> --- a/drivers/crypto/ccp/Kconfig
> +++ b/drivers/crypto/ccp/Kconfig
> @@ -40,3 +40,13 @@ config CRYPTO_DEV_SP_PSP
> Provide the support for AMD Platform Security Processor (PSP) device
> which can be used for communicating with Secure Encryption Virtualization
> (SEV) firmware.
> +
> +config CRYPTO_DEV_PSP_SEV
> + bool "Secure Encrypted Virtualization (SEV) interface"
> + default y
> + depends on CRYPTO_DEV_CCP_DD
> + depends on CRYPTO_DEV_SP_PSP
> + help
> + Provide the kernel and userspace (/dev/sev) interface to communicate with
> + Secure Encrypted Virtualization (SEV) firmware running inside AMD Platform
> + Security Processor (PSP)
> diff --git a/drivers/crypto/ccp/Makefile b/drivers/crypto/ccp/Makefile
> index 8aae4ff..94ca748 100644
> --- a/drivers/crypto/ccp/Makefile
> +++ b/drivers/crypto/ccp/Makefile
> @@ -8,6 +8,7 @@ ccp-$(CONFIG_CRYPTO_DEV_SP_CCP) += ccp-dev.o \
> ccp-debugfs.o
> ccp-$(CONFIG_PCI) += sp-pci.o
> ccp-$(CONFIG_CRYPTO_DEV_SP_PSP) += psp-dev.o
> +ccp-$(CONFIG_CRYPTO_DEV_PSP_SEV) += sev-dev.o sev-ops.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
> index bb0ea9a..0c9d25c 100644
> --- a/drivers/crypto/ccp/psp-dev.c
> +++ b/drivers/crypto/ccp/psp-dev.c
> @@ -97,6 +97,7 @@ irqreturn_t psp_irq_handler(int irq, void *data)
> static int psp_init(struct psp_device *psp)
> {
> psp_add_device(psp);
> + sev_dev_init(psp);
>
> return 0;
> }
> @@ -166,17 +167,20 @@ void psp_dev_destroy(struct sp_device *sp)
> struct psp_device *psp = sp->psp_data;
>
> sp_free_psp_irq(sp, psp);
> + sev_dev_destroy(psp);
>
> psp_del_device(psp);
> }
>
> int psp_dev_resume(struct sp_device *sp)
> {
> + sev_dev_resume(sp->psp_data);
> return 0;
> }
>
> int psp_dev_suspend(struct sp_device *sp, pm_message_t state)
> {
> + sev_dev_suspend(sp->psp_data, state);
> return 0;
> }
>
> diff --git a/drivers/crypto/ccp/psp-dev.h b/drivers/crypto/ccp/psp-dev.h
> index 6e167b8..9334d87 100644
> --- a/drivers/crypto/ccp/psp-dev.h
> +++ b/drivers/crypto/ccp/psp-dev.h
> @@ -78,5 +78,32 @@ 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;
> +#ifdef CONFIG_CRYPTO_DEV_PSP_SEV
> +
> +int sev_dev_init(struct psp_device *psp);
> +void sev_dev_destroy(struct psp_device *psp);
> +int sev_dev_resume(struct psp_device *psp);
> +int sev_dev_suspend(struct psp_device *psp, pm_message_t state);
> +
> +#else /* !CONFIG_CRYPTO_DEV_PSP_SEV */
> +
> +static inline int sev_dev_init(struct psp_device *psp)
> +{
> + return -ENODEV;
> +}
> +
> +static inline void sev_dev_destroy(struct psp_device *psp) { }
> +
> +static inline int sev_dev_resume(struct psp_device *psp)
> +{
> + return -ENODEV;
> +}
> +
> +static inline int sev_dev_suspend(struct psp_device *psp, pm_message_t state)
> +{
> + return -ENODEV;
> +}
> +
> +#endif /* CONFIG_CRYPTO_DEV_PSP_SEV */
>
> #endif /* __PSP_DEV_H */
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> new file mode 100644
> index 0000000..a2b41dd
> --- /dev/null
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -0,0 +1,416 @@
> +/*
> + * AMD Secure Encrypted Virtualization (SEV) interface
> + *
> + * Copyright (C) 2016-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.
> + */
> +
> +#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/wait.h>
> +#include <linux/jiffies.h>
> +
> +#include "psp-dev.h"
> +#include "sev-dev.h"
> +
> +extern const struct file_operations sev_fops;
> +
> +static LIST_HEAD(sev_devs);
> +static DEFINE_SPINLOCK(sev_devs_lock);
> +static atomic_t sev_id;
> +
> +static unsigned int sev_poll;
> +module_param(sev_poll, uint, 0444);
> +MODULE_PARM_DESC(sev_poll, "Poll for sev command completion - any non-zero value");
> +
> +DEFINE_MUTEX(sev_cmd_mutex);
Please audit all those drivers after you redesign them and check for
variables and functions which are not static but used in a single
compilation unit. Like this one, for example.
<... skipping over the rest, will look at it in the next version ...>
...
> +static int sev_cmd_buffer_len(int cmd)
> +{
> + int size;
> +
> + switch (cmd) {
> + case SEV_CMD_INIT:
> + size = sizeof(struct sev_data_init);
> + break;
You could make that more tabular like this:
case SEV_CMD_INIT: return sizeof(struct sev_data_init);
case SEV_CMD_PLATFORM_STATUS: return sizeof(struct sev_data_status);
case SEV_CMD_PEK_CSR: return sizeof(struct sev_data_pek_csr);
...
which should make it more readable.
But looking at this more, this is a static mapping between the commands
and the corresponding struct sizes and you use it in
print_hex_dump_debug("(in): ", DUMP_PREFIX_OFFSET, 16, 2, data,
sev_cmd_buffer_len(cmd), false);
But then, I don't see what that brings you because you're not dumping
the actual @data length but the *expected* data length based on the
command type.
And *that* you can look up in the manual and do not need it in code,
enlarging the driver unnecessarily.
...
> +int sev_platform_init(struct sev_data_init *data, int *error)
> +{
> + return sev_issue_cmd(SEV_CMD_INIT, data, error);
> +}
> +EXPORT_SYMBOL_GPL(sev_platform_init);
> +
> +int sev_platform_shutdown(int *error)
> +{
> + return sev_issue_cmd(SEV_CMD_SHUTDOWN, 0, error);
> +}
> +EXPORT_SYMBOL_GPL(sev_platform_shutdown);
All those could be static inlines in a header instead of separate
symbols.
> +int sev_issue_cmd(int cmd, void *data, int *psp_ret)
> +{
> + struct sev_device *sev = get_sev_master_device();
> + unsigned int phys_lsb, phys_msb;
> + unsigned int reg, ret;
> +
> + if (!sev)
> + return -ENODEV;
> +
> + if (psp_ret)
> + *psp_ret = 0;
> +
> + /* Set the physical address for the PSP */
> + phys_lsb = data ? lower_32_bits(__psp_pa(data)) : 0;
> + phys_msb = data ? upper_32_bits(__psp_pa(data)) : 0;
> +
> + dev_dbg(sev->dev, "sev command id %#x buffer 0x%08x%08x\n",
> + cmd, phys_msb, phys_lsb);
> + print_hex_dump_debug("(in): ", DUMP_PREFIX_OFFSET, 16, 2, data,
> + sev_cmd_buffer_len(cmd), false);
> +
> + /* Only one command at a time... */
> + mutex_lock(&sev_cmd_mutex);
> +
> + iowrite32(phys_lsb, sev->io_regs + PSP_CMDBUFF_ADDR_LO);
> + iowrite32(phys_msb, sev->io_regs + PSP_CMDBUFF_ADDR_HI);
> + wmb();
WARNING: memory barrier without comment
#503: FILE: drivers/crypto/ccp/sev-dev.c:339:
+ wmb();
Again:
Please integrate scripts/checkpatch.pl into your patch creation
workflow. Some of the warnings/errors *actually* make sense.
> diff --git a/drivers/crypto/ccp/sev-dev.h b/drivers/crypto/ccp/sev-dev.h
> new file mode 100644
> index 0000000..0a8ce08
> --- /dev/null
> +++ b/drivers/crypto/ccp/sev-dev.h
> @@ -0,0 +1,67 @@
> +/*
> + * AMD Secure Encrypted Virtualization (SEV) interface
> + *
> + * Copyright (C) 2016-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 __SEV_DEV_H__
> +#define __SEV_DEV_H__
> +
> +#include <linux/device.h>
> +#include <linux/spinlock.h>
> +#include <linux/mutex.h>
> +#include <linux/list.h>
> +#include <linux/wait.h>
> +#include <linux/interrupt.h>
> +#include <linux/irqreturn.h>
> +#include <linux/miscdevice.h>
> +
> +#include <linux/psp-sev.h>
> +
> +#define PSP_C2PMSG(_num) ((_num) << 2)
> +#define PSP_CMDRESP PSP_C2PMSG(32)
> +#define PSP_CMDBUFF_ADDR_LO PSP_C2PMSG(56)
> +#define PSP_CMDBUFF_ADDR_HI PSP_C2PMSG(57)
> +#define PSP_FEATURE_REG PSP_C2PMSG(63)
> +
> +#define PSP_P2CMSG(_num) (_num << 2)
> +#define PSP_CMD_COMPLETE_REG 1
> +#define PSP_CMD_COMPLETE PSP_P2CMSG(PSP_CMD_COMPLETE_REG)
> +
> +#define MAX_PSP_NAME_LEN 16
> +#define SEV_DEFAULT_TIMEOUT 5
> +
> +struct sev_device {
> + struct list_head entry;
> +
> + struct dentry *debugfs;
> + struct miscdevice misc;
> +
> + unsigned int id;
> + char name[MAX_PSP_NAME_LEN];
> +
> + struct device *dev;
> + struct sp_device *sp;
> + struct psp_device *psp;
> +
> + void __iomem *io_regs;
> +
> + unsigned int int_rcvd;
> + wait_queue_head_t int_queue;
> +};
> +
> +void sev_add_device(struct sev_device *sev);
> +void sev_del_device(struct sev_device *sev);
> +
> +int sev_ops_init(struct sev_device *sev);
> +void sev_ops_destroy(struct sev_device *sev);
> +
> +int sev_issue_cmd(int cmd, void *data, int *error);
> +
> +#endif /* __SEV_DEV_H */
> diff --git a/drivers/crypto/ccp/sev-ops.c b/drivers/crypto/ccp/sev-ops.c
> new file mode 100644
> index 0000000..a13d857
> --- /dev/null
> +++ b/drivers/crypto/ccp/sev-ops.c
> @@ -0,0 +1,457 @@
> +/*
> + * AMD Secure Encrypted Virtualization (SEV) command interface
> + *
> + * Copyright (C) 2016-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.
> + */
> +
> +#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/uaccess.h>
> +
> +#include <uapi/linux/psp-sev.h>
> +
> +#include "psp-dev.h"
> +#include "sev-dev.h"
> +
> +static bool sev_initialized;
Variables like this one are a good example that the design is not
optimal. sev-ops should all be in psp-dev and then you don't need all those
registrations and ops passing around and so on... But you get the idea...
> +static int sev_platform_get_state(int *state, int *error)
> +{
> + int ret;
> + struct sev_data_status *data;
> +
> + data = kzalloc(sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + ret = sev_platform_status(data, error);
> + *state = data->state;
> +
> + kfree(data);
> + return ret;
> +}
> +
> +static int __sev_platform_init(int *error)
> +{
> + int ret;
> + struct sev_data_init *data;
> +
> + data = kzalloc(sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + ret = sev_platform_init(data, error);
It is usually the other way around: the function without the "__" calls
the "__" variant.
> +
> + kfree(data);
> + return ret;
> +}
> +
> +static int sev_ioctl_factory_reset(struct sev_issue_cmd *argp)
> +{
> + return sev_issue_cmd(SEV_CMD_FACTORY_RESET, 0, &argp->error);
> +}
static inline in a header.
> +
> +static int sev_ioctl_platform_status(struct sev_issue_cmd *argp)
> +{
> + int ret;
> + struct sev_data_status *data;
> +
> + data = kzalloc(sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + ret = sev_platform_status(data, &argp->error);
> +
> + if (copy_to_user((void *)argp->data, data, sizeof(*data)))
> + ret = -EFAULT;
> +
> + kfree(data);
> + return ret;
> +}
> +
> +static int sev_ioctl_pek_csr(struct sev_issue_cmd *argp)
> +{
> + int do_shutdown = 0;
> + int ret, state, error;
> + void *csr_addr = NULL;
> + struct sev_data_pek_csr *data;
> + struct sev_user_data_pek_csr input;
> +
> + if (copy_from_user(&input, (void *)argp->data,
> + sizeof(struct sev_user_data_pek_csr)))
> + return -EFAULT;
> +
> + data = kzalloc(sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + /*
> + * PEK_CSR command can be issued when firmware is in INIT or WORKING
> + * state. If firmware is in UNINIT state then we transition into INIT
> + * state and issue the command.
> + */
> + ret = sev_platform_get_state(&state, &argp->error);
> + if (ret)
> + return ret;
> +
> + if (state == SEV_STATE_UNINIT) {
> + /* transition the plaform into INIT state */
> + ret = __sev_platform_init(&argp->error);
> + if (ret)
> + return ret;
> + do_shutdown = 1;
> + }
> +
> + if (input.address) {
> + csr_addr = kmalloc(input.length, GFP_KERNEL);
> + if (!csr_addr) {
> + ret = -ENOMEM;
> + goto e_free;
> + }
> + data->address = __psp_pa(csr_addr);
> + data->length = input.length;
> + }
> +
> + ret = sev_issue_cmd(SEV_CMD_PEK_CSR, data, &argp->error);
> +
> + if (csr_addr) {
> + if (copy_to_user((void *)input.address, csr_addr,
> + input.length)) {
> + ret = -EFAULT;
> + goto e_free;
> + }
> + }
> +
> + input.length = data->length;
> + if (copy_to_user((void *)argp->data, &input,
> + sizeof(struct sev_user_data_pek_csr)))
> + ret = -EFAULT;
> +e_free:
> + if (do_shutdown)
> + sev_platform_shutdown(&error);
Who's looking at that error?
No one, AFAICT. It is a stack variable which gets destroyed after this
function returns. The other call sites look the same. Please fix.
> + kfree(csr_addr);
> + kfree(data);
> + return ret;
> +}
> +
> +static int sev_ioctl_pdh_gen(struct sev_issue_cmd *argp)
> +{
> + int ret, state, error, do_shutdown = 0;
> +
> + /*
> + * PDH_GEN command can be issued when platform is in INIT or WORKING
> + * state. If we are in UNINIT state then transition into INIT.
> + */
> + ret = sev_platform_get_state(&state, &argp->error);
> + if (ret)
> + return ret;
> +
> + if (state == SEV_STATE_UNINIT) {
> + /* transition the plaform into INIT state */
> + ret = __sev_platform_init(&argp->error);
> + if (ret)
> + return ret;
> + do_shutdown = 1;
> + }
> +
> + ret = sev_issue_cmd(SEV_CMD_PDH_GEN, 0, &argp->error);
> + if (do_shutdown)
> + sev_platform_shutdown(&error);
> + return ret;
> +}
> +
> +static int sev_ioctl_pek_gen(struct sev_issue_cmd *argp)
> +{
> + int do_shutdown = 0;
> + int error, ret, state;
> +
> + /*
> + * PEK_GEN command can be issued only when firmware is in INIT state.
> + * If firmware is in UNINIT state then we transition into INIT state
> + * and issue the command and then shutdown.
> + */
> + ret = sev_platform_get_state(&state, &argp->error);
> + if (ret)
> + return ret;
> +
> + if (state == SEV_STATE_UNINIT) {
> + /* transition the plaform into INIT state */
> + ret = __sev_platform_init(&argp->error);
> + if (ret)
> + return ret;
> +
> + do_shutdown = 1;
> + }
> +
> + ret = sev_issue_cmd(SEV_CMD_PEK_GEN, 0, &argp->error);
> +
> + if (do_shutdown)
> + sev_platform_shutdown(&error);
> + return ret;
> +}
> +
> +static int sev_ioctl_pek_cert_import(struct sev_issue_cmd *argp)
> +{
> + int ret, state, error, do_shutdown = 0;
> + struct sev_data_pek_cert_import *data;
> + struct sev_user_data_pek_cert_import input;
> + void *pek_cert = NULL, *oca_cert = NULL;
> +
> + if (copy_from_user(&input, (void *)argp->data, sizeof(*data)))
> + return -EFAULT;
> +
> + if (!input.pek_cert_address || !input.pek_cert_length ||
> + !input.oca_cert_address || !input.oca_cert_length)
> + return -EINVAL;
Here and everywhere else: please audit all those copy_from_user() calls
for user-controlled fields and the such and conservatively sanity-check
them. ou don't want to have security bugs in this driver!
You need to massage all those user values into sanity.
> + ret = sev_platform_get_state(&state, &argp->error);
> + if (ret)
> + return ret;
> +
> + /*
> + * CERT_IMPORT command can be issued only when platform is in INIT
> + * state. If we are in UNINIT state then transition into INIT state
> + * and issue the command.
> + */
> + if (state == SEV_STATE_UNINIT) {
> + /* transition platform init INIT state */
> + ret = __sev_platform_init(&argp->error);
> + if (ret)
> + return ret;
> + do_shutdown = 1;
> + }
> +
> + data = kzalloc(sizeof(*data), GFP_KERNEL);
> + if (!data) {
> + ret = -ENOMEM;
> + goto e_free;
> + }
> +
> + pek_cert = kmalloc(input.pek_cert_length, GFP_KERNEL);
> + if (!pek_cert) {
> + ret = -ENOMEM;
> + goto e_free;
> + }
> +
> + /* copy PEK certificate from userspace */
> + if (copy_from_user(pek_cert, (void *)input.pek_cert_address,
> + input.pek_cert_length)) {
> + ret = -EFAULT;
> + goto e_free;
> + }
> +
> + data->pek_cert_address = __psp_pa(pek_cert);
> + data->pek_cert_length = input.pek_cert_length;
> +
> + oca_cert = kmalloc(input.oca_cert_length, GFP_KERNEL);
There's your first overrun: that oca_cert_length you copy from userspace
and check only if it is 0 but it can be huuge.
> + if (!oca_cert) {
> + ret = -ENOMEM;
> + goto e_free;
> + }
> +
> + /* copy OCA certificate from userspace */
> + if (copy_from_user(oca_cert, (void *)input.oca_cert_address,
... and here's the user-controlled address where you copy the exploit
code from. You need to revisit all those copy_from_user() calls and be
absolutely defensive here.
> + input.oca_cert_length)) {
> + ret = -EFAULT;
> + goto e_free;
> + }
> +
> + data->oca_cert_address = __psp_pa(oca_cert);
> + data->oca_cert_length = input.oca_cert_length;
> +
> + ret = sev_issue_cmd(SEV_CMD_PEK_CERT_IMPORT, data, &argp->error);
> +e_free:
> + if (do_shutdown)
> + sev_platform_shutdown(&error);
> + kfree(oca_cert);
> + kfree(pek_cert);
> + kfree(data);
> + return ret;
> +}
> +
> +static int sev_ioctl_pdh_cert_export(struct sev_issue_cmd *argp)
> +{
> + int ret, state, error, need_shutdown = 0;
> + struct sev_data_pdh_cert_export *data;
> + struct sev_user_data_pdh_cert_export input;
> + void *pdh_cert = NULL, *cert_chain = NULL;
> +
> + if (copy_from_user(&input, (void *)argp->data, sizeof(*data)))
> + return -EFAULT;
> +
> + /*
> + * CERT_EXPORT command can be issued in INIT or WORKING state.
> + * If we are in UNINIT state then transition into INIT state and
> + * shutdown before exiting. But if platform is in WORKING state
> + * then EXPORT the certificate but do not shutdown the platform.
> + */
> + ret = sev_platform_get_state(&state, &argp->error);
> + if (ret)
> + return ret;
> +
> + if (state == SEV_STATE_UNINIT) {
> + ret = __sev_platform_init(&argp->error);
> + if (ret)
> + return ret;
> + need_shutdown = 1;
> + }
> +
> + data = kzalloc(sizeof(*data), GFP_KERNEL);
> + if (!data) {
> + ret = -ENOMEM;
> + goto e_free;
> + }
> +
> + if (input.pdh_cert_address) {
Oh wow, so that address is absolutely unchecked.
> + pdh_cert = kmalloc(input.pdh_cert_length, GFP_KERNEL);
> + if (!pdh_cert) {
> + ret = -ENOMEM;
> + goto e_free;
> + }
> +
> + data->pdh_cert_address = __psp_pa(pdh_cert);
> + data->pdh_cert_length = input.pdh_cert_length;
> + }
> +
> + if (input.cert_chain_address) {
Ditto.
And so on and so on...
> + cert_chain = kmalloc(input.cert_chain_length, GFP_KERNEL);
> + if (!cert_chain) {
> + ret = -ENOMEM;
> + goto e_free;
> + }
> +
> + data->cert_chain_address = __psp_pa(cert_chain);
> + data->cert_chain_length = input.cert_chain_length;
> + }
> +
> + ret = sev_issue_cmd(SEV_CMD_PDH_CERT_EXPORT, data, &argp->error);
> +
> + input.cert_chain_length = data->cert_chain_length;
> + input.pdh_cert_length = data->pdh_cert_length;
> +
> + /* copy PDH certificate to userspace */
> + if (pdh_cert) {
> + if (copy_to_user((void *)input.pdh_cert_address,
> + pdh_cert, input.pdh_cert_length)) {
> + ret = -EFAULT;
> + goto e_free;
> + }
> + }
> +
> + /* copy certificate chain to userspace */
> + if (cert_chain) {
> + if (copy_to_user((void *)input.cert_chain_address,
> + cert_chain, input.cert_chain_length)) {
> + ret = -EFAULT;
> + goto e_free;
> + }
> + }
> +
> + /* copy certificate length to userspace */
> + if (copy_to_user((void *)argp->data, &input,
> + sizeof(struct sev_user_data_pdh_cert_export)))
> + ret = -EFAULT;
> +
> +e_free:
> + if (need_shutdown)
> + sev_platform_shutdown(&error);
> +
> + kfree(cert_chain);
> + kfree(pdh_cert);
> + kfree(data);
> + return ret;
> +}
> +
> +static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
> +{
> + int ret = -EFAULT;
> + void __user *argp = (void __user *)arg;
> + struct sev_issue_cmd input;
> +
> + if (ioctl != SEV_ISSUE_CMD)
> + return -EINVAL;
> +
> + if (copy_from_user(&input, argp, sizeof(struct sev_issue_cmd)))
> + return -EFAULT;
> +
> + if (input.cmd > SEV_CMD_MAX)
> + return -EINVAL;
> +
> + switch (input.cmd) {
> +
> + case SEV_USER_CMD_FACTORY_RESET: {
> + ret = sev_ioctl_factory_reset(&input);
> + break;
> + }
> + case SEV_USER_CMD_PLATFORM_STATUS: {
> + ret = sev_ioctl_platform_status(&input);
> + break;
> + }
> + case SEV_USER_CMD_PEK_GEN: {
> + ret = sev_ioctl_pek_gen(&input);
> + break;
> + }
> + case SEV_USER_CMD_PDH_GEN: {
> + ret = sev_ioctl_pdh_gen(&input);
> + break;
> + }
> + case SEV_USER_CMD_PEK_CSR: {
> + ret = sev_ioctl_pek_csr(&input);
> + break;
> + }
> + case SEV_USER_CMD_PEK_CERT_IMPORT: {
> + ret = sev_ioctl_pek_cert_import(&input);
> + break;
> + }
> + case SEV_USER_CMD_PDH_CERT_EXPORT: {
> + ret = sev_ioctl_pdh_cert_export(&input);
> + break;
> + }
> + default:
> + ret = -EINVAL;
> + break;
> + }
> +
> + if (copy_to_user(argp, &input, sizeof(struct sev_issue_cmd)))
> + ret = -EFAULT;
> +
> + return ret;
> +}
> +
> +const struct file_operations sev_fops = {
> + .owner = THIS_MODULE,
> + .unlocked_ioctl = sev_ioctl,
> +};
> +
> +int sev_ops_init(struct sev_device *sev)
> +{
> + struct miscdevice *misc = &sev->misc;
> +
> + /* if sev device is already registered then do nothing */
> + if (sev_initialized)
> + return 0;
> +
> + misc->minor = MISC_DYNAMIC_MINOR;
> + misc->name = sev->name;
> + misc->fops = &sev_fops;
> + sev_initialized = true;
> +
> + return misc_register(misc);
> +}
> +
> +void sev_ops_destroy(struct sev_device *sev)
> +{
> + misc_deregister(&sev->misc);
> +}
> diff --git a/drivers/crypto/ccp/sp-pci.c b/drivers/crypto/ccp/sp-pci.c
> index e58b3ad..20a0f35 100644
> --- a/drivers/crypto/ccp/sp-pci.c
> +++ b/drivers/crypto/ccp/sp-pci.c
> @@ -280,7 +280,7 @@ static const struct sp_dev_vdata dev_vdata[] = {
> #ifdef CONFIG_CRYPTO_DEV_SP_CCP
> .ccp_vdata = &ccpv5a,
> #endif
> -#ifdef CONFIG_CRYPTO_DEV_PSP
> +#ifdef CONFIG_CRYPTO_DEV_SP_PSP
> .psp_vdata = &psp_entry
> #endif
> },
> diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
> new file mode 100644
> index 0000000..1736880
> --- /dev/null
> +++ b/include/linux/psp-sev.h
> @@ -0,0 +1,683 @@
> +/*
> + * AMD Secure Encrypted Virtualization (SEV) driver interface
> + *
> + * Copyright (C) 2016-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.
<--- you probably should have a reference to the document containing all
those commands, i.e.,
http://support.amd.com/TechDocs/55766_SEV-KM%20API_Specification.pdf
Also, for your next submission, please split this huuge patch. Those
definitions can go separately, for example.
> + */
> +
> +#ifndef __PSP_SEV_H__
> +#define __PSP_SEV_H__
> +
> +#ifdef CONFIG_X86
> +#include <linux/mem_encrypt.h>
> +
> +#define __psp_pa(x) __sme_pa(x)
> +#else
> +#define __psp_pa(x) __pa(x)
> +#endif
> +
> +/**
> + * SEV platform state
> + */
> +enum sev_state {
> + SEV_STATE_UNINIT = 0x0,
> + SEV_STATE_INIT = 0x1,
> + SEV_STATE_WORKING = 0x2,
> +
> + SEV_STATE_MAX
> +};
> +
> +/**
> + * SEV platform and guest management commands
> + */
> +enum sev_cmd {
> + /* platform commands */
> + SEV_CMD_INIT = 0x001,
> + SEV_CMD_SHUTDOWN = 0x002,
> + SEV_CMD_FACTORY_RESET = 0x003,
> + SEV_CMD_PLATFORM_STATUS = 0x004,
> + SEV_CMD_PEK_GEN = 0x005,
> + SEV_CMD_PEK_CSR = 0x006,
> + SEV_CMD_PEK_CERT_IMPORT = 0x007,
> + SEV_CMD_PDH_CERT_EXPORT = 0x008,
> + SEV_CMD_PDH_GEN = 0x009,
> + SEV_CMD_DF_FLUSH = 0x00A,
> +
> + /* Guest commands */
> + SEV_CMD_DECOMMISSION = 0x020,
> + SEV_CMD_ACTIVATE = 0x021,
> + SEV_CMD_DEACTIVATE = 0x022,
> + SEV_CMD_GUEST_STATUS = 0x023,
> +
> + /* Guest launch commands */
> + SEV_CMD_LAUNCH_START = 0x030,
> + SEV_CMD_LAUNCH_UPDATE_DATA = 0x031,
> + SEV_CMD_LAUNCH_UPDATE_VMSA = 0x032,
> + SEV_CMD_LAUNCH_MEASURE = 0x033,
> + SEV_CMD_LAUNCH_UPDATE_SECRET = 0x034,
> + SEV_CMD_LAUNCH_FINISH = 0x035,
> +
> + /* Guest migration commands (outgoing) */
> + SEV_CMD_SEND_START = 0x040,
> + SEV_CMD_SEND_UPDATE_DATA = 0x041,
> + SEV_CMD_SEND_UPDATE_VMSA = 0x042,
> + SEV_CMD_SEND_FINISH = 0x043,
> +
> + /* Guest migration commands (incoming) */
> + SEV_CMD_RECEIVE_START = 0x050,
> + SEV_CMD_RECEIVE_UPDATE_DATA = 0x051,
> + SEV_CMD_RECEIVE_UPDATE_VMSA = 0x052,
> + SEV_CMD_RECEIVE_FINISH = 0x053,
> +
> + /* Guest debug commands */
> + SEV_CMD_DBG_DECRYPT = 0x060,
> + SEV_CMD_DBG_ENCRYPT = 0x061,
> +
> + SEV_CMD_MAX,
> +};
> +
> +/**
> + * status code returned by the commands
> + */
> +enum psp_ret_code {
> + SEV_RET_SUCCESS = 0,
> + SEV_RET_INVALID_PLATFORM_STATE,
> + SEV_RET_INVALID_GUEST_STATE,
> + SEV_RET_INAVLID_CONFIG,
> + SEV_RET_INVALID_LENGTH,
> + SEV_RET_ALREADY_OWNED,
> + SEV_RET_INVALID_CERTIFICATE,
> + SEV_RET_POLICY_FAILURE,
> + SEV_RET_INACTIVE,
> + SEV_RET_INVALID_ADDRESS,
> + SEV_RET_BAD_SIGNATURE,
> + SEV_RET_BAD_MEASUREMENT,
> + SEV_RET_ASID_OWNED,
> + SEV_RET_INVALID_ASID,
> + SEV_RET_WBINVD_REQUIRED,
> + SEV_RET_DFFLUSH_REQUIRED,
> + SEV_RET_INVALID_GUEST,
> + SEV_RET_INVALID_COMMAND,
> + SEV_RET_ACTIVE,
> + SEV_RET_HWSEV_RET_PLATFORM,
> + SEV_RET_HWSEV_RET_UNSAFE,
> + SEV_RET_UNSUPPORTED,
> + SEV_RET_MAX,
> +};
> +
> +/**
> + * struct sev_data_init - INIT command parameters
> + *
> + * @flags: processing flags
> + * @tmr_address: system physical address used for SEV-ES
> + * @tmr_length: length of tmr_address
> + */
> +struct sev_data_init {
> + __u32 flags; /* In */
> + __u32 reserved; /* In */
> + __u64 tmr_address; /* In */
> + __u32 tmr_length; /* In */
> +};
I guess all those structs should be __packed to avoid the compiler
adding padding.
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC Part2 PATCH v3 03/26] crypto: ccp: Add Secure Encrypted Virtualization (SEV) device support
2017-09-12 14:02 ` Borislav Petkov
@ 2017-09-12 15:32 ` Brijesh Singh
2017-09-12 16:29 ` Borislav Petkov
0 siblings, 1 reply; 22+ messages in thread
From: Brijesh Singh @ 2017-09-12 15:32 UTC (permalink / raw)
To: Borislav Petkov
Cc: brijesh.singh, linux-kernel, x86, kvm, Thomas Gleixner,
Joerg Roedel, Michael S . Tsirkin, Paolo Bonzini,
\"Radim Krčmář\",
Tom Lendacky, Herbert Xu, David S . Miller, Gary Hook,
linux-crypto
Hi Boris,
I will address all your feedback in next rev.
On 09/12/2017 09:02 AM, Borislav Petkov wrote:
...
>
> You could make that more tabular like this:
>
> case SEV_CMD_INIT: return sizeof(struct sev_data_init);
> case SEV_CMD_PLATFORM_STATUS: return sizeof(struct sev_data_status);
> case SEV_CMD_PEK_CSR: return sizeof(struct sev_data_pek_csr);
> ...
>
> which should make it more readable.
>
> But looking at this more, this is a static mapping between the commands
> and the corresponding struct sizes and you use it in
>
> print_hex_dump_debug("(in): ", DUMP_PREFIX_OFFSET, 16, 2, data,
> sev_cmd_buffer_len(cmd), false);
>
> But then, I don't see what that brings you because you're not dumping
> the actual @data length but the *expected* data length based on the
> command type.
>
> And *that* you can look up in the manual and do not need it in code,
> enlarging the driver unnecessarily.
>
> ...
The debug statement is very helpful during development, it gives me the full
view of what command we send to PSP, data dump of command buffer before and
after the request completion. e.g when dyndbg is enabled the output looks like
this:
[392035.621308] ccp 0000:02:00.2: sev command id 0x4 buffer 0x000080146d232820
[392035.621312] (in): 00000000: 0000 0000 0000 0000 0000 0000
[392035.624725] (out): 00000000: 0e00 0000 0000 0b00 0000 0000
The first debug line prints command ID, second line prints memory dump of the command
structure and third line prints memory dump of command structure after PSP processed
the command.
The caller will use sev_issue_cmd() to issue PSP command. At this time we know
the command id and a opaque pointer (this points to command structure for command id).
Caller does not give us length of the command structure hence we need to derive it
from the command id using sev_cmd_buffer_len(). The command structure length is fixed
for a given command id.
Thanks
Brijesh
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC Part2 PATCH v3 03/26] crypto: ccp: Add Secure Encrypted Virtualization (SEV) device support
2017-09-12 15:32 ` Brijesh Singh
@ 2017-09-12 16:29 ` Borislav Petkov
0 siblings, 0 replies; 22+ messages in thread
From: Borislav Petkov @ 2017-09-12 16:29 UTC (permalink / raw)
To: Brijesh Singh
Cc: linux-kernel, x86, kvm, Thomas Gleixner, Joerg Roedel,
Michael S . Tsirkin, Paolo Bonzini,
\"Radim Krčmář\",
Tom Lendacky, Herbert Xu, David S . Miller, Gary Hook,
linux-crypto
On Tue, Sep 12, 2017 at 10:32:13AM -0500, Brijesh Singh wrote:
> The debug statement is very helpful during development, it gives me the full
> view of what command we send to PSP, data dump of command buffer before and
> after the request completion. e.g when dyndbg is enabled the output looks like
> this:
I'm sure it is all very helpful but you have a bunch of code which is
always built-in and useful only to developers. Which means it could
be behind #ifdef DEBUG at least and disabled on production systems.
You don't have to do it immediately but once the stuff goes up and
everything stabilizes, you could ifdef it out... Something to think
about later, I'd say.
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC Part2 PATCH v3 03/26] crypto: ccp: Add Secure Encrypted Virtualization (SEV) device support
2017-07-24 20:02 ` [RFC Part2 PATCH v3 03/26] crypto: ccp: Add Secure Encrypted Virtualization (SEV) " Brijesh Singh
2017-09-12 14:02 ` Borislav Petkov
@ 2017-09-13 14:17 ` Borislav Petkov
2017-09-13 15:18 ` Brijesh Singh
1 sibling, 1 reply; 22+ messages in thread
From: Borislav Petkov @ 2017-09-13 14:17 UTC (permalink / raw)
To: Brijesh Singh
Cc: linux-kernel, x86, kvm, Thomas Gleixner, Joerg Roedel,
Michael S . Tsirkin, Paolo Bonzini,
\"Radim Krčmář\",
Tom Lendacky, Herbert Xu, David S . Miller, Gary Hook,
linux-crypto
On Mon, Jul 24, 2017 at 03:02:40PM -0500, Brijesh Singh wrote:
> AMDs new Secure Encrypted Virtualization (SEV) feature allows the memory
> contents of a virtual machine to be transparently encrypted with a key
> unique to the guest VM. The programming and management of the encryption
> keys are handled by the AMD Secure Processor (AMD-SP), which exposes the
> commands for these tasks. The complete spec for various commands are
> available at:
> http://support.amd.com/TechDocs/55766_SEV-KM%20API_Specification.pdf
>
> This patch extends AMD-SP driver to provide:
>
> - a in-kernel APIs to communicate with SEV device. The APIs can be used
> by the hypervisor to create encryption context for the SEV guests.
>
> - a userspace IOCTL to manage the platform certificates etc
>
> 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>
...
> +int sev_issue_cmd(int cmd, void *data, int *psp_ret)
> +{
> + struct sev_device *sev = get_sev_master_device();
> + unsigned int phys_lsb, phys_msb;
> + unsigned int reg, ret;
> +
> + if (!sev)
> + return -ENODEV;
> +
> + if (psp_ret)
> + *psp_ret = 0;
So you set psp_ret to 0 here...
> +
> + /* Set the physical address for the PSP */
> + phys_lsb = data ? lower_32_bits(__psp_pa(data)) : 0;
> + phys_msb = data ? upper_32_bits(__psp_pa(data)) : 0;
> +
> + dev_dbg(sev->dev, "sev command id %#x buffer 0x%08x%08x\n",
> + cmd, phys_msb, phys_lsb);
> + print_hex_dump_debug("(in): ", DUMP_PREFIX_OFFSET, 16, 2, data,
> + sev_cmd_buffer_len(cmd), false);
> +
> + /* Only one command at a time... */
> + mutex_lock(&sev_cmd_mutex);
> +
> + iowrite32(phys_lsb, sev->io_regs + PSP_CMDBUFF_ADDR_LO);
> + iowrite32(phys_msb, sev->io_regs + PSP_CMDBUFF_ADDR_HI);
> + wmb();
> +
> + reg = cmd;
> + reg <<= PSP_CMDRESP_CMD_SHIFT;
> + reg |= sev_poll ? 0 : PSP_CMDRESP_IOC;
> + iowrite32(reg, sev->io_regs + PSP_CMDRESP);
> +
> + ret = sev_wait_cmd(sev, ®);
> + if (ret)
> + goto unlock;
... something fails here and you unlock...
> +
> + if (psp_ret)
> + *psp_ret = reg & PSP_CMDRESP_ERR_MASK;
> +
> + if (reg & PSP_CMDRESP_ERR_MASK) {
> + dev_dbg(sev->dev, "sev command %u failed (%#010x)\n",
> + cmd, reg & PSP_CMDRESP_ERR_MASK);
> + ret = -EIO;
> + }
> +
> +unlock:
> + mutex_unlock(&sev_cmd_mutex);
> + print_hex_dump_debug("(out): ", DUMP_PREFIX_OFFSET, 16, 2, data,
> + sev_cmd_buffer_len(cmd), false);
> + return ret;
... and here you return psp_ret == 0 even though something failed.
What I think you should do is not touch @psp_ret when you return before
the SEV command executes and *when* you return, set @psp_ret accordingly
to denote the status of the command execution.
Or if you're touching it before you execute the SEV
command and you return early, it should say something like
PSP_CMDRESP_COMMAND_DIDNT_EXECUTE or so, to tell the caller exactly what
happened.
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC Part2 PATCH v3 03/26] crypto: ccp: Add Secure Encrypted Virtualization (SEV) device support
2017-09-13 14:17 ` Borislav Petkov
@ 2017-09-13 15:18 ` Brijesh Singh
0 siblings, 0 replies; 22+ messages in thread
From: Brijesh Singh @ 2017-09-13 15:18 UTC (permalink / raw)
To: Borislav Petkov
Cc: brijesh.singh, linux-kernel, x86, kvm, Thomas Gleixner,
Joerg Roedel, Michael S . Tsirkin, Paolo Bonzini,
\"Radim Krčmář\",
Tom Lendacky, Herbert Xu, David S . Miller, Gary Hook,
linux-crypto
On 09/13/2017 09:17 AM, Borislav Petkov wrote:
...
>> +
>> +unlock:
>> + mutex_unlock(&sev_cmd_mutex);
>> + print_hex_dump_debug("(out): ", DUMP_PREFIX_OFFSET, 16, 2, data,
>> + sev_cmd_buffer_len(cmd), false);
>> + return ret;
>
> ... and here you return psp_ret == 0 even though something failed.
>
> What I think you should do is not touch @psp_ret when you return before
> the SEV command executes and *when* you return, set @psp_ret accordingly
> to denote the status of the command execution.
>
> Or if you're touching it before you execute the SEV
> command and you return early, it should say something like
> PSP_CMDRESP_COMMAND_DIDNT_EXECUTE or so, to tell the caller exactly what
> happened.
>
Agreed, very good catch thank you. I will fix it.
-Brijesh
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2017-09-13 15:19 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-24 20:02 [RFC Part2 PATCH v3 00/26] x86: Secure Encrypted Virtualization (AMD) Brijesh Singh
2017-07-24 20:02 ` [RFC Part2 PATCH v3 02/26] crypto: ccp: Add Platform Security Processor (PSP) device support Brijesh Singh
2017-07-25 8:29 ` Kamil Konieczny
2017-07-25 15:00 ` Brijesh Singh
2017-09-06 17:00 ` Borislav Petkov
2017-09-06 20:38 ` Brijesh Singh
2017-09-06 20:46 ` Borislav Petkov
2017-09-06 21:26 ` Gary R Hook
2017-09-07 10:34 ` Borislav Petkov
2017-09-07 14:27 ` Borislav Petkov
2017-09-07 22:19 ` Brijesh Singh
2017-09-07 23:15 ` Gary R Hook
2017-09-08 8:22 ` Borislav Petkov
2017-09-08 8:40 ` Borislav Petkov
2017-09-08 13:54 ` Brijesh Singh
2017-09-08 16:06 ` Brijesh Singh
2017-07-24 20:02 ` [RFC Part2 PATCH v3 03/26] crypto: ccp: Add Secure Encrypted Virtualization (SEV) " Brijesh Singh
2017-09-12 14:02 ` Borislav Petkov
2017-09-12 15:32 ` Brijesh Singh
2017-09-12 16:29 ` Borislav Petkov
2017-09-13 14:17 ` Borislav Petkov
2017-09-13 15:18 ` Brijesh Singh
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).