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

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
allocating a 1mb region for !SNP configuration.

While at it, provide API that can be used by others to allocate a page
that can be used by the firmware. The immediate user for this API will
be the KVM driver. The KVM driver to need to allocate a firmware context
page during the guest creation. The context page need to be updated
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(-)

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.

...

On Fri, Dec 08, 2023 at 02:05:20PM +0100, Borislav Petkov wrote:
> 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(-)
> > 
> > +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.

Indeed, and for snp_reclaim_pages() too by the looks of it. Will fix that up,
along with all the other suggestions.

> > +
> > +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.

In that case, the lock isn't actually taken. locked==true is basically
used to tell the code to not to try to acquire the lock, but the caller
is relying on the fact that all the other CPUs are stopped at that point
so there's no need to protect against multiple concurrent firmware
commands being issued.

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

There's another case that will be introduced in the next version of this
series (likely right after this patch) to handle a bug where the buffer used
to access INIT_EX non-volatile data needs to be transitioned to
firmware-owned beforehand. In that case, the CCP cleanup path introduces
another caller of __snp_free_firmware_pages() where locked==true. Maybe this
can be revisited in that context.

Thanks,

Mike


> 
> ...
> 
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette