Re: [Part2 PATCH v9 12/38] crypto: ccp: Add Platform Security Processor (PSP) device support

[Philippe Ombredanne]

On Tue, Dec 5, 2017 at 2:04 AM, Brijesh Singh <brijesh.singh@xxxxxxx> wrote:
> The Platform Security Processor (PSP) is part of the AMD Secure
> Processor (AMD-SP) functionality. The PSP is a dedicated processor
> that provides support for key management commands in Secure Encrypted
> Virtualization (SEV) mode, along with software-based Trusted Execution
> Environment (TEE) to enable third-party trusted applications.
>
> Note that the key management functionality provided by the SEV firmware
> can be used outside of the kvm-amd driver hence it doesn't need to
> depend on CONFIG_KVM_AMD.
>
> Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> Cc: "Radim KrÄmÃÅ" <rkrcmar@xxxxxxxxxx>
> Cc: Borislav Petkov <bp@xxxxxxx>
> Cc: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
> Cc: Gary Hook <gary.hook@xxxxxxx>
> Cc: Tom Lendacky <thomas.lendacky@xxxxxxx>
> Cc: linux-crypto@xxxxxxxxxxxxxxx
> Cc: kvm@xxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> Improvements-by: Borislav Petkov <bp@xxxxxxx>
> Signed-off-by: Brijesh Singh <brijesh.singh@xxxxxxx>
> Reviewed-by: Borislav Petkov <bp@xxxxxxx>
> ---
>  drivers/crypto/ccp/Kconfig   |  11 +++++
>  drivers/crypto/ccp/Makefile  |   1 +
>  drivers/crypto/ccp/psp-dev.c | 105 +++++++++++++++++++++++++++++++++++++++++++
>  drivers/crypto/ccp/psp-dev.h |  59 ++++++++++++++++++++++++
>  drivers/crypto/ccp/sp-dev.c  |  26 +++++++++++
>  drivers/crypto/ccp/sp-dev.h  |  24 +++++++++-
>  drivers/crypto/ccp/sp-pci.c  |  52 +++++++++++++++++++++
>  7 files changed, 277 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 9c84f9838931..b9dfae47aefd 100644
> --- a/drivers/crypto/ccp/Kconfig
> +++ b/drivers/crypto/ccp/Kconfig
> @@ -33,3 +33,14 @@ 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 (PSP) device"
> +       default y
> +       depends on CRYPTO_DEV_CCP_DD && X86_64
> +       help
> +        Provide support for the AMD Platform Security Processor (PSP).
> +        The PSP is a dedicated processor that provides support for key
> +        management commands in Secure Encrypted Virtualization (SEV) mode,
> +        along with software-based Trusted Execution Environment (TEE) to
> +        enable third-party trusted applications.
> diff --git a/drivers/crypto/ccp/Makefile b/drivers/crypto/ccp/Makefile
> index c4ce726b931e..51d1c0cf66c7 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-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 000000000000..b5789f878560
> --- /dev/null
> +++ b/drivers/crypto/ccp/psp-dev.c
> @@ -0,0 +1,105 @@
> +/*
> + * AMD Platform Security Processor (PSP) interface
> + *
> + * Copyright (C) 2016-2017 Advanced Micro Devices, Inc.
> + *
> + * Author: Brijesh Singh <brijesh.singh@xxxxxxx>
> + *
> + * 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.
> + */

Dear Brijesh,

Have you considered using the new SPDX license ids instead?

This would come out this way:
> +// SDPX-License-Identifier: GPL-2.0
> +/*
> + * AMD Platform Security Processor (PSP) interface
> + *
> + * Copyright (C) 2016-2017 Advanced Micro Devices, Inc.
> + *
> + * Author: Brijesh Singh <brijesh.singh@xxxxxxx>
> + */

It is much cleaner and simpler, right?

For the C++ comment style and first line placement, please see Thomas
(tlgx) doc patches and Linus posts explaining his rationale of why he
wants it this way.
It would be awesome if this could be applied to all AMD contributions btw!

-- 
Cordially
Philippe Ombredanne