Re: [PATCH v10 17/50] crypto: ccp: Handle the legacy TMR allocation when SNP is enabled

[Borislav Petkov <bp@alien8.de>]

On Mon, Oct 16, 2023 at 08:27:46AM -0500, Michael Roth wrote:
> From: Brijesh Singh <brijesh.singh@amd.com>
> 
> The behavior and requirement for the SEV-legacy command is altered when
> the SNP firmware is in the INIT state. See SEV-SNP firmware specification
> for more details.
> 
> Allocate the Trusted Memory Region (TMR) as a 2mb sized/aligned region
> when SNP is enabled to satisfy new requirements for the SNP. Continue

s/the //

> allocating a 1mb region for !SNP configuration.
> 
> While at it, provide API that can be used by others to allocate a page

"...an API... ... to allocate a firmware page."

Simple.

> that can be used by the firmware.

> The immediate user for this API will be the KVM driver.

Delete that sentence.

> The KVM driver to need to allocate a firmware context

"The KVM driver needs to allocate ...

> page during the guest creation. The context page need to be updated

"needs"

> by the firmware. See the SEV-SNP specification for further details.
> 
> Co-developed-by: Ashish Kalra <ashish.kalra@amd.com>
> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> [mdr: use struct sev_data_snp_page_reclaim instead of passing paddr
>       directly to SEV_CMD_SNP_PAGE_RECLAIM]
> Signed-off-by: Michael Roth <michael.roth@amd.com>
> ---
>  drivers/crypto/ccp/sev-dev.c | 151 ++++++++++++++++++++++++++++++++---
>  include/linux/psp-sev.h      |   9 +++
>  2 files changed, 151 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index 613b25f81498..ea21307a2b34 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -30,6 +30,7 @@
>  #include <asm/smp.h>
>  #include <asm/cacheflush.h>
>  #include <asm/e820/types.h>
> +#include <asm/sev-host.h>
>  
>  #include "psp-dev.h"
>  #include "sev-dev.h"
> @@ -93,6 +94,13 @@ static void *sev_init_ex_buffer;
>  struct sev_data_range_list *snp_range_list;
>  static int __sev_snp_init_locked(int *error);
>  
> +/* When SEV-SNP is enabled the TMR needs to be 2MB aligned and 2MB size. */
> +#define SEV_SNP_ES_TMR_SIZE	(2 * 1024 * 1024)

There's "SEV", "SNP" *and* "ES". Wow.

Let's do this:

#define SEV_TMR_SIZE	SZ_1M
#define SNP_TMR_SIZE	SZ_2M

Done.

> +static size_t sev_es_tmr_size = SEV_ES_TMR_SIZE;
> +
> +static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret);

Instead of doing forward declarations, move the whole logic around
__sev_do_cmd_locked() up here in the file so that you can call that
function by other functions without forward declarations.

The move should probably be a pre-patch.

>  static inline bool sev_version_greater_or_equal(u8 maj, u8 min)
>  {
>  	struct sev_device *sev = psp_master->sev_data;
> @@ -193,11 +201,131 @@ static int sev_cmd_buffer_len(int cmd)
>  	return 0;
>  }
>  
> +static int snp_reclaim_pages(unsigned long paddr, unsigned int npages, bool locked)
> +{
> +	/* Cbit maybe set in the paddr */
> +	unsigned long pfn = __sme_clr(paddr) >> PAGE_SHIFT;
> +	int ret, err, i, n = 0;
> +
> +	for (i = 0; i < npages; i++, pfn++, n++) {
> +		struct sev_data_snp_page_reclaim data = {0};
> +
> +		data.paddr = pfn << PAGE_SHIFT;

This shifting back'n'forth between paddr and pfn makes this function
hard to read. Let's use only paddr (diff ontop):

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index ea21307a2b34..25078b0253bd 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -203,14 +203,15 @@ static int sev_cmd_buffer_len(int cmd)
 
 static int snp_reclaim_pages(unsigned long paddr, unsigned int npages, bool locked)
 {
-	/* Cbit maybe set in the paddr */
-	unsigned long pfn = __sme_clr(paddr) >> PAGE_SHIFT;
 	int ret, err, i, n = 0;
 
-	for (i = 0; i < npages; i++, pfn++, n++) {
+	/* C-bit maybe set, clear it: */
+	paddr = __sme_clr(paddr);
+
+	for (i = 0; i < npages; i++, paddr += PAGE_SIZE, n++) {
 		struct sev_data_snp_page_reclaim data = {0};
 
-		data.paddr = pfn << PAGE_SHIFT;
+		data.paddr = paddr;
 
 		if (locked)
 			ret = __sev_do_cmd_locked(SEV_CMD_SNP_PAGE_RECLAIM, &data, &err);
@@ -220,7 +221,7 @@ static int snp_reclaim_pages(unsigned long paddr, unsigned int npages, bool lock
 		if (ret)
 			goto cleanup;
 
-		ret = rmp_make_shared(pfn, PG_LEVEL_4K);
+		ret = rmp_make_shared(__phys_to_pfn(paddr), PG_LEVEL_4K);
 		if (ret)
 			goto cleanup;
 	}
@@ -232,7 +233,7 @@ static int snp_reclaim_pages(unsigned long paddr, unsigned int npages, bool lock
 	 * If failed to reclaim the page then page is no longer safe to
 	 * be release back to the system, leak it.
 	 */
-	snp_leak_pages(pfn, npages - n);
+	snp_leak_pages(__phys_to_pfn(paddr), npages - n);
 	return ret;
 }
 
> +
> +		if (locked)
> +			ret = __sev_do_cmd_locked(SEV_CMD_SNP_PAGE_RECLAIM, &data, &err);
> +		else
> +			ret = sev_do_cmd(SEV_CMD_SNP_PAGE_RECLAIM, &data, &err);
> +
> +		if (ret)
> +			goto cleanup;
> +
> +		ret = rmp_make_shared(pfn, PG_LEVEL_4K);
> +		if (ret)
> +			goto cleanup;
> +	}
> +
> +	return 0;
> +
> +cleanup:
> +	/*
> +	 * If failed to reclaim the page then page is no longer safe to
> +	 * be release back to the system, leak it.

"released"

> +	 */
> +	snp_leak_pages(pfn, npages - n);
> +	return ret;
> +}
> +
> +static int rmp_mark_pages_firmware(unsigned long paddr, unsigned int npages, bool locked)
> +{
> +	/* Cbit maybe set in the paddr */
> +	unsigned long pfn = __sme_clr(paddr) >> PAGE_SHIFT;
> +	int rc, n = 0, i;

That n looks like it can be replaced by i.

> +
> +	for (i = 0; i < npages; i++, n++, pfn++) {
> +		rc = rmp_make_private(pfn, 0, PG_LEVEL_4K, 0, true);
> +		if (rc)
> +			goto cleanup;
> +	}
> +
> +	return 0;
> +
> +cleanup:
> +	/*
> +	 * Try unrolling the firmware state changes by
> +	 * reclaiming the pages which were already changed to the
> +	 * firmware state.
> +	 */
> +	snp_reclaim_pages(paddr, n, locked);
> +
> +	return rc;
> +}
> +
> +static struct page *__snp_alloc_firmware_pages(gfp_t gfp_mask, int order, bool locked)

AFAICT, @locked is always false. So it can go.

> +{
> +	unsigned long npages = 1ul << order, paddr;
> +	struct sev_device *sev;
> +	struct page *page;
> +
> +	if (!psp_master || !psp_master->sev_data)
> +		return NULL;
> +
> +	page = alloc_pages(gfp_mask, order);
> +	if (!page)
> +		return NULL;
> +
> +	/* If SEV-SNP is initialized then add the page in RMP table. */
> +	sev = psp_master->sev_data;
> +	if (!sev->snp_initialized)
> +		return page;
> +
> +	paddr = __pa((unsigned long)page_address(page));
> +	if (rmp_mark_pages_firmware(paddr, npages, locked))
> +		return NULL;
> +
> +	return page;
> +}
> +
> +void *snp_alloc_firmware_page(gfp_t gfp_mask)
> +{
> +	struct page *page;
> +
> +	page = __snp_alloc_firmware_pages(gfp_mask, 0, false);
> +
> +	return page ? page_address(page) : NULL;
> +}
> +EXPORT_SYMBOL_GPL(snp_alloc_firmware_page);
> +
> +static void __snp_free_firmware_pages(struct page *page, int order, bool locked)a

This @locked too is always false. It becomes true later in

Subject: [PATCH v10 50/50] crypto: ccp: Add panic notifier for SEV/SNP firmware shutdown on kdump

which talks about some panic notifier running in atomic context. But
then you can't take locks in atomic context.

Looks like this whole dance around the locked thing needs a cleanup.

...


-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette