[PATCH 0/4] Add SEV_INIT_EX support

[PATCH 0/4] Add SEV_INIT_EX support

[Peter Gonda]

SEV_INIT requires users to unlock their SPI bus for the PSP's non
volatile (NV) storage. Users may wish to lock their SPI bus for numerous
reasons, to support this the PSP firmware supports SEV_INIT_EX. INIT_EX
allows the firmware to use a region of memory for its NV storage leaving
the kernel responsible for actually storing the data in a persistent
way. This series adds a new module parameter to ccp allowing users to
specify a path to a file for use as the PSP's NV storage. The ccp driver
then reads the file into memory for the PSP to use and is responsible
for writing the file whenever the PSP modifies the memory region.

Signed-off-by: Peter Gonda <pgonda@google.com>
Acked-by: David Rientjes <rientjes@google.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Marc Orr <marcorr@google.com>
Cc: Joerg Roedel <jroedel@suse.de>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: David Rientjes <rientjes@google.com>
Cc: John Allen <john.allen@amd.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Paolo Bonzini <pbonzini@redhat.com> (
Cc: linux-crypto@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

David Rientjes (1):
  crypto: ccp - Add SEV_INIT_EX support

Peter Gonda (3):
  crypto: ccp - Fix SEV_INIT error logging on init
  crypto: ccp - Move SEV_INIT retry for corrupted data
  crypto: ccp - Refactor out sev_fw_alloc()

 drivers/crypto/ccp/sev-dev.c | 235 ++++++++++++++++++++++++++++++-----
 include/linux/psp-sev.h      |  21 ++++
 2 files changed, 222 insertions(+), 34 deletions(-)

-- 
2.33.1.1089.g2158813163f-goog

[PATCH 1/4] crypto: ccp - Fix SEV_INIT error logging on init

[Peter Gonda]

Currently only the firmware error code is printed. This is incomplete
and also incorrect as error cases exists where the firmware is never
called and therefore does not set an error code. This change zeros the
firmware error code in case the call does not get that far and prints
the return code for non firmware errors.

Signed-off-by: Peter Gonda <pgonda@google.com>
Acked-by: David Rientjes <rientjes@google.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Marc Orr <marcorr@google.com>
Cc: Joerg Roedel <jroedel@suse.de>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: David Rientjes <rientjes@google.com>
Cc: John Allen <john.allen@amd.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Paolo Bonzini <pbonzini@redhat.com> (
Cc: linux-crypto@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/crypto/ccp/sev-dev.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index 2ecb0e1f65d8..ec89a82ba267 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -1065,7 +1065,7 @@ void sev_pci_init(void)
 {
 	struct sev_device *sev = psp_master->sev_data;
 	struct page *tmr_page;
-	int error, rc;
+	int error = 0, rc;
 
 	if (!sev)
 		return;
@@ -1104,7 +1104,8 @@ void sev_pci_init(void)
 	}
 
 	if (rc) {
-		dev_err(sev->dev, "SEV: failed to INIT error %#x\n", error);
+		dev_err(sev->dev, "SEV: failed to INIT error %#x, rc %d\n",
+			error, rc);
 		return;
 	}
 
-- 
2.33.1.1089.g2158813163f-goog

Re: [PATCH 1/4] crypto: ccp - Fix SEV_INIT error logging on init

[Tom Lendacky]

On 10/28/21 12:57 PM, Peter Gonda wrote:
> Currently only the firmware error code is printed. This is incomplete
> and also incorrect as error cases exists where the firmware is never
> called and therefore does not set an error code. This change zeros the
> firmware error code in case the call does not get that far and prints
> the return code for non firmware errors.
> 
> Signed-off-by: Peter Gonda <pgonda@google.com>
> Acked-by: David Rientjes <rientjes@google.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Marc Orr <marcorr@google.com>
> Cc: Joerg Roedel <jroedel@suse.de>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: David Rientjes <rientjes@google.com>
> Cc: John Allen <john.allen@amd.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Paolo Bonzini <pbonzini@redhat.com> (
> Cc: linux-crypto@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org

Acked-by: Tom Lendacky <thomas.lendacky@amd.com>

> ---
>   drivers/crypto/ccp/sev-dev.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index 2ecb0e1f65d8..ec89a82ba267 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -1065,7 +1065,7 @@ void sev_pci_init(void)
>   {
>   	struct sev_device *sev = psp_master->sev_data;
>   	struct page *tmr_page;
> -	int error, rc;
> +	int error = 0, rc;
>   
>   	if (!sev)
>   		return;
> @@ -1104,7 +1104,8 @@ void sev_pci_init(void)
>   	}
>   
>   	if (rc) {
> -		dev_err(sev->dev, "SEV: failed to INIT error %#x\n", error);
> +		dev_err(sev->dev, "SEV: failed to INIT error %#x, rc %d\n",
> +			error, rc);
>   		return;
>   	}
>   
> 

Re: [PATCH 1/4] crypto: ccp - Fix SEV_INIT error logging on init

[Marc Orr]

On Thu, Oct 28, 2021 at 10:57 AM Peter Gonda <pgonda@google.com> wrote:
>
> Currently only the firmware error code is printed. This is incomplete
> and also incorrect as error cases exists where the firmware is never
> called and therefore does not set an error code. This change zeros the
> firmware error code in case the call does not get that far and prints
> the return code for non firmware errors.
>
> Signed-off-by: Peter Gonda <pgonda@google.com>
> Acked-by: David Rientjes <rientjes@google.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Marc Orr <marcorr@google.com>
> Cc: Joerg Roedel <jroedel@suse.de>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: David Rientjes <rientjes@google.com>
> Cc: John Allen <john.allen@amd.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Paolo Bonzini <pbonzini@redhat.com> (
> Cc: linux-crypto@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/crypto/ccp/sev-dev.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index 2ecb0e1f65d8..ec89a82ba267 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -1065,7 +1065,7 @@ void sev_pci_init(void)
>  {
>         struct sev_device *sev = psp_master->sev_data;
>         struct page *tmr_page;
> -       int error, rc;
> +       int error = 0, rc;
>
>         if (!sev)
>                 return;
> @@ -1104,7 +1104,8 @@ void sev_pci_init(void)
>         }
>
>         if (rc) {
> -               dev_err(sev->dev, "SEV: failed to INIT error %#x\n", error);
> +               dev_err(sev->dev, "SEV: failed to INIT error %#x, rc %d\n",
> +                       error, rc);
>                 return;
>         }
>
> --
> 2.33.1.1089.g2158813163f-goog
>

Reviewed-by: Marc Orr <marcorr@google.com>

[PATCH 2/4] crypto: ccp - Move SEV_INIT retry for corrupted data

[Peter Gonda]

This change moves the data corrupted retry of SEV_INIT into the
__sev_platform_init_locked() function. This is for upcoming INIT_EX
support as well as helping direct callers of
__sev_platform_init_locked() which currently do not support the
retry.

Signed-off-by: Peter Gonda <pgonda@google.com>
Acked-by: David Rientjes <rientjes@google.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Marc Orr <marcorr@google.com>
Cc: Joerg Roedel <jroedel@suse.de>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: David Rientjes <rientjes@google.com>
Cc: John Allen <john.allen@amd.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Paolo Bonzini <pbonzini@redhat.com> (
Cc: linux-crypto@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/crypto/ccp/sev-dev.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index ec89a82ba267..e4bc833949a0 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -267,6 +267,18 @@ static int __sev_platform_init_locked(int *error)
 	}
 
 	rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
+	if (rc && *error == SEV_RET_SECURE_DATA_INVALID) {
+		/*
+		 * INIT command returned an integrity check failure
+		 * status code, meaning that firmware load and
+		 * validation of SEV related persistent data has
+		 * failed and persistent state has been erased.
+		 * Retrying INIT command here should succeed.
+		 */
+		dev_dbg(sev->dev, "SEV: retrying INIT command");
+		rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
+	}
+
 	if (rc)
 		return rc;
 
@@ -1091,18 +1103,6 @@ void sev_pci_init(void)
 
 	/* Initialize the platform */
 	rc = sev_platform_init(&error);
-	if (rc && (error == SEV_RET_SECURE_DATA_INVALID)) {
-		/*
-		 * INIT command returned an integrity check failure
-		 * status code, meaning that firmware load and
-		 * validation of SEV related persistent data has
-		 * failed and persistent state has been erased.
-		 * Retrying INIT command here should succeed.
-		 */
-		dev_dbg(sev->dev, "SEV: retrying INIT command");
-		rc = sev_platform_init(&error);
-	}
-
 	if (rc) {
 		dev_err(sev->dev, "SEV: failed to INIT error %#x, rc %d\n",
 			error, rc);
-- 
2.33.1.1089.g2158813163f-goog

Re: [PATCH 2/4] crypto: ccp - Move SEV_INIT retry for corrupted data

[Tom Lendacky]

On 10/28/21 12:57 PM, Peter Gonda wrote:
> This change moves the data corrupted retry of SEV_INIT into the
> __sev_platform_init_locked() function. This is for upcoming INIT_EX
> support as well as helping direct callers of
> __sev_platform_init_locked() which currently do not support the
> retry.
> 
> Signed-off-by: Peter Gonda <pgonda@google.com>
> Acked-by: David Rientjes <rientjes@google.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Marc Orr <marcorr@google.com>
> Cc: Joerg Roedel <jroedel@suse.de>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: David Rientjes <rientjes@google.com>
> Cc: John Allen <john.allen@amd.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Paolo Bonzini <pbonzini@redhat.com> (
> Cc: linux-crypto@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org

Acked-by: Tom Lendacky <thomas.lendacky@amd.com>

> ---
>   drivers/crypto/ccp/sev-dev.c | 24 ++++++++++++------------
>   1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index ec89a82ba267..e4bc833949a0 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -267,6 +267,18 @@ static int __sev_platform_init_locked(int *error)
>   	}
>   
>   	rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> +	if (rc && *error == SEV_RET_SECURE_DATA_INVALID) {
> +		/*
> +		 * INIT command returned an integrity check failure
> +		 * status code, meaning that firmware load and
> +		 * validation of SEV related persistent data has
> +		 * failed and persistent state has been erased.
> +		 * Retrying INIT command here should succeed.
> +		 */
> +		dev_dbg(sev->dev, "SEV: retrying INIT command");
> +		rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> +	}
> +
>   	if (rc)
>   		return rc;
>   
> @@ -1091,18 +1103,6 @@ void sev_pci_init(void)
>   
>   	/* Initialize the platform */
>   	rc = sev_platform_init(&error);
> -	if (rc && (error == SEV_RET_SECURE_DATA_INVALID)) {
> -		/*
> -		 * INIT command returned an integrity check failure
> -		 * status code, meaning that firmware load and
> -		 * validation of SEV related persistent data has
> -		 * failed and persistent state has been erased.
> -		 * Retrying INIT command here should succeed.
> -		 */
> -		dev_dbg(sev->dev, "SEV: retrying INIT command");
> -		rc = sev_platform_init(&error);
> -	}
> -
>   	if (rc) {
>   		dev_err(sev->dev, "SEV: failed to INIT error %#x, rc %d\n",
>   			error, rc);
> 

Re: [PATCH 2/4] crypto: ccp - Move SEV_INIT retry for corrupted data

[Marc Orr]

On Thu, Oct 28, 2021 at 10:58 AM Peter Gonda <pgonda@google.com> wrote:
>
> This change moves the data corrupted retry of SEV_INIT into the
> __sev_platform_init_locked() function. This is for upcoming INIT_EX
> support as well as helping direct callers of
> __sev_platform_init_locked() which currently do not support the
> retry.
>
> Signed-off-by: Peter Gonda <pgonda@google.com>
> Acked-by: David Rientjes <rientjes@google.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Marc Orr <marcorr@google.com>
> Cc: Joerg Roedel <jroedel@suse.de>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: David Rientjes <rientjes@google.com>
> Cc: John Allen <john.allen@amd.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Paolo Bonzini <pbonzini@redhat.com> (
> Cc: linux-crypto@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/crypto/ccp/sev-dev.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index ec89a82ba267..e4bc833949a0 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -267,6 +267,18 @@ static int __sev_platform_init_locked(int *error)
>         }
>
>         rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> +       if (rc && *error == SEV_RET_SECURE_DATA_INVALID) {
> +               /*
> +                * INIT command returned an integrity check failure
> +                * status code, meaning that firmware load and
> +                * validation of SEV related persistent data has
> +                * failed and persistent state has been erased.
> +                * Retrying INIT command here should succeed.
> +                */
> +               dev_dbg(sev->dev, "SEV: retrying INIT command");
> +               rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> +       }
> +
>         if (rc)
>                 return rc;
>
> @@ -1091,18 +1103,6 @@ void sev_pci_init(void)
>
>         /* Initialize the platform */
>         rc = sev_platform_init(&error);
> -       if (rc && (error == SEV_RET_SECURE_DATA_INVALID)) {
> -               /*
> -                * INIT command returned an integrity check failure
> -                * status code, meaning that firmware load and
> -                * validation of SEV related persistent data has
> -                * failed and persistent state has been erased.
> -                * Retrying INIT command here should succeed.
> -                */
> -               dev_dbg(sev->dev, "SEV: retrying INIT command");
> -               rc = sev_platform_init(&error);
> -       }
> -
>         if (rc) {
>                 dev_err(sev->dev, "SEV: failed to INIT error %#x, rc %d\n",
>                         error, rc);
> --
> 2.33.1.1089.g2158813163f-goog
>

Reviewed-by: Marc Orr <marcorr@google.com>

[PATCH 3/4] crypto: ccp - Refactor out sev_fw_alloc()

[Peter Gonda]

Creates a helper function sev_fw_alloc() which can be used to allocate
aligned memory regions for use by the PSP firmware. Currently only used
for the SEV-ES TMR region but will be used for the SEV_INIT_EX NV memory
region.

Signed-off-by: Peter Gonda <pgonda@google.com>
Acked-by: David Rientjes <rientjes@google.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Marc Orr <marcorr@google.com>
Cc: Joerg Roedel <jroedel@suse.de>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: David Rientjes <rientjes@google.com>
Cc: John Allen <john.allen@amd.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Paolo Bonzini <pbonzini@redhat.com> (
Cc: linux-crypto@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/crypto/ccp/sev-dev.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index e4bc833949a0..b568ae734857 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -141,6 +141,21 @@ static int sev_cmd_buffer_len(int cmd)
 	return 0;
 }
 
+static void *sev_fw_alloc(unsigned long len)
+{
+	const int order = get_order(len);
+	struct page *page;
+
+	if (order > MAX_ORDER-1)
+		return NULL;
+
+	page = alloc_pages(GFP_KERNEL, order);
+	if (!page)
+		return NULL;
+
+	return page_address(page);
+}
+
 static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
 {
 	struct psp_device *psp = psp_master;
@@ -1076,7 +1091,6 @@ EXPORT_SYMBOL_GPL(sev_issue_cmd_external_user);
 void sev_pci_init(void)
 {
 	struct sev_device *sev = psp_master->sev_data;
-	struct page *tmr_page;
 	int error = 0, rc;
 
 	if (!sev)
@@ -1092,14 +1106,10 @@ void sev_pci_init(void)
 		sev_get_api_version();
 
 	/* Obtain the TMR memory area for SEV-ES use */
-	tmr_page = alloc_pages(GFP_KERNEL, get_order(SEV_ES_TMR_SIZE));
-	if (tmr_page) {
-		sev_es_tmr = page_address(tmr_page);
-	} else {
-		sev_es_tmr = NULL;
+	sev_es_tmr = sev_fw_alloc(SEV_ES_TMR_SIZE);
+	if (!sev_es_tmr)
 		dev_warn(sev->dev,
 			 "SEV: TMR allocation failed, SEV-ES support unavailable\n");
-	}
 
 	/* Initialize the platform */
 	rc = sev_platform_init(&error);
-- 
2.33.1.1089.g2158813163f-goog

Re: [PATCH 3/4] crypto: ccp - Refactor out sev_fw_alloc()

[Tom Lendacky]

On 10/28/21 12:57 PM, Peter Gonda wrote:
> Creates a helper function sev_fw_alloc() which can be used to allocate
> aligned memory regions for use by the PSP firmware. Currently only used
> for the SEV-ES TMR region but will be used for the SEV_INIT_EX NV memory
> region.
> 
> Signed-off-by: Peter Gonda <pgonda@google.com>
> Acked-by: David Rientjes <rientjes@google.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Marc Orr <marcorr@google.com>
> Cc: Joerg Roedel <jroedel@suse.de>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: David Rientjes <rientjes@google.com>
> Cc: John Allen <john.allen@amd.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Paolo Bonzini <pbonzini@redhat.com> (
> Cc: linux-crypto@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
>   drivers/crypto/ccp/sev-dev.c | 24 +++++++++++++++++-------
>   1 file changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index e4bc833949a0..b568ae734857 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -141,6 +141,21 @@ static int sev_cmd_buffer_len(int cmd)
>   	return 0;
>   }
>   
> +static void *sev_fw_alloc(unsigned long len)
> +{
> +	const int order = get_order(len);

This should be an unsigned int to match the function definition, but is 
probably not needed given the comment below.

> +	struct page *page;
> +
> +	if (order > MAX_ORDER-1)
> +		return NULL;

I believe alloc_pages() already does this check (and provides a warning 
unless requested not to), so this check isn't needed.

> +
> +	page = alloc_pages(GFP_KERNEL, order);

Without the above check, you can just replace the 'order' variable with 
'get_order(len)'.

Thanks,
Tom

> +	if (!page)
> +		return NULL;
> +
> +	return page_address(page);
> +}
> +
>   static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
>   {
>   	struct psp_device *psp = psp_master;
> @@ -1076,7 +1091,6 @@ EXPORT_SYMBOL_GPL(sev_issue_cmd_external_user);
>   void sev_pci_init(void)
>   {
>   	struct sev_device *sev = psp_master->sev_data;
> -	struct page *tmr_page;
>   	int error = 0, rc;
>   
>   	if (!sev)
> @@ -1092,14 +1106,10 @@ void sev_pci_init(void)
>   		sev_get_api_version();
>   
>   	/* Obtain the TMR memory area for SEV-ES use */
> -	tmr_page = alloc_pages(GFP_KERNEL, get_order(SEV_ES_TMR_SIZE));
> -	if (tmr_page) {
> -		sev_es_tmr = page_address(tmr_page);
> -	} else {
> -		sev_es_tmr = NULL;
> +	sev_es_tmr = sev_fw_alloc(SEV_ES_TMR_SIZE);
> +	if (!sev_es_tmr)
>   		dev_warn(sev->dev,
>   			 "SEV: TMR allocation failed, SEV-ES support unavailable\n");
> -	}
>   
>   	/* Initialize the platform */
>   	rc = sev_platform_init(&error);
> 

Re: [PATCH 3/4] crypto: ccp - Refactor out sev_fw_alloc()

[Peter Gonda]

On Fri, Oct 29, 2021 at 7:48 AM Tom Lendacky <thomas.lendacky@amd.com> wrote:
>
> On 10/28/21 12:57 PM, Peter Gonda wrote:
> > Creates a helper function sev_fw_alloc() which can be used to allocate
> > aligned memory regions for use by the PSP firmware. Currently only used
> > for the SEV-ES TMR region but will be used for the SEV_INIT_EX NV memory
> > region.
> >
> > Signed-off-by: Peter Gonda <pgonda@google.com>
> > Acked-by: David Rientjes <rientjes@google.com>
> > Cc: Tom Lendacky <thomas.lendacky@amd.com>
> > Cc: Brijesh Singh <brijesh.singh@amd.com>
> > Cc: Marc Orr <marcorr@google.com>
> > Cc: Joerg Roedel <jroedel@suse.de>
> > Cc: Herbert Xu <herbert@gondor.apana.org.au>
> > Cc: David Rientjes <rientjes@google.com>
> > Cc: John Allen <john.allen@amd.com>
> > Cc: "David S. Miller" <davem@davemloft.net>
> > Cc: Paolo Bonzini <pbonzini@redhat.com> (
> > Cc: linux-crypto@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > ---
> >   drivers/crypto/ccp/sev-dev.c | 24 +++++++++++++++++-------
> >   1 file changed, 17 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> > index e4bc833949a0..b568ae734857 100644
> > --- a/drivers/crypto/ccp/sev-dev.c
> > +++ b/drivers/crypto/ccp/sev-dev.c
> > @@ -141,6 +141,21 @@ static int sev_cmd_buffer_len(int cmd)
> >       return 0;
> >   }
> >
> > +static void *sev_fw_alloc(unsigned long len)
> > +{
> > +     const int order = get_order(len);
>
> This should be an unsigned int to match the function definition, but is
> probably not needed given the comment below.
>
> > +     struct page *page;
> > +
> > +     if (order > MAX_ORDER-1)
> > +             return NULL;
>
> I believe alloc_pages() already does this check (and provides a warning
> unless requested not to), so this check isn't needed.

Oh I missed that. Removed.

>
> > +
> > +     page = alloc_pages(GFP_KERNEL, order);
>
> Without the above check, you can just replace the 'order' variable with
> 'get_order(len)'.

Moved the get_order() inline here as suggested.

>
> Thanks,
> Tom
>
> > +     if (!page)
> > +             return NULL;
> > +
> > +     return page_address(page);
> > +}
> > +
> >   static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
> >   {
> >       struct psp_device *psp = psp_master;
> > @@ -1076,7 +1091,6 @@ EXPORT_SYMBOL_GPL(sev_issue_cmd_external_user);
> >   void sev_pci_init(void)
> >   {
> >       struct sev_device *sev = psp_master->sev_data;
> > -     struct page *tmr_page;
> >       int error = 0, rc;
> >
> >       if (!sev)
> > @@ -1092,14 +1106,10 @@ void sev_pci_init(void)
> >               sev_get_api_version();
> >
> >       /* Obtain the TMR memory area for SEV-ES use */
> > -     tmr_page = alloc_pages(GFP_KERNEL, get_order(SEV_ES_TMR_SIZE));
> > -     if (tmr_page) {
> > -             sev_es_tmr = page_address(tmr_page);
> > -     } else {
> > -             sev_es_tmr = NULL;
> > +     sev_es_tmr = sev_fw_alloc(SEV_ES_TMR_SIZE);
> > +     if (!sev_es_tmr)
> >               dev_warn(sev->dev,
> >                        "SEV: TMR allocation failed, SEV-ES support unavailable\n");
> > -     }
> >
> >       /* Initialize the platform */
> >       rc = sev_platform_init(&error);
> >

Re: [PATCH 3/4] crypto: ccp - Refactor out sev_fw_alloc()

[Marc Orr]

On Thu, Oct 28, 2021 at 10:58 AM Peter Gonda <pgonda@google.com> wrote:
>
> Creates a helper function sev_fw_alloc() which can be used to allocate
> aligned memory regions for use by the PSP firmware. Currently only used
> for the SEV-ES TMR region but will be used for the SEV_INIT_EX NV memory
> region.
>
> Signed-off-by: Peter Gonda <pgonda@google.com>
> Acked-by: David Rientjes <rientjes@google.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Marc Orr <marcorr@google.com>
> Cc: Joerg Roedel <jroedel@suse.de>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: David Rientjes <rientjes@google.com>
> Cc: John Allen <john.allen@amd.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Paolo Bonzini <pbonzini@redhat.com> (
> Cc: linux-crypto@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/crypto/ccp/sev-dev.c | 24 +++++++++++++++++-------
>  1 file changed, 17 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index e4bc833949a0..b568ae734857 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -141,6 +141,21 @@ static int sev_cmd_buffer_len(int cmd)
>         return 0;
>  }
>
> +static void *sev_fw_alloc(unsigned long len)
> +{
> +       const int order = get_order(len);
> +       struct page *page;
> +
> +       if (order > MAX_ORDER-1)
> +               return NULL;
> +
> +       page = alloc_pages(GFP_KERNEL, order);
> +       if (!page)
> +               return NULL;
> +
> +       return page_address(page);
> +}
> +
>  static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
>  {
>         struct psp_device *psp = psp_master;
> @@ -1076,7 +1091,6 @@ EXPORT_SYMBOL_GPL(sev_issue_cmd_external_user);
>  void sev_pci_init(void)
>  {
>         struct sev_device *sev = psp_master->sev_data;
> -       struct page *tmr_page;
>         int error = 0, rc;
>
>         if (!sev)
> @@ -1092,14 +1106,10 @@ void sev_pci_init(void)
>                 sev_get_api_version();
>
>         /* Obtain the TMR memory area for SEV-ES use */
> -       tmr_page = alloc_pages(GFP_KERNEL, get_order(SEV_ES_TMR_SIZE));
> -       if (tmr_page) {
> -               sev_es_tmr = page_address(tmr_page);
> -       } else {
> -               sev_es_tmr = NULL;
> +       sev_es_tmr = sev_fw_alloc(SEV_ES_TMR_SIZE);
> +       if (!sev_es_tmr)
>                 dev_warn(sev->dev,
>                          "SEV: TMR allocation failed, SEV-ES support unavailable\n");
> -       }
>
>         /* Initialize the platform */
>         rc = sev_platform_init(&error);
> --
> 2.33.1.1089.g2158813163f-goog
>

Reviewed-by: Marc Orr <marcorr@google.com>

[PATCH 4/4] crypto: ccp - Add SEV_INIT_EX support

[Peter Gonda]

From: David Rientjes <rientjes@google.com>

Add new module parameter to allow users to use SEV_INIT_EX instead of
SEV_INIT. This helps users who lock their SPI bus to use the PSP for SEV
functionality. The 'init_ex_path' parameter defaults to NULL which means
the kernel will use SEV_INIT, if a path is specified SEV_INIT_EX will be
used with the data found at the path. On certain PSP commands this
file is written to as the PSP updates the NV memory region. Depending on
file system initialization this file open may fail during module init
but the CCP driver for SEV already has sufficient retries for platform
initialization. During normal operation of PSP system and SEV commands
if the PSP has not been initialized it is at run time.

Signed-off-by: David Rientjes <rientjes@google.com>
Co-developed-by: Peter Gonda <pgonda@google.com>
Signed-off-by: Peter Gonda <pgonda@google.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Marc Orr <marcorr@google.com>
Cc: Joerg Roedel <jroedel@suse.de>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: David Rientjes <rientjes@google.com>
Cc: John Allen <john.allen@amd.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Paolo Bonzini <pbonzini@redhat.com> (
Cc: linux-crypto@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/crypto/ccp/sev-dev.c | 186 ++++++++++++++++++++++++++++++++---
 include/linux/psp-sev.h      |  21 ++++
 2 files changed, 192 insertions(+), 15 deletions(-)

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index b568ae734857..c8718b4cbc93 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -22,6 +22,7 @@
 #include <linux/firmware.h>
 #include <linux/gfp.h>
 #include <linux/cpufeature.h>
+#include <linux/fs.h>
 
 #include <asm/smp.h>
 
@@ -43,6 +44,10 @@ static int psp_probe_timeout = 5;
 module_param(psp_probe_timeout, int, 0644);
 MODULE_PARM_DESC(psp_probe_timeout, " default timeout value, in seconds, during PSP device probe");
 
+static char *init_ex_path;
+module_param(init_ex_path, charp, 0660);
+MODULE_PARM_DESC(init_ex_path, " Path for INIT_EX data; if set try INIT_EX");
+
 MODULE_FIRMWARE("amd/amd_sev_fam17h_model0xh.sbin"); /* 1st gen EPYC */
 MODULE_FIRMWARE("amd/amd_sev_fam17h_model3xh.sbin"); /* 2nd gen EPYC */
 MODULE_FIRMWARE("amd/amd_sev_fam19h_model0xh.sbin"); /* 3rd gen EPYC */
@@ -58,6 +63,14 @@ static int psp_timeout;
 #define SEV_ES_TMR_SIZE		(1024 * 1024)
 static void *sev_es_tmr;
 
+/* INIT_EX NV Storage:
+ *   The NV Storage is a 32Kb area and must be 4Kb page aligned.  Use the page
+ *   allocator to allocate the memory, which will return aligned memory for the
+ *   specified allocation order.
+ */
+#define NV_LENGTH (32 << 10)
+static void *sev_init_ex_nv_address;
+
 static inline bool sev_version_greater_or_equal(u8 maj, u8 min)
 {
 	struct sev_device *sev = psp_master->sev_data;
@@ -135,6 +148,7 @@ static int sev_cmd_buffer_len(int cmd)
 	case SEV_CMD_GET_ID:			return sizeof(struct sev_data_get_id);
 	case SEV_CMD_ATTESTATION_REPORT:	return sizeof(struct sev_data_attestation_report);
 	case SEV_CMD_SEND_CANCEL:			return sizeof(struct sev_data_send_cancel);
+	case SEV_CMD_INIT_EX:                   return sizeof(struct sev_data_init_ex);
 	default:				return 0;
 	}
 
@@ -156,6 +170,89 @@ static void *sev_fw_alloc(unsigned long len)
 	return page_address(page);
 }
 
+static int sev_read_nv_memory(void)
+{
+	struct file *fp;
+	ssize_t nread;
+
+	if (!sev_init_ex_nv_address)
+		return -EOPNOTSUPP;
+
+	fp = filp_open(init_ex_path, O_RDONLY, 0);
+	if (IS_ERR(fp)) {
+		dev_err(psp_master->dev, "sev could not open file for read\n");
+		return PTR_ERR(fp);
+	}
+
+	nread = kernel_read(fp, sev_init_ex_nv_address, NV_LENGTH, 0);
+	dev_dbg(psp_master->dev, "sev NV read %d bytes\n", nread);
+	filp_close(fp, NULL);
+	return 0;
+}
+
+static int sev_write_nv_memory(void)
+{
+	struct sev_device *sev = psp_master->sev_data;
+	struct file *fp;
+	loff_t offset = 0;
+	int ret;
+
+	if (!sev_init_ex_nv_address)
+		return -EOPNOTSUPP;
+
+	fp = filp_open(init_ex_path, O_CREAT | O_WRONLY, 0600);
+	if (IS_ERR(fp)) {
+		dev_err(sev->dev, "sev NV data could not be created\n");
+		return PTR_ERR(fp);
+	}
+	ret = kernel_write(fp, sev_init_ex_nv_address, NV_LENGTH, &offset);
+	vfs_fsync(fp, 0);
+	filp_close(fp, NULL);
+
+	if (ret != NV_LENGTH) {
+		dev_err(sev->dev,
+			"failed to write %d bytes to non volatile memory area, ret=%lu\n",
+			NV_LENGTH, ret);
+		if (ret >= 0)
+			return -EIO;
+		return ret;
+	}
+
+	dev_dbg(sev->dev, "wrote to non volatile memory area\n");
+	return 0;
+}
+
+static void sev_write_nv_memory_if_required(int cmd_id)
+{
+	struct sev_device *sev = psp_master->sev_data;
+	int ret;
+
+	if (!sev_init_ex_nv_address)
+		return;
+
+	/*
+	 * Only a few platform commands modify the SPI/NV area,
+	 * but none of the non-platform commands do. Only INIT,
+	 * PLATFORM_RESET, PEK_GEN, PEK_CERT_IMPORT, and
+	 * PDH_GEN do.
+	 */
+	switch (cmd_id) {
+	case SEV_CMD_FACTORY_RESET:
+	case SEV_CMD_INIT_EX:
+	case SEV_CMD_PDH_GEN:
+	case SEV_CMD_PEK_CERT_IMPORT:
+	case SEV_CMD_PEK_GEN:
+	case SEV_CMD_SHUTDOWN:
+		break;
+	default:
+		return;
+	};
+
+	ret = sev_write_nv_memory();
+	if (ret)
+		dev_err(sev->dev, "sev NV write failed %d\n", ret);
+}
+
 static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
 {
 	struct psp_device *psp = psp_master;
@@ -225,6 +322,8 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
 		dev_dbg(sev->dev, "sev command %#x failed (%#010x)\n",
 			cmd, reg & PSP_CMDRESP_ERR_MASK);
 		ret = -EIO;
+	} else {
+		sev_write_nv_memory_if_required(cmd);
 	}
 
 	print_hex_dump_debug("(out): ", DUMP_PREFIX_OFFSET, 16, 2, data,
@@ -251,22 +350,42 @@ static int sev_do_cmd(int cmd, void *data, int *psp_ret)
 	return rc;
 }
 
-static int __sev_platform_init_locked(int *error)
+static int __sev_init_locked(int *error)
 {
-	struct psp_device *psp = psp_master;
 	struct sev_data_init data;
-	struct sev_device *sev;
-	int rc = 0;
 
-	if (!psp || !psp->sev_data)
-		return -ENODEV;
+	memset(&data, 0, sizeof(data));
+	if (sev_es_tmr) {
+		u64 tmr_pa;
 
-	sev = psp->sev_data;
+		/*
+		 * Do not include the encryption mask on the physical
+		 * address of the TMR (firmware should clear it anyway).
+		 */
+		tmr_pa = __pa(sev_es_tmr);
 
-	if (sev->state == SEV_STATE_INIT)
-		return 0;
+		data.flags |= SEV_INIT_FLAGS_SEV_ES;
+		data.tmr_address = tmr_pa;
+		data.tmr_len = SEV_ES_TMR_SIZE;
+	}
+
+	return __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
+}
+
+static int __sev_init_ex_locked(int *error)
+{
+	struct sev_data_init_ex data;
+	int ret;
 
 	memset(&data, 0, sizeof(data));
+	data.length = sizeof(data);
+	data.nv_address = __psp_pa(sev_init_ex_nv_address);
+	data.nv_len = NV_LENGTH;
+
+	ret = sev_read_nv_memory();
+	if (ret)
+		return ret;
+
 	if (sev_es_tmr) {
 		u64 tmr_pa;
 
@@ -276,12 +395,30 @@ static int __sev_platform_init_locked(int *error)
 		 */
 		tmr_pa = __pa(sev_es_tmr);
 
-		data.flags |= SEV_INIT_FLAGS_SEV_ES;
 		data.tmr_address = tmr_pa;
 		data.tmr_len = SEV_ES_TMR_SIZE;
 	}
+	return __sev_do_cmd_locked(SEV_CMD_INIT_EX, &data, error);
+}
+
+static int __sev_platform_init_locked(int *error)
+{
+	struct psp_device *psp = psp_master;
+	struct sev_device *sev;
+	int rc;
+	int (*init_function)(int *error) = sev_init_ex_nv_address ?
+			__sev_init_ex_locked :
+			__sev_init_locked;
+
+	if (!psp || !psp->sev_data)
+		return -ENODEV;
+
+	sev = psp->sev_data;
 
-	rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
+	if (sev->state == SEV_STATE_INIT)
+		return 0;
+
+	rc = init_function(error);
 	if (rc && *error == SEV_RET_SECURE_DATA_INVALID) {
 		/*
 		 * INIT command returned an integrity check failure
@@ -290,8 +427,8 @@ static int __sev_platform_init_locked(int *error)
 		 * failed and persistent state has been erased.
 		 * Retrying INIT command here should succeed.
 		 */
-		dev_dbg(sev->dev, "SEV: retrying INIT command");
-		rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
+		dev_err(sev->dev, "SEV: retrying INIT command");
+		rc = init_function(error);
 	}
 
 	if (rc)
@@ -307,7 +444,7 @@ static int __sev_platform_init_locked(int *error)
 
 	dev_dbg(sev->dev, "SEV firmware initialized\n");
 
-	return rc;
+	return 0;
 }
 
 int sev_platform_init(int *error)
@@ -987,7 +1124,7 @@ static int sev_misc_init(struct sev_device *sev)
 
 	init_waitqueue_head(&sev->int_queue);
 	sev->misc = misc_dev;
-	dev_dbg(dev, "registered SEV device\n");
+	dev_err(dev, "registered SEV device\n");
 
 	return 0;
 }
@@ -1061,6 +1198,12 @@ static void sev_firmware_shutdown(struct sev_device *sev)
 			   get_order(SEV_ES_TMR_SIZE));
 		sev_es_tmr = NULL;
 	}
+
+	if (sev_init_ex_nv_address) {
+		free_pages((unsigned long)sev_init_ex_nv_address,
+			   get_order(NV_LENGTH));
+		sev_init_ex_nv_address = NULL;
+	}
 }
 
 void sev_dev_destroy(struct psp_device *psp)
@@ -1105,6 +1248,19 @@ void sev_pci_init(void)
 	    sev_update_firmware(sev->dev) == 0)
 		sev_get_api_version();
 
+	/* If an init_ex_path is provided rely on INIT_EX for PSP initialization
+	 * instead of INIT.
+	 */
+	if (init_ex_path) {
+		sev_init_ex_nv_address = sev_fw_alloc(NV_LENGTH);
+		if (!sev_init_ex_nv_address) {
+			dev_warn(
+				sev->dev,
+				"SEV: INIT_EX NV storage allocation failed, INIT-EX support unavailable\n");
+			goto err;
+		}
+	}
+
 	/* Obtain the TMR memory area for SEV-ES use */
 	sev_es_tmr = sev_fw_alloc(SEV_ES_TMR_SIZE);
 	if (!sev_es_tmr)
diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
index d48a7192e881..1595088c428b 100644
--- a/include/linux/psp-sev.h
+++ b/include/linux/psp-sev.h
@@ -52,6 +52,7 @@ enum sev_cmd {
 	SEV_CMD_DF_FLUSH		= 0x00A,
 	SEV_CMD_DOWNLOAD_FIRMWARE	= 0x00B,
 	SEV_CMD_GET_ID			= 0x00C,
+	SEV_CMD_INIT_EX                 = 0x00D,
 
 	/* Guest commands */
 	SEV_CMD_DECOMMISSION		= 0x020,
@@ -102,6 +103,26 @@ struct sev_data_init {
 	u32 tmr_len;			/* In */
 } __packed;
 
+/**
+ * struct sev_data_init_ex - INIT_EX command parameters
+ *
+ * @length: len of the command buffer read by the PSP
+ * @flags: processing flags
+ * @tmr_address: system physical address used for SEV-ES
+ * @tmr_len: len of tmr_address
+ * @nv_address: system physical address used for PSP NV storage
+ * @nv_len: len of nv_address
+ */
+struct sev_data_init_ex {
+	u32 length;                     /* In */
+	u32 flags;                      /* In */
+	u64 tmr_address;                /* In */
+	u32 tmr_len;                    /* In */
+	u32 reserved;                   /* In */
+	u64 nv_address;                 /* In/Out */
+	u32 nv_len;                     /* In */
+} __packed;
+
 #define SEV_INIT_FLAGS_SEV_ES	0x01
 
 /**
-- 
2.33.1.1089.g2158813163f-goog

Re: [PATCH 4/4] crypto: ccp - Add SEV_INIT_EX support

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2270 bytes --]

Hi Peter,

I love your patch! Perhaps something to improve:

[auto build test WARNING on herbert-crypto-2.6/master]
[also build test WARNING on linus/master v5.15-rc7]
[cannot apply to herbert-cryptodev-2.6/master next-20211028]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Peter-Gonda/Add-SEV_INIT_EX-support/20211029-020022
base:   https://git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6.git master
config: x86_64-rhel-8.3-kselftests (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.4-dirty
        # https://github.com/0day-ci/linux/commit/aa961b54617e0edd40b4df9d9d159014cabb7953
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Peter-Gonda/Add-SEV_INIT_EX-support/20211029-020022
        git checkout aa961b54617e0edd40b4df9d9d159014cabb7953
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/crypto/ccp/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)
>> drivers/crypto/ccp/sev-dev.c:187:68: sparse: sparse: Using plain integer as NULL pointer

vim +187 drivers/crypto/ccp/sev-dev.c

   172	
   173	static int sev_read_nv_memory(void)
   174	{
   175		struct file *fp;
   176		ssize_t nread;
   177	
   178		if (!sev_init_ex_nv_address)
   179			return -EOPNOTSUPP;
   180	
   181		fp = filp_open(init_ex_path, O_RDONLY, 0);
   182		if (IS_ERR(fp)) {
   183			dev_err(psp_master->dev, "sev could not open file for read\n");
   184			return PTR_ERR(fp);
   185		}
   186	
 > 187		nread = kernel_read(fp, sev_init_ex_nv_address, NV_LENGTH, 0);
   188		dev_dbg(psp_master->dev, "sev NV read %d bytes\n", nread);
   189		filp_close(fp, NULL);
   190		return 0;
   191	}
   192	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 42204 bytes --]

Re: [PATCH 4/4] crypto: ccp - Add SEV_INIT_EX support

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2331 bytes --]

Hi Peter,

I love your patch! Perhaps something to improve:

[auto build test WARNING on herbert-crypto-2.6/master]
[also build test WARNING on linus/master v5.15-rc7]
[cannot apply to herbert-cryptodev-2.6/master next-20211028]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Peter-Gonda/Add-SEV_INIT_EX-support/20211029-020022
base:   https://git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6.git master
config: x86_64-rhel-8.3-kselftests (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.4-dirty
        # https://github.com/0day-ci/linux/commit/aa961b54617e0edd40b4df9d9d159014cabb7953
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Peter-Gonda/Add-SEV_INIT_EX-support/20211029-020022
        git checkout aa961b54617e0edd40b4df9d9d159014cabb7953
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/crypto/ccp/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)
>> drivers/crypto/ccp/sev-dev.c:187:68: sparse: sparse: Using plain integer as NULL pointer

vim +187 drivers/crypto/ccp/sev-dev.c

   172	
   173	static int sev_read_nv_memory(void)
   174	{
   175		struct file *fp;
   176		ssize_t nread;
   177	
   178		if (!sev_init_ex_nv_address)
   179			return -EOPNOTSUPP;
   180	
   181		fp = filp_open(init_ex_path, O_RDONLY, 0);
   182		if (IS_ERR(fp)) {
   183			dev_err(psp_master->dev, "sev could not open file for read\n");
   184			return PTR_ERR(fp);
   185		}
   186	
 > 187		nread = kernel_read(fp, sev_init_ex_nv_address, NV_LENGTH, 0);
   188		dev_dbg(psp_master->dev, "sev NV read %d bytes\n", nread);
   189		filp_close(fp, NULL);
   190		return 0;
   191	}
   192	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 42204 bytes --]

Re: [PATCH 4/4] crypto: ccp - Add SEV_INIT_EX support

[Tom Lendacky]

On 10/28/21 12:57 PM, Peter Gonda wrote:
> From: David Rientjes <rientjes@google.com>
> 
> Add new module parameter to allow users to use SEV_INIT_EX instead of
> SEV_INIT. This helps users who lock their SPI bus to use the PSP for SEV
> functionality. The 'init_ex_path' parameter defaults to NULL which means
> the kernel will use SEV_INIT, if a path is specified SEV_INIT_EX will be
> used with the data found at the path. On certain PSP commands this
> file is written to as the PSP updates the NV memory region. Depending on
> file system initialization this file open may fail during module init
> but the CCP driver for SEV already has sufficient retries for platform
> initialization. During normal operation of PSP system and SEV commands
> if the PSP has not been initialized it is at run time.

IIUC, it looks as though the file has to exist before the very first use, 
otherwise the initialization will fail. Did you consider checking for the 
presence of the file first and, if not there, just using a memory area of 
all f's (as documented in the SEV API)? Then on successful INIT, the 
memory would be written and the file created.

Either way, probably worth adding to the commit message. And if you stay 
with having to pre-allocate the file, it's worth adding to the SEV 
documentation what is required to be done to initialize the file.

Although, INIT_EX is probably worth adding to the SEV documentation in 
general.

> 
> Signed-off-by: David Rientjes <rientjes@google.com>
> Co-developed-by: Peter Gonda <pgonda@google.com>
> Signed-off-by: Peter Gonda <pgonda@google.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Marc Orr <marcorr@google.com>
> Cc: Joerg Roedel <jroedel@suse.de>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: David Rientjes <rientjes@google.com>
> Cc: John Allen <john.allen@amd.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Paolo Bonzini <pbonzini@redhat.com> (
> Cc: linux-crypto@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
>   drivers/crypto/ccp/sev-dev.c | 186 ++++++++++++++++++++++++++++++++---
>   include/linux/psp-sev.h      |  21 ++++
>   2 files changed, 192 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index b568ae734857..c8718b4cbc93 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -22,6 +22,7 @@
>   #include <linux/firmware.h>
>   #include <linux/gfp.h>
>   #include <linux/cpufeature.h>
> +#include <linux/fs.h>
>   
>   #include <asm/smp.h>
>   
> @@ -43,6 +44,10 @@ static int psp_probe_timeout = 5;
>   module_param(psp_probe_timeout, int, 0644);
>   MODULE_PARM_DESC(psp_probe_timeout, " default timeout value, in seconds, during PSP device probe");
>   
> +static char *init_ex_path;
> +module_param(init_ex_path, charp, 0660);
> +MODULE_PARM_DESC(init_ex_path, " Path for INIT_EX data; if set try INIT_EX");
> +
>   MODULE_FIRMWARE("amd/amd_sev_fam17h_model0xh.sbin"); /* 1st gen EPYC */
>   MODULE_FIRMWARE("amd/amd_sev_fam17h_model3xh.sbin"); /* 2nd gen EPYC */
>   MODULE_FIRMWARE("amd/amd_sev_fam19h_model0xh.sbin"); /* 3rd gen EPYC */
> @@ -58,6 +63,14 @@ static int psp_timeout;
>   #define SEV_ES_TMR_SIZE		(1024 * 1024)
>   static void *sev_es_tmr;
>   
> +/* INIT_EX NV Storage:
> + *   The NV Storage is a 32Kb area and must be 4Kb page aligned.  Use the page
> + *   allocator to allocate the memory, which will return aligned memory for the
> + *   specified allocation order.
> + */
> +#define NV_LENGTH (32 << 10)

Just me, but I think '32 * 1024' would be a bit clearer.

> +static void *sev_init_ex_nv_address;
> +
>   static inline bool sev_version_greater_or_equal(u8 maj, u8 min)
>   {
>   	struct sev_device *sev = psp_master->sev_data;
> @@ -135,6 +148,7 @@ static int sev_cmd_buffer_len(int cmd)
>   	case SEV_CMD_GET_ID:			return sizeof(struct sev_data_get_id);
>   	case SEV_CMD_ATTESTATION_REPORT:	return sizeof(struct sev_data_attestation_report);
>   	case SEV_CMD_SEND_CANCEL:			return sizeof(struct sev_data_send_cancel);
> +	case SEV_CMD_INIT_EX:                   return sizeof(struct sev_data_init_ex);

Maybe move this to just under the SEV_CMD_INIT: case statement?

>   	default:				return 0;
>   	}
>   
> @@ -156,6 +170,89 @@ static void *sev_fw_alloc(unsigned long len)
>   	return page_address(page);
>   }
>   
> +static int sev_read_nv_memory(void)
> +{
> +	struct file *fp;
> +	ssize_t nread;
> +
> +	if (!sev_init_ex_nv_address)
> +		return -EOPNOTSUPP;
> +
> +	fp = filp_open(init_ex_path, O_RDONLY, 0);
> +	if (IS_ERR(fp)) {
> +		dev_err(psp_master->dev, "sev could not open file for read\n");
> +		return PTR_ERR(fp);
> +	}
> +
> +	nread = kernel_read(fp, sev_init_ex_nv_address, NV_LENGTH, 0);
> +	dev_dbg(psp_master->dev, "sev NV read %d bytes\n", nread);
> +	filp_close(fp, NULL);

Add a blank line here.

> +	return 0;
> +}
> +
> +static int sev_write_nv_memory(void)
> +{
> +	struct sev_device *sev = psp_master->sev_data;
> +	struct file *fp;
> +	loff_t offset = 0;
> +	int ret;
> +
> +	if (!sev_init_ex_nv_address)
> +		return -EOPNOTSUPP;
> +
> +	fp = filp_open(init_ex_path, O_CREAT | O_WRONLY, 0600);
> +	if (IS_ERR(fp)) {
> +		dev_err(sev->dev, "sev NV data could not be created\n");
> +		return PTR_ERR(fp);
> +	}

Add a blank line here.

> +	ret = kernel_write(fp, sev_init_ex_nv_address, NV_LENGTH, &offset);
> +	vfs_fsync(fp, 0);
> +	filp_close(fp, NULL);
> +
> +	if (ret != NV_LENGTH) {
> +		dev_err(sev->dev,
> +			"failed to write %d bytes to non volatile memory area, ret=%lu\n",
> +			NV_LENGTH, ret);
> +		if (ret >= 0)
> +			return -EIO;
> +		return ret;
> +	}
> +
> +	dev_dbg(sev->dev, "wrote to non volatile memory area\n");

Add a blank line here.

> +	return 0;
> +}
> +
> +static void sev_write_nv_memory_if_required(int cmd_id)
> +{
> +	struct sev_device *sev = psp_master->sev_data;
> +	int ret;
> +
> +	if (!sev_init_ex_nv_address)
> +		return;
> +
> +	/*
> +	 * Only a few platform commands modify the SPI/NV area,
> +	 * but none of the non-platform commands do. Only INIT,

maybe say INIT(_EX)?

> +	 * PLATFORM_RESET, PEK_GEN, PEK_CERT_IMPORT, and
> +	 * PDH_GEN do.

Does SHUTDOWN modify the SPI/NV area? Otherwise a separate comment about 
why it is included below.

> +	 */
> +	switch (cmd_id) {
> +	case SEV_CMD_FACTORY_RESET:
> +	case SEV_CMD_INIT_EX:
> +	case SEV_CMD_PDH_GEN:
> +	case SEV_CMD_PEK_CERT_IMPORT:
> +	case SEV_CMD_PEK_GEN:
> +	case SEV_CMD_SHUTDOWN:
> +		break;
> +	default:
> +		return;
> +	};
> +
> +	ret = sev_write_nv_memory();
> +	if (ret)
> +		dev_err(sev->dev, "sev NV write failed %d\n", ret);

You already have error messages in the sev_write_nv_memory() function, 
this one probably isn't needed.

> +}
> +
>   static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
>   {
>   	struct psp_device *psp = psp_master;
> @@ -225,6 +322,8 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
>   		dev_dbg(sev->dev, "sev command %#x failed (%#010x)\n",
>   			cmd, reg & PSP_CMDRESP_ERR_MASK);
>   		ret = -EIO;
> +	} else {
> +		sev_write_nv_memory_if_required(cmd);
>   	}
>   
>   	print_hex_dump_debug("(out): ", DUMP_PREFIX_OFFSET, 16, 2, data,
> @@ -251,22 +350,42 @@ static int sev_do_cmd(int cmd, void *data, int *psp_ret)
>   	return rc;
>   }
>   
> -static int __sev_platform_init_locked(int *error)
> +static int __sev_init_locked(int *error)
>   {
> -	struct psp_device *psp = psp_master;
>   	struct sev_data_init data;
> -	struct sev_device *sev;
> -	int rc = 0;
>   
> -	if (!psp || !psp->sev_data)
> -		return -ENODEV;
> +	memset(&data, 0, sizeof(data));
> +	if (sev_es_tmr) {
> +		u64 tmr_pa;
>   
> -	sev = psp->sev_data;
> +		/*
> +		 * Do not include the encryption mask on the physical
> +		 * address of the TMR (firmware should clear it anyway).
> +		 */
> +		tmr_pa = __pa(sev_es_tmr);
>   
> -	if (sev->state == SEV_STATE_INIT)
> -		return 0;
> +		data.flags |= SEV_INIT_FLAGS_SEV_ES;
> +		data.tmr_address = tmr_pa;
> +		data.tmr_len = SEV_ES_TMR_SIZE;
> +	}
> +
> +	return __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> +}
> +
> +static int __sev_init_ex_locked(int *error)
> +{
> +	struct sev_data_init_ex data;
> +	int ret;
>   
>   	memset(&data, 0, sizeof(data));
> +	data.length = sizeof(data);
> +	data.nv_address = __psp_pa(sev_init_ex_nv_address);
> +	data.nv_len = NV_LENGTH;
> +
> +	ret = sev_read_nv_memory();
> +	if (ret)
> +		return ret;
> +
>   	if (sev_es_tmr) {
>   		u64 tmr_pa;
>   
> @@ -276,12 +395,30 @@ static int __sev_platform_init_locked(int *error)
>   		 */
>   		tmr_pa = __pa(sev_es_tmr);
>   
> -		data.flags |= SEV_INIT_FLAGS_SEV_ES;

Inadvertant deletion?

>   		data.tmr_address = tmr_pa;
>   		data.tmr_len = SEV_ES_TMR_SIZE;
>   	}

Add a blank line here.

> +	return __sev_do_cmd_locked(SEV_CMD_INIT_EX, &data, error);
> +}
> +
> +static int __sev_platform_init_locked(int *error)
> +{
> +	struct psp_device *psp = psp_master;
> +	struct sev_device *sev;
> +	int rc;
> +	int (*init_function)(int *error) = sev_init_ex_nv_address ?
> +			__sev_init_ex_locked :
> +			__sev_init_locked;

This seems a bit much in the declaration. How about moving the assignment 
down to just before the first call?

> +
> +	if (!psp || !psp->sev_data)
> +		return -ENODEV;
> +
> +	sev = psp->sev_data;
>   
> -	rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> +	if (sev->state == SEV_STATE_INIT)
> +		return 0;
> +
> +	rc = init_function(error);
>   	if (rc && *error == SEV_RET_SECURE_DATA_INVALID) {
>   		/*
>   		 * INIT command returned an integrity check failure
> @@ -290,8 +427,8 @@ static int __sev_platform_init_locked(int *error)
>   		 * failed and persistent state has been erased.
>   		 * Retrying INIT command here should succeed.
>   		 */
> -		dev_dbg(sev->dev, "SEV: retrying INIT command");
> -		rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> +		dev_err(sev->dev, "SEV: retrying INIT command");

Maybe dev_notice() instead of dev_err()?

> +		rc = init_function(error);
>   	}
>   
>   	if (rc)
> @@ -307,7 +444,7 @@ static int __sev_platform_init_locked(int *error)
>   
>   	dev_dbg(sev->dev, "SEV firmware initialized\n");
>   
> -	return rc;
> +	return 0;
>   }
>   
>   int sev_platform_init(int *error)
> @@ -987,7 +1124,7 @@ static int sev_misc_init(struct sev_device *sev)
>   
>   	init_waitqueue_head(&sev->int_queue);
>   	sev->misc = misc_dev;
> -	dev_dbg(dev, "registered SEV device\n");
> +	dev_err(dev, "registered SEV device\n");

Not sure this is a necessary change... but, if you don't want this as a 
dev_dbg() then it should be a dev_info(), because it is not an error.

>   
>   	return 0;
>   }
> @@ -1061,6 +1198,12 @@ static void sev_firmware_shutdown(struct sev_device *sev)
>   			   get_order(SEV_ES_TMR_SIZE));
>   		sev_es_tmr = NULL;
>   	}
> +
> +	if (sev_init_ex_nv_address) {
> +		free_pages((unsigned long)sev_init_ex_nv_address,
> +			   get_order(NV_LENGTH));
> +		sev_init_ex_nv_address = NULL;
> +	}
>   }
>   
>   void sev_dev_destroy(struct psp_device *psp)
> @@ -1105,6 +1248,19 @@ void sev_pci_init(void)
>   	    sev_update_firmware(sev->dev) == 0)
>   		sev_get_api_version();
>   
> +	/* If an init_ex_path is provided rely on INIT_EX for PSP initialization
> +	 * instead of INIT.
> +	 */
> +	if (init_ex_path) {
> +		sev_init_ex_nv_address = sev_fw_alloc(NV_LENGTH);
> +		if (!sev_init_ex_nv_address) {
> +			dev_warn(

Shouldn't this be a dev_err(), since you are erroring out?

> +				sev->dev,

Move this up to the previous line, i.e.: dev_err(sev->dev,

> +				"SEV: INIT_EX NV storage allocation failed, INIT-EX support unavailable\n");

Since you're erroring out, probably enough to just have the first part of 
that message. But if not:

s/INIT-EX/INIT_EX/

Thanks,
Tom

> +			goto err;
> +		}
> +	}
> +
>   	/* Obtain the TMR memory area for SEV-ES use */
>   	sev_es_tmr = sev_fw_alloc(SEV_ES_TMR_SIZE);
>   	if (!sev_es_tmr)
> diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
> index d48a7192e881..1595088c428b 100644
> --- a/include/linux/psp-sev.h
> +++ b/include/linux/psp-sev.h
> @@ -52,6 +52,7 @@ enum sev_cmd {
>   	SEV_CMD_DF_FLUSH		= 0x00A,
>   	SEV_CMD_DOWNLOAD_FIRMWARE	= 0x00B,
>   	SEV_CMD_GET_ID			= 0x00C,
> +	SEV_CMD_INIT_EX                 = 0x00D,
>   
>   	/* Guest commands */
>   	SEV_CMD_DECOMMISSION		= 0x020,
> @@ -102,6 +103,26 @@ struct sev_data_init {
>   	u32 tmr_len;			/* In */
>   } __packed;
>   
> +/**
> + * struct sev_data_init_ex - INIT_EX command parameters
> + *
> + * @length: len of the command buffer read by the PSP
> + * @flags: processing flags
> + * @tmr_address: system physical address used for SEV-ES
> + * @tmr_len: len of tmr_address
> + * @nv_address: system physical address used for PSP NV storage
> + * @nv_len: len of nv_address
> + */
> +struct sev_data_init_ex {
> +	u32 length;                     /* In */
> +	u32 flags;                      /* In */
> +	u64 tmr_address;                /* In */
> +	u32 tmr_len;                    /* In */
> +	u32 reserved;                   /* In */
> +	u64 nv_address;                 /* In/Out */
> +	u32 nv_len;                     /* In */
> +} __packed;
> +
>   #define SEV_INIT_FLAGS_SEV_ES	0x01
>   
>   /**
> 

Re: [PATCH 4/4] crypto: ccp - Add SEV_INIT_EX support

[Peter Gonda]

On Fri, Oct 29, 2021 at 8:45 AM Tom Lendacky <thomas.lendacky@amd.com> wrote:
>
> On 10/28/21 12:57 PM, Peter Gonda wrote:
> > From: David Rientjes <rientjes@google.com>
> >
> > Add new module parameter to allow users to use SEV_INIT_EX instead of
> > SEV_INIT. This helps users who lock their SPI bus to use the PSP for SEV
> > functionality. The 'init_ex_path' parameter defaults to NULL which means
> > the kernel will use SEV_INIT, if a path is specified SEV_INIT_EX will be
> > used with the data found at the path. On certain PSP commands this
> > file is written to as the PSP updates the NV memory region. Depending on
> > file system initialization this file open may fail during module init
> > but the CCP driver for SEV already has sufficient retries for platform
> > initialization. During normal operation of PSP system and SEV commands
> > if the PSP has not been initialized it is at run time.
>
> IIUC, it looks as though the file has to exist before the very first use,
> otherwise the initialization will fail. Did you consider checking for the
> presence of the file first and, if not there, just using a memory area of
> all f's (as documented in the SEV API)? Then on successful INIT, the
> memory would be written and the file created.

Thats a great idea. I'll add that functionality.

>
> Either way, probably worth adding to the commit message. And if you stay
> with having to pre-allocate the file, it's worth adding to the SEV
> documentation what is required to be done to initialize the file.
>
> Although, INIT_EX is probably worth adding to the SEV documentation in
> general.

Is this (https://www.kernel.org/doc/Documentation/virt/kvm/amd-memory-encryption.rst)
the documentation you were referring too? If so I can add another
patch before this one to document INIT, then add the INIT_EX
documentation here. Or were you thinking something else?

>
> >
> > Signed-off-by: David Rientjes <rientjes@google.com>
> > Co-developed-by: Peter Gonda <pgonda@google.com>
> > Signed-off-by: Peter Gonda <pgonda@google.com>
> > Cc: Tom Lendacky <thomas.lendacky@amd.com>
> > Cc: Brijesh Singh <brijesh.singh@amd.com>
> > Cc: Marc Orr <marcorr@google.com>
> > Cc: Joerg Roedel <jroedel@suse.de>
> > Cc: Herbert Xu <herbert@gondor.apana.org.au>
> > Cc: David Rientjes <rientjes@google.com>
> > Cc: John Allen <john.allen@amd.com>
> > Cc: "David S. Miller" <davem@davemloft.net>
> > Cc: Paolo Bonzini <pbonzini@redhat.com> (
> > Cc: linux-crypto@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > ---
> >   drivers/crypto/ccp/sev-dev.c | 186 ++++++++++++++++++++++++++++++++---
> >   include/linux/psp-sev.h      |  21 ++++
> >   2 files changed, 192 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> > index b568ae734857..c8718b4cbc93 100644
> > --- a/drivers/crypto/ccp/sev-dev.c
> > +++ b/drivers/crypto/ccp/sev-dev.c
> > @@ -22,6 +22,7 @@
> >   #include <linux/firmware.h>
> >   #include <linux/gfp.h>
> >   #include <linux/cpufeature.h>
> > +#include <linux/fs.h>
> >
> >   #include <asm/smp.h>
> >
> > @@ -43,6 +44,10 @@ static int psp_probe_timeout = 5;
> >   module_param(psp_probe_timeout, int, 0644);
> >   MODULE_PARM_DESC(psp_probe_timeout, " default timeout value, in seconds, during PSP device probe");
> >
> > +static char *init_ex_path;
> > +module_param(init_ex_path, charp, 0660);
> > +MODULE_PARM_DESC(init_ex_path, " Path for INIT_EX data; if set try INIT_EX");
> > +
> >   MODULE_FIRMWARE("amd/amd_sev_fam17h_model0xh.sbin"); /* 1st gen EPYC */
> >   MODULE_FIRMWARE("amd/amd_sev_fam17h_model3xh.sbin"); /* 2nd gen EPYC */
> >   MODULE_FIRMWARE("amd/amd_sev_fam19h_model0xh.sbin"); /* 3rd gen EPYC */
> > @@ -58,6 +63,14 @@ static int psp_timeout;
> >   #define SEV_ES_TMR_SIZE             (1024 * 1024)
> >   static void *sev_es_tmr;
> >
> > +/* INIT_EX NV Storage:
> > + *   The NV Storage is a 32Kb area and must be 4Kb page aligned.  Use the page
> > + *   allocator to allocate the memory, which will return aligned memory for the
> > + *   specified allocation order.
> > + */
> > +#define NV_LENGTH (32 << 10)
>
> Just me, but I think '32 * 1024' would be a bit clearer.

SGTM and more consistent with SEV_ES_TMR_SIZE.

>
> > +static void *sev_init_ex_nv_address;
> > +
> >   static inline bool sev_version_greater_or_equal(u8 maj, u8 min)
> >   {
> >       struct sev_device *sev = psp_master->sev_data;
> > @@ -135,6 +148,7 @@ static int sev_cmd_buffer_len(int cmd)
> >       case SEV_CMD_GET_ID:                    return sizeof(struct sev_data_get_id);
> >       case SEV_CMD_ATTESTATION_REPORT:        return sizeof(struct sev_data_attestation_report);
> >       case SEV_CMD_SEND_CANCEL:                       return sizeof(struct sev_data_send_cancel);
> > +     case SEV_CMD_INIT_EX:                   return sizeof(struct sev_data_init_ex);
>
> Maybe move this to just under the SEV_CMD_INIT: case statement?
>
> >       default:                                return 0;
> >       }
> >
> > @@ -156,6 +170,89 @@ static void *sev_fw_alloc(unsigned long len)
> >       return page_address(page);
> >   }
> >
> > +static int sev_read_nv_memory(void)
> > +{
> > +     struct file *fp;
> > +     ssize_t nread;
> > +
> > +     if (!sev_init_ex_nv_address)
> > +             return -EOPNOTSUPP;
> > +
> > +     fp = filp_open(init_ex_path, O_RDONLY, 0);
> > +     if (IS_ERR(fp)) {
> > +             dev_err(psp_master->dev, "sev could not open file for read\n");
> > +             return PTR_ERR(fp);
> > +     }
> > +
> > +     nread = kernel_read(fp, sev_init_ex_nv_address, NV_LENGTH, 0);
> > +     dev_dbg(psp_master->dev, "sev NV read %d bytes\n", nread);
> > +     filp_close(fp, NULL);
>
> Add a blank line here.
>
> > +     return 0;
> > +}
> > +
> > +static int sev_write_nv_memory(void)
> > +{
> > +     struct sev_device *sev = psp_master->sev_data;
> > +     struct file *fp;
> > +     loff_t offset = 0;
> > +     int ret;
> > +
> > +     if (!sev_init_ex_nv_address)
> > +             return -EOPNOTSUPP;
> > +
> > +     fp = filp_open(init_ex_path, O_CREAT | O_WRONLY, 0600);
> > +     if (IS_ERR(fp)) {
> > +             dev_err(sev->dev, "sev NV data could not be created\n");
> > +             return PTR_ERR(fp);
> > +     }
>
> Add a blank line here.
>
> > +     ret = kernel_write(fp, sev_init_ex_nv_address, NV_LENGTH, &offset);
> > +     vfs_fsync(fp, 0);
> > +     filp_close(fp, NULL);
> > +
> > +     if (ret != NV_LENGTH) {
> > +             dev_err(sev->dev,
> > +                     "failed to write %d bytes to non volatile memory area, ret=%lu\n",
> > +                     NV_LENGTH, ret);
> > +             if (ret >= 0)
> > +                     return -EIO;
> > +             return ret;
> > +     }
> > +
> > +     dev_dbg(sev->dev, "wrote to non volatile memory area\n");
>
> Add a blank line here.

Added all these blank lines.

>
> > +     return 0;
> > +}
> > +
> > +static void sev_write_nv_memory_if_required(int cmd_id)
> > +{
> > +     struct sev_device *sev = psp_master->sev_data;
> > +     int ret;
> > +
> > +     if (!sev_init_ex_nv_address)
> > +             return;
> > +
> > +     /*
> > +      * Only a few platform commands modify the SPI/NV area,
> > +      * but none of the non-platform commands do. Only INIT,
>
> maybe say INIT(_EX)?

Done.

>
> > +      * PLATFORM_RESET, PEK_GEN, PEK_CERT_IMPORT, and
> > +      * PDH_GEN do.
>
> Does SHUTDOWN modify the SPI/NV area? Otherwise a separate comment about
> why it is included below.

Double checked the FW doc looks like it does not. Richard can you
confirm? If it doesn't I'll remove this case.

 >
> > +      */
> > +     switch (cmd_id) {
> > +     case SEV_CMD_FACTORY_RESET:
> > +     case SEV_CMD_INIT_EX:
> > +     case SEV_CMD_PDH_GEN:
> > +     case SEV_CMD_PEK_CERT_IMPORT:
> > +     case SEV_CMD_PEK_GEN:
> > +     case SEV_CMD_SHUTDOWN:
> > +             break;
> > +     default:
> > +             return;
> > +     };
> > +
> > +     ret = sev_write_nv_memory();
> > +     if (ret)
> > +             dev_err(sev->dev, "sev NV write failed %d\n", ret);
>
> You already have error messages in the sev_write_nv_memory() function,
> this one probably isn't needed.

Removed.

>
> > +}
> > +
> >   static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
> >   {
> >       struct psp_device *psp = psp_master;
> > @@ -225,6 +322,8 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
> >               dev_dbg(sev->dev, "sev command %#x failed (%#010x)\n",
> >                       cmd, reg & PSP_CMDRESP_ERR_MASK);
> >               ret = -EIO;
> > +     } else {
> > +             sev_write_nv_memory_if_required(cmd);
> >       }
> >
> >       print_hex_dump_debug("(out): ", DUMP_PREFIX_OFFSET, 16, 2, data,
> > @@ -251,22 +350,42 @@ static int sev_do_cmd(int cmd, void *data, int *psp_ret)
> >       return rc;
> >   }
> >
> > -static int __sev_platform_init_locked(int *error)
> > +static int __sev_init_locked(int *error)
> >   {
> > -     struct psp_device *psp = psp_master;
> >       struct sev_data_init data;
> > -     struct sev_device *sev;
> > -     int rc = 0;
> >
> > -     if (!psp || !psp->sev_data)
> > -             return -ENODEV;
> > +     memset(&data, 0, sizeof(data));
> > +     if (sev_es_tmr) {
> > +             u64 tmr_pa;
> >
> > -     sev = psp->sev_data;
> > +             /*
> > +              * Do not include the encryption mask on the physical
> > +              * address of the TMR (firmware should clear it anyway).
> > +              */
> > +             tmr_pa = __pa(sev_es_tmr);
> >
> > -     if (sev->state == SEV_STATE_INIT)
> > -             return 0;
> > +             data.flags |= SEV_INIT_FLAGS_SEV_ES;
> > +             data.tmr_address = tmr_pa;
> > +             data.tmr_len = SEV_ES_TMR_SIZE;
> > +     }
> > +
> > +     return __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> > +}
> > +
> > +static int __sev_init_ex_locked(int *error)
> > +{
> > +     struct sev_data_init_ex data;
> > +     int ret;
> >
> >       memset(&data, 0, sizeof(data));
> > +     data.length = sizeof(data);
> > +     data.nv_address = __psp_pa(sev_init_ex_nv_address);
> > +     data.nv_len = NV_LENGTH;
> > +
> > +     ret = sev_read_nv_memory();
> > +     if (ret)
> > +             return ret;
> > +
> >       if (sev_es_tmr) {
> >               u64 tmr_pa;
> >
> > @@ -276,12 +395,30 @@ static int __sev_platform_init_locked(int *error)
> >                */
> >               tmr_pa = __pa(sev_es_tmr);
> >
> > -             data.flags |= SEV_INIT_FLAGS_SEV_ES;
>
> Inadvertant deletion?

Oops only testing with SEV guests. Will test with ES too. Fixed.

>
> >               data.tmr_address = tmr_pa;
> >               data.tmr_len = SEV_ES_TMR_SIZE;
> >       }
>
> Add a blank line here.
>
> > +     return __sev_do_cmd_locked(SEV_CMD_INIT_EX, &data, error);
> > +}
> > +
> > +static int __sev_platform_init_locked(int *error)
> > +{
> > +     struct psp_device *psp = psp_master;
> > +     struct sev_device *sev;
> > +     int rc;
> > +     int (*init_function)(int *error) = sev_init_ex_nv_address ?
> > +                     __sev_init_ex_locked :
> > +                     __sev_init_locked;
>
> This seems a bit much in the declaration. How about moving the assignment
> down to just before the first call?

Done.

>
> > +
> > +     if (!psp || !psp->sev_data)
> > +             return -ENODEV;
> > +
> > +     sev = psp->sev_data;
> >
> > -     rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> > +     if (sev->state == SEV_STATE_INIT)
> > +             return 0;
> > +
> > +     rc = init_function(error);
> >       if (rc && *error == SEV_RET_SECURE_DATA_INVALID) {
> >               /*
> >                * INIT command returned an integrity check failure
> > @@ -290,8 +427,8 @@ static int __sev_platform_init_locked(int *error)
> >                * failed and persistent state has been erased.
> >                * Retrying INIT command here should succeed.
> >                */
> > -             dev_dbg(sev->dev, "SEV: retrying INIT command");
> > -             rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> > +             dev_err(sev->dev, "SEV: retrying INIT command");
>
> Maybe dev_notice() instead of dev_err()?

Done.,

>
> > +             rc = init_function(error);
> >       }
> >
> >       if (rc)
> > @@ -307,7 +444,7 @@ static int __sev_platform_init_locked(int *error)
> >
> >       dev_dbg(sev->dev, "SEV firmware initialized\n");
> >
> > -     return rc;
> > +     return 0;
> >   }
> >
> >   int sev_platform_init(int *error)
> > @@ -987,7 +1124,7 @@ static int sev_misc_init(struct sev_device *sev)
> >
> >       init_waitqueue_head(&sev->int_queue);
> >       sev->misc = misc_dev;
> > -     dev_dbg(dev, "registered SEV device\n");
> > +     dev_err(dev, "registered SEV device\n");
>
> Not sure this is a necessary change... but, if you don't want this as a
> dev_dbg() then it should be a dev_info(), because it is not an error.

Oops this was my debugging. Reverted.

>
> >
> >       return 0;
> >   }
> > @@ -1061,6 +1198,12 @@ static void sev_firmware_shutdown(struct sev_device *sev)
> >                          get_order(SEV_ES_TMR_SIZE));
> >               sev_es_tmr = NULL;
> >       }
> > +
> > +     if (sev_init_ex_nv_address) {
> > +             free_pages((unsigned long)sev_init_ex_nv_address,
> > +                        get_order(NV_LENGTH));
> > +             sev_init_ex_nv_address = NULL;
> > +     }
> >   }
> >
> >   void sev_dev_destroy(struct psp_device *psp)
> > @@ -1105,6 +1248,19 @@ void sev_pci_init(void)
> >           sev_update_firmware(sev->dev) == 0)
> >               sev_get_api_version();
> >
> > +     /* If an init_ex_path is provided rely on INIT_EX for PSP initialization
> > +      * instead of INIT.
> > +      */
> > +     if (init_ex_path) {
> > +             sev_init_ex_nv_address = sev_fw_alloc(NV_LENGTH);
> > +             if (!sev_init_ex_nv_address) {
> > +                     dev_warn(
>
> Shouldn't this be a dev_err(), since you are erroring out?
>
> > +                             sev->dev,
>
> Move this up to the previous line, i.e.: dev_err(sev->dev,
>
> > +                             "SEV: INIT_EX NV storage allocation failed, INIT-EX support unavailable\n");
>
> Since you're erroring out, probably enough to just have the first part of
> that message. But if not:
>
> s/INIT-EX/INIT_EX/

Fixed this log line.

>
> Thanks,
> Tom
>
> > +                     goto err;
> > +             }
> > +     }
> > +
> >       /* Obtain the TMR memory area for SEV-ES use */
> >       sev_es_tmr = sev_fw_alloc(SEV_ES_TMR_SIZE);
> >       if (!sev_es_tmr)
> > diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
> > index d48a7192e881..1595088c428b 100644
> > --- a/include/linux/psp-sev.h
> > +++ b/include/linux/psp-sev.h
> > @@ -52,6 +52,7 @@ enum sev_cmd {
> >       SEV_CMD_DF_FLUSH                = 0x00A,
> >       SEV_CMD_DOWNLOAD_FIRMWARE       = 0x00B,
> >       SEV_CMD_GET_ID                  = 0x00C,
> > +     SEV_CMD_INIT_EX                 = 0x00D,
> >
> >       /* Guest commands */
> >       SEV_CMD_DECOMMISSION            = 0x020,
> > @@ -102,6 +103,26 @@ struct sev_data_init {
> >       u32 tmr_len;                    /* In */
> >   } __packed;
> >
> > +/**
> > + * struct sev_data_init_ex - INIT_EX command parameters
> > + *
> > + * @length: len of the command buffer read by the PSP
> > + * @flags: processing flags
> > + * @tmr_address: system physical address used for SEV-ES
> > + * @tmr_len: len of tmr_address
> > + * @nv_address: system physical address used for PSP NV storage
> > + * @nv_len: len of nv_address
> > + */
> > +struct sev_data_init_ex {
> > +     u32 length;                     /* In */
> > +     u32 flags;                      /* In */
> > +     u64 tmr_address;                /* In */
> > +     u32 tmr_len;                    /* In */
> > +     u32 reserved;                   /* In */
> > +     u64 nv_address;                 /* In/Out */
> > +     u32 nv_len;                     /* In */
> > +} __packed;
> > +
> >   #define SEV_INIT_FLAGS_SEV_ES       0x01
> >
> >   /**
> >

Re: [PATCH 4/4] crypto: ccp - Add SEV_INIT_EX support

[Tom Lendacky]

On 10/29/21 12:26 PM, Peter Gonda wrote:
> On Fri, Oct 29, 2021 at 8:45 AM Tom Lendacky <thomas.lendacky@amd.com> wrote:
>>
>> On 10/28/21 12:57 PM, Peter Gonda wrote:
>>> From: David Rientjes <rientjes@google.com>
>>>
>>> Add new module parameter to allow users to use SEV_INIT_EX instead of
>>> SEV_INIT. This helps users who lock their SPI bus to use the PSP for SEV
>>> functionality. The 'init_ex_path' parameter defaults to NULL which means
>>> the kernel will use SEV_INIT, if a path is specified SEV_INIT_EX will be
>>> used with the data found at the path. On certain PSP commands this
>>> file is written to as the PSP updates the NV memory region. Depending on
>>> file system initialization this file open may fail during module init
>>> but the CCP driver for SEV already has sufficient retries for platform
>>> initialization. During normal operation of PSP system and SEV commands
>>> if the PSP has not been initialized it is at run time.
>>
>> IIUC, it looks as though the file has to exist before the very first use,
>> otherwise the initialization will fail. Did you consider checking for the
>> presence of the file first and, if not there, just using a memory area of
>> all f's (as documented in the SEV API)? Then on successful INIT, the
>> memory would be written and the file created.
> 
> Thats a great idea. I'll add that functionality.

It all depends on how you want to treat the absence of the file. I can see 
use cases for both creating automatically or failing (e.g. there was a 
typo in the path).

> 
>>
>> Either way, probably worth adding to the commit message. And if you stay
>> with having to pre-allocate the file, it's worth adding to the SEV
>> documentation what is required to be done to initialize the file.
>>
>> Although, INIT_EX is probably worth adding to the SEV documentation in
>> general.
> 
> Is this (https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.kernel.org%2Fdoc%2FDocumentation%2Fvirt%2Fkvm%2Famd-memory-encryption.rst&amp;data=04%7C01%7CThomas.Lendacky%40amd.com%7Cd9723c44ea764cc7540e08d99b013bbb%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637711251880475091%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=WJtwak6F%2Bn7PC8R1ohsC%2FNKGi7oDWFTi72NJDteyQoM%3D&amp;reserved=0)
> the documentation you were referring too? If so I can add another
> patch before this one to document INIT, then add the INIT_EX
> documentation here. Or were you thinking something else?

Yes, that's the documentation I was referring to. I think you can just add 
something to the KVM_SEV_INIT entry that talks about the new module 
parameter and a bit about the file and how that file will be used. If you 
decide to stay with a pre-created file, some details about how to create 
the file would then be appropriate.

Thanks,
Tom

> 
>>
>>>
>>> Signed-off-by: David Rientjes <rientjes@google.com>
>>> Co-developed-by: Peter Gonda <pgonda@google.com>
>>> Signed-off-by: Peter Gonda <pgonda@google.com>
>>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>>> Cc: Brijesh Singh <brijesh.singh@amd.com>
>>> Cc: Marc Orr <marcorr@google.com>
>>> Cc: Joerg Roedel <jroedel@suse.de>
>>> Cc: Herbert Xu <herbert@gondor.apana.org.au>
>>> Cc: David Rientjes <rientjes@google.com>
>>> Cc: John Allen <john.allen@amd.com>
>>> Cc: "David S. Miller" <davem@davemloft.net>
>>> Cc: Paolo Bonzini <pbonzini@redhat.com> (
>>> Cc: linux-crypto@vger.kernel.org
>>> Cc: linux-kernel@vger.kernel.org
>>> ---
>>>    drivers/crypto/ccp/sev-dev.c | 186 ++++++++++++++++++++++++++++++++---
>>>    include/linux/psp-sev.h      |  21 ++++
>>>    2 files changed, 192 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
>>> index b568ae734857..c8718b4cbc93 100644
>>> --- a/drivers/crypto/ccp/sev-dev.c
>>> +++ b/drivers/crypto/ccp/sev-dev.c
>>> @@ -22,6 +22,7 @@
>>>    #include <linux/firmware.h>
>>>    #include <linux/gfp.h>
>>>    #include <linux/cpufeature.h>
>>> +#include <linux/fs.h>
>>>
>>>    #include <asm/smp.h>
>>>
>>> @@ -43,6 +44,10 @@ static int psp_probe_timeout = 5;
>>>    module_param(psp_probe_timeout, int, 0644);
>>>    MODULE_PARM_DESC(psp_probe_timeout, " default timeout value, in seconds, during PSP device probe");
>>>
>>> +static char *init_ex_path;
>>> +module_param(init_ex_path, charp, 0660);
>>> +MODULE_PARM_DESC(init_ex_path, " Path for INIT_EX data; if set try INIT_EX");
>>> +
>>>    MODULE_FIRMWARE("amd/amd_sev_fam17h_model0xh.sbin"); /* 1st gen EPYC */
>>>    MODULE_FIRMWARE("amd/amd_sev_fam17h_model3xh.sbin"); /* 2nd gen EPYC */
>>>    MODULE_FIRMWARE("amd/amd_sev_fam19h_model0xh.sbin"); /* 3rd gen EPYC */
>>> @@ -58,6 +63,14 @@ static int psp_timeout;
>>>    #define SEV_ES_TMR_SIZE             (1024 * 1024)
>>>    static void *sev_es_tmr;
>>>
>>> +/* INIT_EX NV Storage:
>>> + *   The NV Storage is a 32Kb area and must be 4Kb page aligned.  Use the page
>>> + *   allocator to allocate the memory, which will return aligned memory for the
>>> + *   specified allocation order.
>>> + */
>>> +#define NV_LENGTH (32 << 10)
>>
>> Just me, but I think '32 * 1024' would be a bit clearer.
> 
> SGTM and more consistent with SEV_ES_TMR_SIZE.
> 
>>
>>> +static void *sev_init_ex_nv_address;
>>> +
>>>    static inline bool sev_version_greater_or_equal(u8 maj, u8 min)
>>>    {
>>>        struct sev_device *sev = psp_master->sev_data;
>>> @@ -135,6 +148,7 @@ static int sev_cmd_buffer_len(int cmd)
>>>        case SEV_CMD_GET_ID:                    return sizeof(struct sev_data_get_id);
>>>        case SEV_CMD_ATTESTATION_REPORT:        return sizeof(struct sev_data_attestation_report);
>>>        case SEV_CMD_SEND_CANCEL:                       return sizeof(struct sev_data_send_cancel);
>>> +     case SEV_CMD_INIT_EX:                   return sizeof(struct sev_data_init_ex);
>>
>> Maybe move this to just under the SEV_CMD_INIT: case statement?
>>
>>>        default:                                return 0;
>>>        }
>>>
>>> @@ -156,6 +170,89 @@ static void *sev_fw_alloc(unsigned long len)
>>>        return page_address(page);
>>>    }
>>>
>>> +static int sev_read_nv_memory(void)
>>> +{
>>> +     struct file *fp;
>>> +     ssize_t nread;
>>> +
>>> +     if (!sev_init_ex_nv_address)
>>> +             return -EOPNOTSUPP;
>>> +
>>> +     fp = filp_open(init_ex_path, O_RDONLY, 0);
>>> +     if (IS_ERR(fp)) {
>>> +             dev_err(psp_master->dev, "sev could not open file for read\n");
>>> +             return PTR_ERR(fp);
>>> +     }
>>> +
>>> +     nread = kernel_read(fp, sev_init_ex_nv_address, NV_LENGTH, 0);
>>> +     dev_dbg(psp_master->dev, "sev NV read %d bytes\n", nread);
>>> +     filp_close(fp, NULL);
>>
>> Add a blank line here.
>>
>>> +     return 0;
>>> +}
>>> +
>>> +static int sev_write_nv_memory(void)
>>> +{
>>> +     struct sev_device *sev = psp_master->sev_data;
>>> +     struct file *fp;
>>> +     loff_t offset = 0;
>>> +     int ret;
>>> +
>>> +     if (!sev_init_ex_nv_address)
>>> +             return -EOPNOTSUPP;
>>> +
>>> +     fp = filp_open(init_ex_path, O_CREAT | O_WRONLY, 0600);
>>> +     if (IS_ERR(fp)) {
>>> +             dev_err(sev->dev, "sev NV data could not be created\n");
>>> +             return PTR_ERR(fp);
>>> +     }
>>
>> Add a blank line here.
>>
>>> +     ret = kernel_write(fp, sev_init_ex_nv_address, NV_LENGTH, &offset);
>>> +     vfs_fsync(fp, 0);
>>> +     filp_close(fp, NULL);
>>> +
>>> +     if (ret != NV_LENGTH) {
>>> +             dev_err(sev->dev,
>>> +                     "failed to write %d bytes to non volatile memory area, ret=%lu\n",
>>> +                     NV_LENGTH, ret);
>>> +             if (ret >= 0)
>>> +                     return -EIO;
>>> +             return ret;
>>> +     }
>>> +
>>> +     dev_dbg(sev->dev, "wrote to non volatile memory area\n");
>>
>> Add a blank line here.
> 
> Added all these blank lines.
> 
>>
>>> +     return 0;
>>> +}
>>> +
>>> +static void sev_write_nv_memory_if_required(int cmd_id)
>>> +{
>>> +     struct sev_device *sev = psp_master->sev_data;
>>> +     int ret;
>>> +
>>> +     if (!sev_init_ex_nv_address)
>>> +             return;
>>> +
>>> +     /*
>>> +      * Only a few platform commands modify the SPI/NV area,
>>> +      * but none of the non-platform commands do. Only INIT,
>>
>> maybe say INIT(_EX)?
> 
> Done.
> 
>>
>>> +      * PLATFORM_RESET, PEK_GEN, PEK_CERT_IMPORT, and
>>> +      * PDH_GEN do.
>>
>> Does SHUTDOWN modify the SPI/NV area? Otherwise a separate comment about
>> why it is included below.
> 
> Double checked the FW doc looks like it does not. Richard can you
> confirm? If it doesn't I'll remove this case.
> 
>   >
>>> +      */
>>> +     switch (cmd_id) {
>>> +     case SEV_CMD_FACTORY_RESET:
>>> +     case SEV_CMD_INIT_EX:
>>> +     case SEV_CMD_PDH_GEN:
>>> +     case SEV_CMD_PEK_CERT_IMPORT:
>>> +     case SEV_CMD_PEK_GEN:
>>> +     case SEV_CMD_SHUTDOWN:
>>> +             break;
>>> +     default:
>>> +             return;
>>> +     };
>>> +
>>> +     ret = sev_write_nv_memory();
>>> +     if (ret)
>>> +             dev_err(sev->dev, "sev NV write failed %d\n", ret);
>>
>> You already have error messages in the sev_write_nv_memory() function,
>> this one probably isn't needed.
> 
> Removed.
> 
>>
>>> +}
>>> +
>>>    static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
>>>    {
>>>        struct psp_device *psp = psp_master;
>>> @@ -225,6 +322,8 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
>>>                dev_dbg(sev->dev, "sev command %#x failed (%#010x)\n",
>>>                        cmd, reg & PSP_CMDRESP_ERR_MASK);
>>>                ret = -EIO;
>>> +     } else {
>>> +             sev_write_nv_memory_if_required(cmd);
>>>        }
>>>
>>>        print_hex_dump_debug("(out): ", DUMP_PREFIX_OFFSET, 16, 2, data,
>>> @@ -251,22 +350,42 @@ static int sev_do_cmd(int cmd, void *data, int *psp_ret)
>>>        return rc;
>>>    }
>>>
>>> -static int __sev_platform_init_locked(int *error)
>>> +static int __sev_init_locked(int *error)
>>>    {
>>> -     struct psp_device *psp = psp_master;
>>>        struct sev_data_init data;
>>> -     struct sev_device *sev;
>>> -     int rc = 0;
>>>
>>> -     if (!psp || !psp->sev_data)
>>> -             return -ENODEV;
>>> +     memset(&data, 0, sizeof(data));
>>> +     if (sev_es_tmr) {
>>> +             u64 tmr_pa;
>>>
>>> -     sev = psp->sev_data;
>>> +             /*
>>> +              * Do not include the encryption mask on the physical
>>> +              * address of the TMR (firmware should clear it anyway).
>>> +              */
>>> +             tmr_pa = __pa(sev_es_tmr);
>>>
>>> -     if (sev->state == SEV_STATE_INIT)
>>> -             return 0;
>>> +             data.flags |= SEV_INIT_FLAGS_SEV_ES;
>>> +             data.tmr_address = tmr_pa;
>>> +             data.tmr_len = SEV_ES_TMR_SIZE;
>>> +     }
>>> +
>>> +     return __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
>>> +}
>>> +
>>> +static int __sev_init_ex_locked(int *error)
>>> +{
>>> +     struct sev_data_init_ex data;
>>> +     int ret;
>>>
>>>        memset(&data, 0, sizeof(data));
>>> +     data.length = sizeof(data);
>>> +     data.nv_address = __psp_pa(sev_init_ex_nv_address);
>>> +     data.nv_len = NV_LENGTH;
>>> +
>>> +     ret = sev_read_nv_memory();
>>> +     if (ret)
>>> +             return ret;
>>> +
>>>        if (sev_es_tmr) {
>>>                u64 tmr_pa;
>>>
>>> @@ -276,12 +395,30 @@ static int __sev_platform_init_locked(int *error)
>>>                 */
>>>                tmr_pa = __pa(sev_es_tmr);
>>>
>>> -             data.flags |= SEV_INIT_FLAGS_SEV_ES;
>>
>> Inadvertant deletion?
> 
> Oops only testing with SEV guests. Will test with ES too. Fixed.
> 
>>
>>>                data.tmr_address = tmr_pa;
>>>                data.tmr_len = SEV_ES_TMR_SIZE;
>>>        }
>>
>> Add a blank line here.
>>
>>> +     return __sev_do_cmd_locked(SEV_CMD_INIT_EX, &data, error);
>>> +}
>>> +
>>> +static int __sev_platform_init_locked(int *error)
>>> +{
>>> +     struct psp_device *psp = psp_master;
>>> +     struct sev_device *sev;
>>> +     int rc;
>>> +     int (*init_function)(int *error) = sev_init_ex_nv_address ?
>>> +                     __sev_init_ex_locked :
>>> +                     __sev_init_locked;
>>
>> This seems a bit much in the declaration. How about moving the assignment
>> down to just before the first call?
> 
> Done.
> 
>>
>>> +
>>> +     if (!psp || !psp->sev_data)
>>> +             return -ENODEV;
>>> +
>>> +     sev = psp->sev_data;
>>>
>>> -     rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
>>> +     if (sev->state == SEV_STATE_INIT)
>>> +             return 0;
>>> +
>>> +     rc = init_function(error);
>>>        if (rc && *error == SEV_RET_SECURE_DATA_INVALID) {
>>>                /*
>>>                 * INIT command returned an integrity check failure
>>> @@ -290,8 +427,8 @@ static int __sev_platform_init_locked(int *error)
>>>                 * failed and persistent state has been erased.
>>>                 * Retrying INIT command here should succeed.
>>>                 */
>>> -             dev_dbg(sev->dev, "SEV: retrying INIT command");
>>> -             rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
>>> +             dev_err(sev->dev, "SEV: retrying INIT command");
>>
>> Maybe dev_notice() instead of dev_err()?
> 
> Done.,
> 
>>
>>> +             rc = init_function(error);
>>>        }
>>>
>>>        if (rc)
>>> @@ -307,7 +444,7 @@ static int __sev_platform_init_locked(int *error)
>>>
>>>        dev_dbg(sev->dev, "SEV firmware initialized\n");
>>>
>>> -     return rc;
>>> +     return 0;
>>>    }
>>>
>>>    int sev_platform_init(int *error)
>>> @@ -987,7 +1124,7 @@ static int sev_misc_init(struct sev_device *sev)
>>>
>>>        init_waitqueue_head(&sev->int_queue);
>>>        sev->misc = misc_dev;
>>> -     dev_dbg(dev, "registered SEV device\n");
>>> +     dev_err(dev, "registered SEV device\n");
>>
>> Not sure this is a necessary change... but, if you don't want this as a
>> dev_dbg() then it should be a dev_info(), because it is not an error.
> 
> Oops this was my debugging. Reverted.
> 
>>
>>>
>>>        return 0;
>>>    }
>>> @@ -1061,6 +1198,12 @@ static void sev_firmware_shutdown(struct sev_device *sev)
>>>                           get_order(SEV_ES_TMR_SIZE));
>>>                sev_es_tmr = NULL;
>>>        }
>>> +
>>> +     if (sev_init_ex_nv_address) {
>>> +             free_pages((unsigned long)sev_init_ex_nv_address,
>>> +                        get_order(NV_LENGTH));
>>> +             sev_init_ex_nv_address = NULL;
>>> +     }
>>>    }
>>>
>>>    void sev_dev_destroy(struct psp_device *psp)
>>> @@ -1105,6 +1248,19 @@ void sev_pci_init(void)
>>>            sev_update_firmware(sev->dev) == 0)
>>>                sev_get_api_version();
>>>
>>> +     /* If an init_ex_path is provided rely on INIT_EX for PSP initialization
>>> +      * instead of INIT.
>>> +      */
>>> +     if (init_ex_path) {
>>> +             sev_init_ex_nv_address = sev_fw_alloc(NV_LENGTH);
>>> +             if (!sev_init_ex_nv_address) {
>>> +                     dev_warn(
>>
>> Shouldn't this be a dev_err(), since you are erroring out?
>>
>>> +                             sev->dev,
>>
>> Move this up to the previous line, i.e.: dev_err(sev->dev,
>>
>>> +                             "SEV: INIT_EX NV storage allocation failed, INIT-EX support unavailable\n");
>>
>> Since you're erroring out, probably enough to just have the first part of
>> that message. But if not:
>>
>> s/INIT-EX/INIT_EX/
> 
> Fixed this log line.
> 
>>
>> Thanks,
>> Tom
>>
>>> +                     goto err;
>>> +             }
>>> +     }
>>> +
>>>        /* Obtain the TMR memory area for SEV-ES use */
>>>        sev_es_tmr = sev_fw_alloc(SEV_ES_TMR_SIZE);
>>>        if (!sev_es_tmr)
>>> diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
>>> index d48a7192e881..1595088c428b 100644
>>> --- a/include/linux/psp-sev.h
>>> +++ b/include/linux/psp-sev.h
>>> @@ -52,6 +52,7 @@ enum sev_cmd {
>>>        SEV_CMD_DF_FLUSH                = 0x00A,
>>>        SEV_CMD_DOWNLOAD_FIRMWARE       = 0x00B,
>>>        SEV_CMD_GET_ID                  = 0x00C,
>>> +     SEV_CMD_INIT_EX                 = 0x00D,
>>>
>>>        /* Guest commands */
>>>        SEV_CMD_DECOMMISSION            = 0x020,
>>> @@ -102,6 +103,26 @@ struct sev_data_init {
>>>        u32 tmr_len;                    /* In */
>>>    } __packed;
>>>
>>> +/**
>>> + * struct sev_data_init_ex - INIT_EX command parameters
>>> + *
>>> + * @length: len of the command buffer read by the PSP
>>> + * @flags: processing flags
>>> + * @tmr_address: system physical address used for SEV-ES
>>> + * @tmr_len: len of tmr_address
>>> + * @nv_address: system physical address used for PSP NV storage
>>> + * @nv_len: len of nv_address
>>> + */
>>> +struct sev_data_init_ex {
>>> +     u32 length;                     /* In */
>>> +     u32 flags;                      /* In */
>>> +     u64 tmr_address;                /* In */
>>> +     u32 tmr_len;                    /* In */
>>> +     u32 reserved;                   /* In */
>>> +     u64 nv_address;                 /* In/Out */
>>> +     u32 nv_len;                     /* In */
>>> +} __packed;
>>> +
>>>    #define SEV_INIT_FLAGS_SEV_ES       0x01
>>>
>>>    /**
>>>

Re: [PATCH 4/4] crypto: ccp - Add SEV_INIT_EX support

[Peter Gonda]

On Fri, Oct 29, 2021 at 11:56 AM Tom Lendacky <thomas.lendacky@amd.com> wrote:
>
> On 10/29/21 12:26 PM, Peter Gonda wrote:
> > On Fri, Oct 29, 2021 at 8:45 AM Tom Lendacky <thomas.lendacky@amd.com> wrote:
> >>
> >> On 10/28/21 12:57 PM, Peter Gonda wrote:
> >>> From: David Rientjes <rientjes@google.com>
> >>>
> >>> Add new module parameter to allow users to use SEV_INIT_EX instead of
> >>> SEV_INIT. This helps users who lock their SPI bus to use the PSP for SEV
> >>> functionality. The 'init_ex_path' parameter defaults to NULL which means
> >>> the kernel will use SEV_INIT, if a path is specified SEV_INIT_EX will be
> >>> used with the data found at the path. On certain PSP commands this
> >>> file is written to as the PSP updates the NV memory region. Depending on
> >>> file system initialization this file open may fail during module init
> >>> but the CCP driver for SEV already has sufficient retries for platform
> >>> initialization. During normal operation of PSP system and SEV commands
> >>> if the PSP has not been initialized it is at run time.
> >>
> >> IIUC, it looks as though the file has to exist before the very first use,
> >> otherwise the initialization will fail. Did you consider checking for the
> >> presence of the file first and, if not there, just using a memory area of
> >> all f's (as documented in the SEV API)? Then on successful INIT, the
> >> memory would be written and the file created.
> >
> > Thats a great idea. I'll add that functionality.
>
> It all depends on how you want to treat the absence of the file. I can see
> use cases for both creating automatically or failing (e.g. there was a
> typo in the path).
>
> >
> >>
> >> Either way, probably worth adding to the commit message. And if you stay
> >> with having to pre-allocate the file, it's worth adding to the SEV
> >> documentation what is required to be done to initialize the file.
> >>
> >> Although, INIT_EX is probably worth adding to the SEV documentation in
> >> general.
> >
> > Is this (https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.kernel.org%2Fdoc%2FDocumentation%2Fvirt%2Fkvm%2Famd-memory-encryption.rst&amp;data=04%7C01%7CThomas.Lendacky%40amd.com%7Cd9723c44ea764cc7540e08d99b013bbb%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637711251880475091%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=WJtwak6F%2Bn7PC8R1ohsC%2FNKGi7oDWFTi72NJDteyQoM%3D&amp;reserved=0)
> > the documentation you were referring too? If so I can add another
> > patch before this one to document INIT, then add the INIT_EX
> > documentation here. Or were you thinking something else?
>
> Yes, that's the documentation I was referring to. I think you can just add
> something to the KVM_SEV_INIT entry that talks about the new module
> parameter and a bit about the file and how that file will be used. If you
> decide to stay with a pre-created file, some details about how to create
> the file would then be appropriate.
>

I've updated the doc and the commit message. I went with the do not
auto create option since it keeps the patch simpler, fixes typos as
you've said, and its pretty easy for users to create with a dd command
or something.

> Thanks,
> Tom
>
> >
> >>
> >>>
> >>> Signed-off-by: David Rientjes <rientjes@google.com>
> >>> Co-developed-by: Peter Gonda <pgonda@google.com>
> >>> Signed-off-by: Peter Gonda <pgonda@google.com>
> >>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> >>> Cc: Brijesh Singh <brijesh.singh@amd.com>
> >>> Cc: Marc Orr <marcorr@google.com>
> >>> Cc: Joerg Roedel <jroedel@suse.de>
> >>> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> >>> Cc: David Rientjes <rientjes@google.com>
> >>> Cc: John Allen <john.allen@amd.com>
> >>> Cc: "David S. Miller" <davem@davemloft.net>
> >>> Cc: Paolo Bonzini <pbonzini@redhat.com> (
> >>> Cc: linux-crypto@vger.kernel.org
> >>> Cc: linux-kernel@vger.kernel.org
> >>> ---
> >>>    drivers/crypto/ccp/sev-dev.c | 186 ++++++++++++++++++++++++++++++++---
> >>>    include/linux/psp-sev.h      |  21 ++++
> >>>    2 files changed, 192 insertions(+), 15 deletions(-)
> >>>
> >>> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> >>> index b568ae734857..c8718b4cbc93 100644
> >>> --- a/drivers/crypto/ccp/sev-dev.c
> >>> +++ b/drivers/crypto/ccp/sev-dev.c
> >>> @@ -22,6 +22,7 @@
> >>>    #include <linux/firmware.h>
> >>>    #include <linux/gfp.h>
> >>>    #include <linux/cpufeature.h>
> >>> +#include <linux/fs.h>
> >>>
> >>>    #include <asm/smp.h>
> >>>
> >>> @@ -43,6 +44,10 @@ static int psp_probe_timeout = 5;
> >>>    module_param(psp_probe_timeout, int, 0644);
> >>>    MODULE_PARM_DESC(psp_probe_timeout, " default timeout value, in seconds, during PSP device probe");
> >>>
> >>> +static char *init_ex_path;
> >>> +module_param(init_ex_path, charp, 0660);
> >>> +MODULE_PARM_DESC(init_ex_path, " Path for INIT_EX data; if set try INIT_EX");
> >>> +
> >>>    MODULE_FIRMWARE("amd/amd_sev_fam17h_model0xh.sbin"); /* 1st gen EPYC */
> >>>    MODULE_FIRMWARE("amd/amd_sev_fam17h_model3xh.sbin"); /* 2nd gen EPYC */
> >>>    MODULE_FIRMWARE("amd/amd_sev_fam19h_model0xh.sbin"); /* 3rd gen EPYC */
> >>> @@ -58,6 +63,14 @@ static int psp_timeout;
> >>>    #define SEV_ES_TMR_SIZE             (1024 * 1024)
> >>>    static void *sev_es_tmr;
> >>>
> >>> +/* INIT_EX NV Storage:
> >>> + *   The NV Storage is a 32Kb area and must be 4Kb page aligned.  Use the page
> >>> + *   allocator to allocate the memory, which will return aligned memory for the
> >>> + *   specified allocation order.
> >>> + */
> >>> +#define NV_LENGTH (32 << 10)
> >>
> >> Just me, but I think '32 * 1024' would be a bit clearer.
> >
> > SGTM and more consistent with SEV_ES_TMR_SIZE.
> >
> >>
> >>> +static void *sev_init_ex_nv_address;
> >>> +
> >>>    static inline bool sev_version_greater_or_equal(u8 maj, u8 min)
> >>>    {
> >>>        struct sev_device *sev = psp_master->sev_data;
> >>> @@ -135,6 +148,7 @@ static int sev_cmd_buffer_len(int cmd)
> >>>        case SEV_CMD_GET_ID:                    return sizeof(struct sev_data_get_id);
> >>>        case SEV_CMD_ATTESTATION_REPORT:        return sizeof(struct sev_data_attestation_report);
> >>>        case SEV_CMD_SEND_CANCEL:                       return sizeof(struct sev_data_send_cancel);
> >>> +     case SEV_CMD_INIT_EX:                   return sizeof(struct sev_data_init_ex);
> >>
> >> Maybe move this to just under the SEV_CMD_INIT: case statement?
> >>
> >>>        default:                                return 0;
> >>>        }
> >>>
> >>> @@ -156,6 +170,89 @@ static void *sev_fw_alloc(unsigned long len)
> >>>        return page_address(page);
> >>>    }
> >>>
> >>> +static int sev_read_nv_memory(void)
> >>> +{
> >>> +     struct file *fp;
> >>> +     ssize_t nread;
> >>> +
> >>> +     if (!sev_init_ex_nv_address)
> >>> +             return -EOPNOTSUPP;
> >>> +
> >>> +     fp = filp_open(init_ex_path, O_RDONLY, 0);
> >>> +     if (IS_ERR(fp)) {
> >>> +             dev_err(psp_master->dev, "sev could not open file for read\n");
> >>> +             return PTR_ERR(fp);
> >>> +     }
> >>> +
> >>> +     nread = kernel_read(fp, sev_init_ex_nv_address, NV_LENGTH, 0);
> >>> +     dev_dbg(psp_master->dev, "sev NV read %d bytes\n", nread);
> >>> +     filp_close(fp, NULL);
> >>
> >> Add a blank line here.
> >>
> >>> +     return 0;
> >>> +}
> >>> +
> >>> +static int sev_write_nv_memory(void)
> >>> +{
> >>> +     struct sev_device *sev = psp_master->sev_data;
> >>> +     struct file *fp;
> >>> +     loff_t offset = 0;
> >>> +     int ret;
> >>> +
> >>> +     if (!sev_init_ex_nv_address)
> >>> +             return -EOPNOTSUPP;
> >>> +
> >>> +     fp = filp_open(init_ex_path, O_CREAT | O_WRONLY, 0600);
> >>> +     if (IS_ERR(fp)) {
> >>> +             dev_err(sev->dev, "sev NV data could not be created\n");
> >>> +             return PTR_ERR(fp);
> >>> +     }
> >>
> >> Add a blank line here.
> >>
> >>> +     ret = kernel_write(fp, sev_init_ex_nv_address, NV_LENGTH, &offset);
> >>> +     vfs_fsync(fp, 0);
> >>> +     filp_close(fp, NULL);
> >>> +
> >>> +     if (ret != NV_LENGTH) {
> >>> +             dev_err(sev->dev,
> >>> +                     "failed to write %d bytes to non volatile memory area, ret=%lu\n",
> >>> +                     NV_LENGTH, ret);
> >>> +             if (ret >= 0)
> >>> +                     return -EIO;
> >>> +             return ret;
> >>> +     }
> >>> +
> >>> +     dev_dbg(sev->dev, "wrote to non volatile memory area\n");
> >>
> >> Add a blank line here.
> >
> > Added all these blank lines.
> >
> >>
> >>> +     return 0;
> >>> +}
> >>> +
> >>> +static void sev_write_nv_memory_if_required(int cmd_id)
> >>> +{
> >>> +     struct sev_device *sev = psp_master->sev_data;
> >>> +     int ret;
> >>> +
> >>> +     if (!sev_init_ex_nv_address)
> >>> +             return;
> >>> +
> >>> +     /*
> >>> +      * Only a few platform commands modify the SPI/NV area,
> >>> +      * but none of the non-platform commands do. Only INIT,
> >>
> >> maybe say INIT(_EX)?
> >
> > Done.
> >
> >>
> >>> +      * PLATFORM_RESET, PEK_GEN, PEK_CERT_IMPORT, and
> >>> +      * PDH_GEN do.
> >>
> >> Does SHUTDOWN modify the SPI/NV area? Otherwise a separate comment about
> >> why it is included below.
> >
> > Double checked the FW doc looks like it does not. Richard can you
> > confirm? If it doesn't I'll remove this case.
> >
> >   >
> >>> +      */
> >>> +     switch (cmd_id) {
> >>> +     case SEV_CMD_FACTORY_RESET:
> >>> +     case SEV_CMD_INIT_EX:
> >>> +     case SEV_CMD_PDH_GEN:
> >>> +     case SEV_CMD_PEK_CERT_IMPORT:
> >>> +     case SEV_CMD_PEK_GEN:
> >>> +     case SEV_CMD_SHUTDOWN:
> >>> +             break;
> >>> +     default:
> >>> +             return;
> >>> +     };
> >>> +
> >>> +     ret = sev_write_nv_memory();
> >>> +     if (ret)
> >>> +             dev_err(sev->dev, "sev NV write failed %d\n", ret);
> >>
> >> You already have error messages in the sev_write_nv_memory() function,
> >> this one probably isn't needed.
> >
> > Removed.
> >
> >>
> >>> +}
> >>> +
> >>>    static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
> >>>    {
> >>>        struct psp_device *psp = psp_master;
> >>> @@ -225,6 +322,8 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
> >>>                dev_dbg(sev->dev, "sev command %#x failed (%#010x)\n",
> >>>                        cmd, reg & PSP_CMDRESP_ERR_MASK);
> >>>                ret = -EIO;
> >>> +     } else {
> >>> +             sev_write_nv_memory_if_required(cmd);
> >>>        }
> >>>
> >>>        print_hex_dump_debug("(out): ", DUMP_PREFIX_OFFSET, 16, 2, data,
> >>> @@ -251,22 +350,42 @@ static int sev_do_cmd(int cmd, void *data, int *psp_ret)
> >>>        return rc;
> >>>    }
> >>>
> >>> -static int __sev_platform_init_locked(int *error)
> >>> +static int __sev_init_locked(int *error)
> >>>    {
> >>> -     struct psp_device *psp = psp_master;
> >>>        struct sev_data_init data;
> >>> -     struct sev_device *sev;
> >>> -     int rc = 0;
> >>>
> >>> -     if (!psp || !psp->sev_data)
> >>> -             return -ENODEV;
> >>> +     memset(&data, 0, sizeof(data));
> >>> +     if (sev_es_tmr) {
> >>> +             u64 tmr_pa;
> >>>
> >>> -     sev = psp->sev_data;
> >>> +             /*
> >>> +              * Do not include the encryption mask on the physical
> >>> +              * address of the TMR (firmware should clear it anyway).
> >>> +              */
> >>> +             tmr_pa = __pa(sev_es_tmr);
> >>>
> >>> -     if (sev->state == SEV_STATE_INIT)
> >>> -             return 0;
> >>> +             data.flags |= SEV_INIT_FLAGS_SEV_ES;
> >>> +             data.tmr_address = tmr_pa;
> >>> +             data.tmr_len = SEV_ES_TMR_SIZE;
> >>> +     }
> >>> +
> >>> +     return __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> >>> +}
> >>> +
> >>> +static int __sev_init_ex_locked(int *error)
> >>> +{
> >>> +     struct sev_data_init_ex data;
> >>> +     int ret;
> >>>
> >>>        memset(&data, 0, sizeof(data));
> >>> +     data.length = sizeof(data);
> >>> +     data.nv_address = __psp_pa(sev_init_ex_nv_address);
> >>> +     data.nv_len = NV_LENGTH;
> >>> +
> >>> +     ret = sev_read_nv_memory();
> >>> +     if (ret)
> >>> +             return ret;
> >>> +
> >>>        if (sev_es_tmr) {
> >>>                u64 tmr_pa;
> >>>
> >>> @@ -276,12 +395,30 @@ static int __sev_platform_init_locked(int *error)
> >>>                 */
> >>>                tmr_pa = __pa(sev_es_tmr);
> >>>
> >>> -             data.flags |= SEV_INIT_FLAGS_SEV_ES;
> >>
> >> Inadvertant deletion?
> >
> > Oops only testing with SEV guests. Will test with ES too. Fixed.
> >
> >>
> >>>                data.tmr_address = tmr_pa;
> >>>                data.tmr_len = SEV_ES_TMR_SIZE;
> >>>        }
> >>
> >> Add a blank line here.
> >>
> >>> +     return __sev_do_cmd_locked(SEV_CMD_INIT_EX, &data, error);
> >>> +}
> >>> +
> >>> +static int __sev_platform_init_locked(int *error)
> >>> +{
> >>> +     struct psp_device *psp = psp_master;
> >>> +     struct sev_device *sev;
> >>> +     int rc;
> >>> +     int (*init_function)(int *error) = sev_init_ex_nv_address ?
> >>> +                     __sev_init_ex_locked :
> >>> +                     __sev_init_locked;
> >>
> >> This seems a bit much in the declaration. How about moving the assignment
> >> down to just before the first call?
> >
> > Done.
> >
> >>
> >>> +
> >>> +     if (!psp || !psp->sev_data)
> >>> +             return -ENODEV;
> >>> +
> >>> +     sev = psp->sev_data;
> >>>
> >>> -     rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> >>> +     if (sev->state == SEV_STATE_INIT)
> >>> +             return 0;
> >>> +
> >>> +     rc = init_function(error);
> >>>        if (rc && *error == SEV_RET_SECURE_DATA_INVALID) {
> >>>                /*
> >>>                 * INIT command returned an integrity check failure
> >>> @@ -290,8 +427,8 @@ static int __sev_platform_init_locked(int *error)
> >>>                 * failed and persistent state has been erased.
> >>>                 * Retrying INIT command here should succeed.
> >>>                 */
> >>> -             dev_dbg(sev->dev, "SEV: retrying INIT command");
> >>> -             rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> >>> +             dev_err(sev->dev, "SEV: retrying INIT command");
> >>
> >> Maybe dev_notice() instead of dev_err()?
> >
> > Done.,
> >
> >>
> >>> +             rc = init_function(error);
> >>>        }
> >>>
> >>>        if (rc)
> >>> @@ -307,7 +444,7 @@ static int __sev_platform_init_locked(int *error)
> >>>
> >>>        dev_dbg(sev->dev, "SEV firmware initialized\n");
> >>>
> >>> -     return rc;
> >>> +     return 0;
> >>>    }
> >>>
> >>>    int sev_platform_init(int *error)
> >>> @@ -987,7 +1124,7 @@ static int sev_misc_init(struct sev_device *sev)
> >>>
> >>>        init_waitqueue_head(&sev->int_queue);
> >>>        sev->misc = misc_dev;
> >>> -     dev_dbg(dev, "registered SEV device\n");
> >>> +     dev_err(dev, "registered SEV device\n");
> >>
> >> Not sure this is a necessary change... but, if you don't want this as a
> >> dev_dbg() then it should be a dev_info(), because it is not an error.
> >
> > Oops this was my debugging. Reverted.
> >
> >>
> >>>
> >>>        return 0;
> >>>    }
> >>> @@ -1061,6 +1198,12 @@ static void sev_firmware_shutdown(struct sev_device *sev)
> >>>                           get_order(SEV_ES_TMR_SIZE));
> >>>                sev_es_tmr = NULL;
> >>>        }
> >>> +
> >>> +     if (sev_init_ex_nv_address) {
> >>> +             free_pages((unsigned long)sev_init_ex_nv_address,
> >>> +                        get_order(NV_LENGTH));
> >>> +             sev_init_ex_nv_address = NULL;
> >>> +     }
> >>>    }
> >>>
> >>>    void sev_dev_destroy(struct psp_device *psp)
> >>> @@ -1105,6 +1248,19 @@ void sev_pci_init(void)
> >>>            sev_update_firmware(sev->dev) == 0)
> >>>                sev_get_api_version();
> >>>
> >>> +     /* If an init_ex_path is provided rely on INIT_EX for PSP initialization
> >>> +      * instead of INIT.
> >>> +      */
> >>> +     if (init_ex_path) {
> >>> +             sev_init_ex_nv_address = sev_fw_alloc(NV_LENGTH);
> >>> +             if (!sev_init_ex_nv_address) {
> >>> +                     dev_warn(
> >>
> >> Shouldn't this be a dev_err(), since you are erroring out?
> >>
> >>> +                             sev->dev,
> >>
> >> Move this up to the previous line, i.e.: dev_err(sev->dev,
> >>
> >>> +                             "SEV: INIT_EX NV storage allocation failed, INIT-EX support unavailable\n");
> >>
> >> Since you're erroring out, probably enough to just have the first part of
> >> that message. But if not:
> >>
> >> s/INIT-EX/INIT_EX/
> >
> > Fixed this log line.
> >
> >>
> >> Thanks,
> >> Tom
> >>
> >>> +                     goto err;
> >>> +             }
> >>> +     }
> >>> +
> >>>        /* Obtain the TMR memory area for SEV-ES use */
> >>>        sev_es_tmr = sev_fw_alloc(SEV_ES_TMR_SIZE);
> >>>        if (!sev_es_tmr)
> >>> diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
> >>> index d48a7192e881..1595088c428b 100644
> >>> --- a/include/linux/psp-sev.h
> >>> +++ b/include/linux/psp-sev.h
> >>> @@ -52,6 +52,7 @@ enum sev_cmd {
> >>>        SEV_CMD_DF_FLUSH                = 0x00A,
> >>>        SEV_CMD_DOWNLOAD_FIRMWARE       = 0x00B,
> >>>        SEV_CMD_GET_ID                  = 0x00C,
> >>> +     SEV_CMD_INIT_EX                 = 0x00D,
> >>>
> >>>        /* Guest commands */
> >>>        SEV_CMD_DECOMMISSION            = 0x020,
> >>> @@ -102,6 +103,26 @@ struct sev_data_init {
> >>>        u32 tmr_len;                    /* In */
> >>>    } __packed;
> >>>
> >>> +/**
> >>> + * struct sev_data_init_ex - INIT_EX command parameters
> >>> + *
> >>> + * @length: len of the command buffer read by the PSP
> >>> + * @flags: processing flags
> >>> + * @tmr_address: system physical address used for SEV-ES
> >>> + * @tmr_len: len of tmr_address
> >>> + * @nv_address: system physical address used for PSP NV storage
> >>> + * @nv_len: len of nv_address
> >>> + */
> >>> +struct sev_data_init_ex {
> >>> +     u32 length;                     /* In */
> >>> +     u32 flags;                      /* In */
> >>> +     u64 tmr_address;                /* In */
> >>> +     u32 tmr_len;                    /* In */
> >>> +     u32 reserved;                   /* In */
> >>> +     u64 nv_address;                 /* In/Out */
> >>> +     u32 nv_len;                     /* In */
> >>> +} __packed;
> >>> +
> >>>    #define SEV_INIT_FLAGS_SEV_ES       0x01
> >>>
> >>>    /**
> >>>

Re: [PATCH 4/4] crypto: ccp - Add SEV_INIT_EX support

[Peter Gonda]

On Fri, Oct 29, 2021 at 1:26 PM Relph, Richard <Richard.Relph@amd.com> wrote:
>
> [AMD Official Use Only]
>
>
>
> > Does SHUTDOWN modify the SPI/NV area? Otherwise a separate comment about
> > why it is included below.
>
> Double checked the FW doc looks like it does not. Richard can you
> confirm? If it doesn't I'll remove this case.
>
> Peter,
>     SHUTDOWN does NOT modify the SPI or write to the NV area in memory.
>  Richard

Thanks! I'll leave it out of this conditional.

Re: [PATCH 4/4] crypto: ccp - Add SEV_INIT_EX support

[Marc Orr]

On Thu, Oct 28, 2021 at 10:58 AM Peter Gonda <pgonda@google.com> wrote:
>
> From: David Rientjes <rientjes@google.com>
>
> Add new module parameter to allow users to use SEV_INIT_EX instead of
> SEV_INIT. This helps users who lock their SPI bus to use the PSP for SEV
> functionality. The 'init_ex_path' parameter defaults to NULL which means
> the kernel will use SEV_INIT, if a path is specified SEV_INIT_EX will be
> used with the data found at the path. On certain PSP commands this
> file is written to as the PSP updates the NV memory region. Depending on
> file system initialization this file open may fail during module init
> but the CCP driver for SEV already has sufficient retries for platform
> initialization. During normal operation of PSP system and SEV commands
> if the PSP has not been initialized it is at run time.
>
> Signed-off-by: David Rientjes <rientjes@google.com>
> Co-developed-by: Peter Gonda <pgonda@google.com>
> Signed-off-by: Peter Gonda <pgonda@google.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Marc Orr <marcorr@google.com>
> Cc: Joerg Roedel <jroedel@suse.de>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: David Rientjes <rientjes@google.com>
> Cc: John Allen <john.allen@amd.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Paolo Bonzini <pbonzini@redhat.com> (
> Cc: linux-crypto@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/crypto/ccp/sev-dev.c | 186 ++++++++++++++++++++++++++++++++---
>  include/linux/psp-sev.h      |  21 ++++
>  2 files changed, 192 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index b568ae734857..c8718b4cbc93 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -22,6 +22,7 @@
>  #include <linux/firmware.h>
>  #include <linux/gfp.h>
>  #include <linux/cpufeature.h>
> +#include <linux/fs.h>
>
>  #include <asm/smp.h>
>
> @@ -43,6 +44,10 @@ static int psp_probe_timeout = 5;
>  module_param(psp_probe_timeout, int, 0644);
>  MODULE_PARM_DESC(psp_probe_timeout, " default timeout value, in seconds, during PSP device probe");
>
> +static char *init_ex_path;
> +module_param(init_ex_path, charp, 0660);
> +MODULE_PARM_DESC(init_ex_path, " Path for INIT_EX data; if set try INIT_EX");
> +
>  MODULE_FIRMWARE("amd/amd_sev_fam17h_model0xh.sbin"); /* 1st gen EPYC */
>  MODULE_FIRMWARE("amd/amd_sev_fam17h_model3xh.sbin"); /* 2nd gen EPYC */
>  MODULE_FIRMWARE("amd/amd_sev_fam19h_model0xh.sbin"); /* 3rd gen EPYC */
> @@ -58,6 +63,14 @@ static int psp_timeout;
>  #define SEV_ES_TMR_SIZE                (1024 * 1024)
>  static void *sev_es_tmr;
>
> +/* INIT_EX NV Storage:
> + *   The NV Storage is a 32Kb area and must be 4Kb page aligned.  Use the page
> + *   allocator to allocate the memory, which will return aligned memory for the
> + *   specified allocation order.
> + */
> +#define NV_LENGTH (32 << 10)
> +static void *sev_init_ex_nv_address;
> +
>  static inline bool sev_version_greater_or_equal(u8 maj, u8 min)
>  {
>         struct sev_device *sev = psp_master->sev_data;
> @@ -135,6 +148,7 @@ static int sev_cmd_buffer_len(int cmd)
>         case SEV_CMD_GET_ID:                    return sizeof(struct sev_data_get_id);
>         case SEV_CMD_ATTESTATION_REPORT:        return sizeof(struct sev_data_attestation_report);
>         case SEV_CMD_SEND_CANCEL:                       return sizeof(struct sev_data_send_cancel);
> +       case SEV_CMD_INIT_EX:                   return sizeof(struct sev_data_init_ex);
>         default:                                return 0;
>         }
>
> @@ -156,6 +170,89 @@ static void *sev_fw_alloc(unsigned long len)
>         return page_address(page);
>  }
>
> +static int sev_read_nv_memory(void)
> +{
> +       struct file *fp;
> +       ssize_t nread;
> +
> +       if (!sev_init_ex_nv_address)
> +               return -EOPNOTSUPP;
> +
> +       fp = filp_open(init_ex_path, O_RDONLY, 0);
> +       if (IS_ERR(fp)) {
> +               dev_err(psp_master->dev, "sev could not open file for read\n");
> +               return PTR_ERR(fp);
> +       }
> +
> +       nread = kernel_read(fp, sev_init_ex_nv_address, NV_LENGTH, 0);
> +       dev_dbg(psp_master->dev, "sev NV read %d bytes\n", nread);
> +       filp_close(fp, NULL);
> +       return 0;
> +}
> +
> +static int sev_write_nv_memory(void)
> +{
> +       struct sev_device *sev = psp_master->sev_data;
> +       struct file *fp;
> +       loff_t offset = 0;
> +       int ret;
> +
> +       if (!sev_init_ex_nv_address)
> +               return -EOPNOTSUPP;
> +
> +       fp = filp_open(init_ex_path, O_CREAT | O_WRONLY, 0600);
> +       if (IS_ERR(fp)) {
> +               dev_err(sev->dev, "sev NV data could not be created\n");
> +               return PTR_ERR(fp);
> +       }
> +       ret = kernel_write(fp, sev_init_ex_nv_address, NV_LENGTH, &offset);
> +       vfs_fsync(fp, 0);
> +       filp_close(fp, NULL);
> +
> +       if (ret != NV_LENGTH) {
> +               dev_err(sev->dev,
> +                       "failed to write %d bytes to non volatile memory area, ret=%lu\n",
> +                       NV_LENGTH, ret);
> +               if (ret >= 0)
> +                       return -EIO;
> +               return ret;
> +       }
> +
> +       dev_dbg(sev->dev, "wrote to non volatile memory area\n");
> +       return 0;
> +}
> +
> +static void sev_write_nv_memory_if_required(int cmd_id)
> +{
> +       struct sev_device *sev = psp_master->sev_data;
> +       int ret;
> +
> +       if (!sev_init_ex_nv_address)
> +               return;
> +
> +       /*
> +        * Only a few platform commands modify the SPI/NV area,
> +        * but none of the non-platform commands do. Only INIT,
> +        * PLATFORM_RESET, PEK_GEN, PEK_CERT_IMPORT, and
> +        * PDH_GEN do.
> +        */
> +       switch (cmd_id) {
> +       case SEV_CMD_FACTORY_RESET:
> +       case SEV_CMD_INIT_EX:
> +       case SEV_CMD_PDH_GEN:
> +       case SEV_CMD_PEK_CERT_IMPORT:
> +       case SEV_CMD_PEK_GEN:
> +       case SEV_CMD_SHUTDOWN:
> +               break;
> +       default:
> +               return;
> +       };
> +
> +       ret = sev_write_nv_memory();
> +       if (ret)
> +               dev_err(sev->dev, "sev NV write failed %d\n", ret);
> +}
> +
>  static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
>  {
>         struct psp_device *psp = psp_master;
> @@ -225,6 +322,8 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
>                 dev_dbg(sev->dev, "sev command %#x failed (%#010x)\n",
>                         cmd, reg & PSP_CMDRESP_ERR_MASK);
>                 ret = -EIO;
> +       } else {
> +               sev_write_nv_memory_if_required(cmd);
>         }
>
>         print_hex_dump_debug("(out): ", DUMP_PREFIX_OFFSET, 16, 2, data,
> @@ -251,22 +350,42 @@ static int sev_do_cmd(int cmd, void *data, int *psp_ret)
>         return rc;
>  }
>
> -static int __sev_platform_init_locked(int *error)
> +static int __sev_init_locked(int *error)
>  {
> -       struct psp_device *psp = psp_master;
>         struct sev_data_init data;
> -       struct sev_device *sev;
> -       int rc = 0;
>
> -       if (!psp || !psp->sev_data)
> -               return -ENODEV;
> +       memset(&data, 0, sizeof(data));
> +       if (sev_es_tmr) {
> +               u64 tmr_pa;
>
> -       sev = psp->sev_data;
> +               /*
> +                * Do not include the encryption mask on the physical
> +                * address of the TMR (firmware should clear it anyway).
> +                */
> +               tmr_pa = __pa(sev_es_tmr);
>
> -       if (sev->state == SEV_STATE_INIT)
> -               return 0;
> +               data.flags |= SEV_INIT_FLAGS_SEV_ES;
> +               data.tmr_address = tmr_pa;
> +               data.tmr_len = SEV_ES_TMR_SIZE;
> +       }
> +
> +       return __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> +}
> +
> +static int __sev_init_ex_locked(int *error)
> +{
> +       struct sev_data_init_ex data;
> +       int ret;
>
>         memset(&data, 0, sizeof(data));
> +       data.length = sizeof(data);
> +       data.nv_address = __psp_pa(sev_init_ex_nv_address);
> +       data.nv_len = NV_LENGTH;
> +
> +       ret = sev_read_nv_memory();
> +       if (ret)
> +               return ret;
> +
>         if (sev_es_tmr) {
>                 u64 tmr_pa;
>
> @@ -276,12 +395,30 @@ static int __sev_platform_init_locked(int *error)
>                  */
>                 tmr_pa = __pa(sev_es_tmr);
>
> -               data.flags |= SEV_INIT_FLAGS_SEV_ES;
>                 data.tmr_address = tmr_pa;
>                 data.tmr_len = SEV_ES_TMR_SIZE;
>         }
> +       return __sev_do_cmd_locked(SEV_CMD_INIT_EX, &data, error);
> +}
> +
> +static int __sev_platform_init_locked(int *error)
> +{
> +       struct psp_device *psp = psp_master;
> +       struct sev_device *sev;
> +       int rc;
> +       int (*init_function)(int *error) = sev_init_ex_nv_address ?
> +                       __sev_init_ex_locked :
> +                       __sev_init_locked;
> +
> +       if (!psp || !psp->sev_data)
> +               return -ENODEV;
> +
> +       sev = psp->sev_data;
>
> -       rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> +       if (sev->state == SEV_STATE_INIT)
> +               return 0;
> +
> +       rc = init_function(error);
>         if (rc && *error == SEV_RET_SECURE_DATA_INVALID) {
>                 /*
>                  * INIT command returned an integrity check failure
> @@ -290,8 +427,8 @@ static int __sev_platform_init_locked(int *error)
>                  * failed and persistent state has been erased.
>                  * Retrying INIT command here should succeed.
>                  */
> -               dev_dbg(sev->dev, "SEV: retrying INIT command");
> -               rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> +               dev_err(sev->dev, "SEV: retrying INIT command");
> +               rc = init_function(error);
>         }
>
>         if (rc)
> @@ -307,7 +444,7 @@ static int __sev_platform_init_locked(int *error)
>
>         dev_dbg(sev->dev, "SEV firmware initialized\n");
>
> -       return rc;
> +       return 0;
>  }
>
>  int sev_platform_init(int *error)
> @@ -987,7 +1124,7 @@ static int sev_misc_init(struct sev_device *sev)
>
>         init_waitqueue_head(&sev->int_queue);
>         sev->misc = misc_dev;
> -       dev_dbg(dev, "registered SEV device\n");
> +       dev_err(dev, "registered SEV device\n");
>
>         return 0;
>  }
> @@ -1061,6 +1198,12 @@ static void sev_firmware_shutdown(struct sev_device *sev)
>                            get_order(SEV_ES_TMR_SIZE));
>                 sev_es_tmr = NULL;
>         }
> +
> +       if (sev_init_ex_nv_address) {
> +               free_pages((unsigned long)sev_init_ex_nv_address,
> +                          get_order(NV_LENGTH));
> +               sev_init_ex_nv_address = NULL;
> +       }
>  }
>
>  void sev_dev_destroy(struct psp_device *psp)
> @@ -1105,6 +1248,19 @@ void sev_pci_init(void)
>             sev_update_firmware(sev->dev) == 0)
>                 sev_get_api_version();
>
> +       /* If an init_ex_path is provided rely on INIT_EX for PSP initialization
> +        * instead of INIT.
> +        */
> +       if (init_ex_path) {
> +               sev_init_ex_nv_address = sev_fw_alloc(NV_LENGTH);
> +               if (!sev_init_ex_nv_address) {
> +                       dev_warn(
> +                               sev->dev,
> +                               "SEV: INIT_EX NV storage allocation failed, INIT-EX support unavailable\n");
> +                       goto err;
> +               }
> +       }
> +
>         /* Obtain the TMR memory area for SEV-ES use */
>         sev_es_tmr = sev_fw_alloc(SEV_ES_TMR_SIZE);
>         if (!sev_es_tmr)
> diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
> index d48a7192e881..1595088c428b 100644
> --- a/include/linux/psp-sev.h
> +++ b/include/linux/psp-sev.h
> @@ -52,6 +52,7 @@ enum sev_cmd {
>         SEV_CMD_DF_FLUSH                = 0x00A,
>         SEV_CMD_DOWNLOAD_FIRMWARE       = 0x00B,
>         SEV_CMD_GET_ID                  = 0x00C,
> +       SEV_CMD_INIT_EX                 = 0x00D,
>
>         /* Guest commands */
>         SEV_CMD_DECOMMISSION            = 0x020,
> @@ -102,6 +103,26 @@ struct sev_data_init {
>         u32 tmr_len;                    /* In */
>  } __packed;
>
> +/**
> + * struct sev_data_init_ex - INIT_EX command parameters
> + *
> + * @length: len of the command buffer read by the PSP
> + * @flags: processing flags
> + * @tmr_address: system physical address used for SEV-ES
> + * @tmr_len: len of tmr_address
> + * @nv_address: system physical address used for PSP NV storage
> + * @nv_len: len of nv_address
> + */
> +struct sev_data_init_ex {
> +       u32 length;                     /* In */
> +       u32 flags;                      /* In */
> +       u64 tmr_address;                /* In */
> +       u32 tmr_len;                    /* In */
> +       u32 reserved;                   /* In */
> +       u64 nv_address;                 /* In/Out */
> +       u32 nv_len;                     /* In */
> +} __packed;
> +
>  #define SEV_INIT_FLAGS_SEV_ES  0x01
>
>  /**
> --
> 2.33.1.1089.g2158813163f-goog
>

Reviewed-by: Marc Orr <marcorr@google.com>

Re: [PATCH 0/4] Add SEV_INIT_EX support

[Marc Orr]

On Thu, Oct 28, 2021 at 10:57 AM Peter Gonda <pgonda@google.com> wrote:
>
> SEV_INIT requires users to unlock their SPI bus for the PSP's non
> volatile (NV) storage. Users may wish to lock their SPI bus for numerous
> reasons, to support this the PSP firmware supports SEV_INIT_EX. INIT_EX
> allows the firmware to use a region of memory for its NV storage leaving
> the kernel responsible for actually storing the data in a persistent
> way. This series adds a new module parameter to ccp allowing users to
> specify a path to a file for use as the PSP's NV storage. The ccp driver
> then reads the file into memory for the PSP to use and is responsible
> for writing the file whenever the PSP modifies the memory region.
>
> Signed-off-by: Peter Gonda <pgonda@google.com>
> Acked-by: David Rientjes <rientjes@google.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Marc Orr <marcorr@google.com>
> Cc: Joerg Roedel <jroedel@suse.de>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: David Rientjes <rientjes@google.com>
> Cc: John Allen <john.allen@amd.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Paolo Bonzini <pbonzini@redhat.com> (
> Cc: linux-crypto@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
>
> David Rientjes (1):
>   crypto: ccp - Add SEV_INIT_EX support
>
> Peter Gonda (3):
>   crypto: ccp - Fix SEV_INIT error logging on init
>   crypto: ccp - Move SEV_INIT retry for corrupted data
>   crypto: ccp - Refactor out sev_fw_alloc()
>
>  drivers/crypto/ccp/sev-dev.c | 235 ++++++++++++++++++++++++++++++-----
>  include/linux/psp-sev.h      |  21 ++++
>  2 files changed, 222 insertions(+), 34 deletions(-)
>
> --
> 2.33.1.1089.g2158813163f-goog
>

I've just replied with my Reviewed-by tag to all of the patches
because I had reviewed the v1 internally, before Peter posted
externally. Thank you, Tom, for the excellent reviews! I'm looking
forward to seeing the v2 with all of this feedback incorporated.