Message-ID: <7a8a20be-3942-5d5c-719c-2e02d0a85b6c@amd.com>
Date: Tue, 5 Nov 2024 15:06:32 -0600
From: Tom Lendacky <thomas.lendacky@....com>
To: Dionna Glaze <dionnaglaze@...gle.com>, x86@...nel.org,
 linux-kernel@...r.kernel.org, John Allen <john.allen@....com>,
 Herbert Xu <herbert@...dor.apana.org.au>,
 "David S. Miller" <davem@...emloft.net>, Ashish Kalra <ashish.kalra@....com>
Cc: linux-crypto@...r.kernel.org
Subject: Re: [PATCH v4 3/6] crypto: ccp: Track GCTX through sev commands

On 11/4/24 19:05, Dionna Glaze wrote:
> In preparation for SEV firmware hotloading support, add bookkeeping
> structures for GCTX pages that are in use.
> 
> Compliance with SEV-SNP API section 3.3 Firmware Updates and 4.1.1
> Live Update: before a firmware is committed, all active GCTX pages
> should be updated with SNP_GUEST_STATUS to ensure their data structure
> remains consistent for the new firmware version.
> There can only be cpuid_edx(0x8000001f)-1 many SEV-SNP asids in use at
> one time, so this map associates asid to gctx in order to track which
> addresses are active gctx pages that need updating. When an asid and
> gctx page are decommissioned, the page is removed from tracking for
> update-purposes.
> Given that GCTX page creation and binding through the SNP_ACTIVATE
> command are separate, the creation operation also tracks pages that are
> yet to be bound to an asid.

I believe we have an ASID "allocated" by the time we call
snp_context_create(). And snp_decommission_context() is always called to
remove the context. Maybe a ccp driver API to create and destroy a
context would be good here. The ASID would be an additional parameter
and allow for associating the guest context page to the ASID right from
the start.

This way you don't need an snp_unbound_gctx_pages array.

I think it will make the code a lot simpler, but it does add an API
dependency between the CCP and KVM that needs to be worked out between
the maintainers.

> 
> The hotloading support depends on FW_UPLOAD, so the new functions are
> added in a new file whose object file is conditionally included in the
> module build.
> 
> Signed-off-by: Dionna Glaze <dionnaglaze@...gle.com>
> ---
>  drivers/crypto/ccp/Makefile  |   1 +
>  drivers/crypto/ccp/sev-dev.c |   5 ++
>  drivers/crypto/ccp/sev-dev.h |  15 +++++
>  drivers/crypto/ccp/sev-fw.c  | 117 +++++++++++++++++++++++++++++++++++
>  4 files changed, 138 insertions(+)
>  create mode 100644 drivers/crypto/ccp/sev-fw.c
> 
> diff --git a/drivers/crypto/ccp/Makefile b/drivers/crypto/ccp/Makefile
> index 394484929dae3..b8b5102cc7973 100644
> --- a/drivers/crypto/ccp/Makefile
> +++ b/drivers/crypto/ccp/Makefile
> @@ -14,6 +14,7 @@ ccp-$(CONFIG_CRYPTO_DEV_SP_PSP) += psp-dev.o \
>                                     platform-access.o \
>                                     dbc.o \
>                                     hsti.o
> +ccp-$(CONFIG_FW_UPLOAD) += sev-fw.o

As you saw from the krobot mail, you will probably need to create
something like CRYPTO_DEV_SP_PSP_FW_UPLOAD, which depends on
CRYPTO_DEV_SP_PSP and FW_UPLOAD.

>  
>  obj-$(CONFIG_CRYPTO_DEV_CCP_CRYPTO) += ccp-crypto.o
>  ccp-crypto-objs := ccp-crypto-main.o \
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index 9810edbb272d2..9265b6d534bbe 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -982,6 +982,9 @@ static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
>  	print_hex_dump_debug("(out): ", DUMP_PREFIX_OFFSET, 16, 2, data,
>  			     buf_len, false);
>  
> +	if (!ret)
> +		snp_cmd_bookkeeping_locked(cmd, sev, data);

I prefer to see this flow as:

	if (ret)
		return ret;

	snp_cmd_bookkeeping_locked(cmd, sev, data);

	return 0;

And if you end up creating APIs to create and destroy context pages,
then this call can be removed and each API call directly into the
appropriate tracking function.

> +
>  	return ret;
>  }
>  
> @@ -1179,6 +1182,8 @@ static int __sev_snp_init_locked(int *error)
>  
>  	sev_es_tmr_size = SNP_TMR_SIZE;
>  
> +	rc = sev_snp_platform_init_firmware_upload(sev);

Since this is mainly doing memory allocation, this should be moved to
just after the minimum firmware version check so that a failure would be
before SNP_INIT.

> +
>  	return rc;
>  }
>  
> diff --git a/drivers/crypto/ccp/sev-dev.h b/drivers/crypto/ccp/sev-dev.h
> index 3e4e5574e88a3..28add34484ed1 100644
> --- a/drivers/crypto/ccp/sev-dev.h
> +++ b/drivers/crypto/ccp/sev-dev.h
> @@ -57,6 +57,13 @@ struct sev_device {
>  	bool cmd_buf_backup_active;
>  
>  	bool snp_initialized;
> +
> +#ifdef CONFIG_FW_UPLOAD

This would be changed to whatever your new CONFIG option is.

> +	u32 last_snp_asid;

unsigned int max_snp_asid;

> +	u64 *snp_asid_to_gctx_pages_map;

s/_pages//

> +	u64 *snp_unbound_gctx_pages;

s/_pages//

> +	u32 snp_unbound_gctx_end;

unsigned int snp_unbound_gctx_end;

> +#endif /* CONFIG_FW_UPLOAD */
>  };
>  
>  int sev_dev_init(struct psp_device *psp);
> @@ -65,4 +72,12 @@ void sev_dev_destroy(struct psp_device *psp);
>  void sev_pci_init(void);
>  void sev_pci_exit(void);
>  
> +#ifdef CONFIG_FW_UPLOAD
> +void snp_cmd_bookkeeping_locked(int cmd, struct sev_device *sev, void *data);
> +int sev_snp_platform_init_firmware_upload(struct sev_device *sev);
> +#else
> +static inline void snp_cmd_bookkeeping_locked(int cmd, struct sev_device *sev, void *data) { }
> +static inline int sev_snp_platform_init_firmware_upload(struct sev_device *sev) { return 0; }
> +#endif /* CONFIG_FW_UPLOAD */
> +
>  #endif /* __SEV_DEV_H */
> diff --git a/drivers/crypto/ccp/sev-fw.c b/drivers/crypto/ccp/sev-fw.c
> new file mode 100644
> index 0000000000000..55a5a572da8f1
> --- /dev/null
> +++ b/drivers/crypto/ccp/sev-fw.c
> @@ -0,0 +1,117 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * AMD Secure Encrypted Virtualization (SEV) firmware upload API
> + */
> +
> +#include <linux/firmware.h>
> +#include <linux/psp-sev.h>
> +
> +#include <asm/sev.h>
> +
> +#include "sev-dev.h"
> +
> +/*
> + * After a gctx is created, it is used by snp_launch_start before getting

s/gctx/guest context page/

> + * bound to an asid. The launch protocol allocates an asid before creating a

s/asid/ASID/

> + * matching gctx page, so there should never be more unbound gctx pages than
> + * there are possible SNP asids.
> + *
> + * The unbound gctx pages must be updated after executing DOWNLOAD_FIRMWARE_EX
> + * and before committing the firmware.

Not needed here.

> + */
> +static void snp_gctx_create_track_locked(struct sev_device *sev, void *data)
> +{
> +	struct sev_data_snp_addr *gctx_create = data;
> +
> +	/* This condition should never happen, but is needed for memory safety. */
> +	if (sev->snp_unbound_gctx_end >= sev->last_snp_asid) {
> +		dev_warn(sev->dev, "Too many unbound SNP GCTX pages to track\n");
> +		return;

Should this return an error and fail the command?

> +	}
> +
> +	sev->snp_unbound_gctx_pages[sev->snp_unbound_gctx_end] = gctx_create->address;
> +	sev->snp_unbound_gctx_end++;
> +}
> +
> +/*
> + * PREREQUISITE: The snp_activate command was successful, meaning the asid

s/snp_activate/SNP_ACTIVATE/
s/asid/ASID/

> + * is in the acceptable range 1..sev->last_snp_asid.
> + *
> + * The gctx_paddr must be in the unbound gctx buffer.

Do you mean unbound gctx array?

> + */
> +static void snp_activate_track_locked(struct sev_device *sev, void *data)
> +{
> +	struct sev_data_snp_activate *data_activate = data;
> +
> +	sev->snp_asid_to_gctx_pages_map[data_activate->asid] = data_activate->gctx_paddr;
> +
> +	for (int i = 0; i < sev->snp_unbound_gctx_end; i++) {
> +		if (sev->snp_unbound_gctx_pages[i] == data_activate->gctx_paddr) {
> +			/*
> +			 * Swap the last unbound gctx page with the now-bound
> +			 * gctx page to shrink the buffer.
> +			 */
> +			sev->snp_unbound_gctx_end--;
> +			sev->snp_unbound_gctx_pages[i] =
> +				sev->snp_unbound_gctx_pages[sev->snp_unbound_gctx_end];
> +			sev->snp_unbound_gctx_pages[sev->snp_unbound_gctx_end] = 0;
> +			break;
> +		}
> +	}

What if it isn't found for some reason, should an error be returned and
fail the SNP_ACTIVATE command?

> +}
> +
> +static void snp_decommission_track_locked(struct sev_device *sev, void *data)
> +{
> +	struct sev_data_snp_addr *data_decommission = data;
> +
> +	for (int i = 1; i <= sev->last_snp_asid; i++) {
> +		if (sev->snp_asid_to_gctx_pages_map[i] == data_decommission->address) {
> +			sev->snp_asid_to_gctx_pages_map[i] = 0;
> +			break;
> +		}
> +	}
> +}
> +
> +void snp_cmd_bookkeeping_locked(int cmd, struct sev_device *sev, void *data)
> +{
> +	if (!sev->snp_asid_to_gctx_pages_map)
> +		return;
> +
> +	switch (cmd) {
> +	case SEV_CMD_SNP_GCTX_CREATE:
> +		snp_gctx_create_track_locked(sev, data);
> +		break;
> +	case SEV_CMD_SNP_ACTIVATE:
> +		snp_activate_track_locked(sev, data);
> +		break;
> +	case SEV_CMD_SNP_DECOMMISSION:
> +		snp_decommission_track_locked(sev, data);
> +		break;
> +	default:
> +		break;
> +	}
> +}
> +
> +int sev_snp_platform_init_firmware_upload(struct sev_device *sev)

snp_init_guest_context_tracking

> +{
> +	u32 max_snp_asid;

s/max_snp_asid/min_sev_asid/

> +
> +	/*
> +	 * cpuid_edx(0x8000001f) is the minimum SEV ASID, hence the exclusive

CPUID 0x8000001f_EDX contains the ...

> +	 * maximum SEV-SNP ASID. Save the inclusive maximum to avoid confusing
> +	 * logic elsewhere.
> +	 */
> +	max_snp_asid = cpuid_edx(0x8000001f);

if (max_snp_asid <= 1)
	return 0;

Then the indentation below can be removed and the alignment fixed.

> +	sev->last_snp_asid = max_snp_asid - 1;
> +	if (sev->last_snp_asid) {
> +		sev->snp_asid_to_gctx_pages_map = devm_kmalloc_array(
> +			sev->dev, max_snp_asid * 2, sizeof(u64), GFP_KERNEL | __GFP_ZERO);
> +		sev->snp_unbound_gctx_pages = &sev->snp_asid_to_gctx_pages_map[max_snp_asid];
> +		if (!sev->snp_asid_to_gctx_pages_map) {

I'd prefer to see the success check immediately after the allocation call.

Thanks,
Tom


> +			dev_err(sev->dev,
> +				"SEV-SNP: snp_asid_to_gctx_pages_map memory allocation failed\n");
> +			return -ENOMEM;
> +		}
> +	}
> +	return 0;
> +}

Powered by blists - more mailing lists