[V3,4/4] crypto: ccp - Add SEV_INIT_EX support

Commit Message

Peter Gonda Nov. 2, 2021, 2:23 p.m. UTC

From: David Rientjes <[e-mail]>

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. If the file at
'init_ex_path' does not exist the PSP will not be initialized. The user
must create the file prior to use with 32Kb of 0xFFs per spec.

Signed-off-by: David Rientjes <[e-mail]>
Co-developed-by: Peter Gonda <[e-mail]>
Signed-off-by: Peter Gonda <[e-mail]>
Reviewed-by: Marc Orr <[e-mail]>
Cc: Tom Lendacky <[e-mail]>
Cc: Brijesh Singh <[e-mail]>
Cc: Marc Orr <[e-mail]>
Cc: Joerg Roedel <[e-mail]>
Cc: Herbert Xu <[e-mail]>
Cc: David Rientjes <[e-mail]>
Cc: John Allen <[e-mail]>
Cc: "David S. Miller" <[e-mail]>
Cc: Paolo Bonzini <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
---
 .../virt/kvm/amd-memory-encryption.rst        |   6 +
 drivers/crypto/ccp/sev-dev.c                  | 181 ++++++++++++++++--
 include/linux/psp-sev.h                       |  21 ++
 3 files changed, 195 insertions(+), 13 deletions(-)

Comments

Tom Lendacky Nov. 2, 2021, 3:38 p.m. UTC | #1

On 11/2/21 9:23 AM, Peter Gonda wrote:
> From: David Rientjes <[e-mail]>
> 
> +
> +	nread = kernel_read(fp, sev_init_ex_nv_address, NV_LENGTH, NULL);

Not sure if you missed the previous comment, but kernel_read can return an 
error, shouldn't it be checked and fail on error?

Thanks,
Tom

> +	dev_dbg(psp_master->dev, "SEV: read %ld bytes from NV file\n", nread);
> +	filp_close(fp, NULL);
> +
> +	return 0;
> +}
> +

Peter Gonda Nov. 2, 2021, 4:28 p.m. UTC | #2

On Tue, Nov 2, 2021 at 9:38 AM Tom Lendacky <[e-mail]> wrote:
>
> On 11/2/21 9:23 AM, Peter Gonda wrote:
> > From: David Rientjes <[e-mail]>
> >
> > +
> > +     nread = kernel_read(fp, sev_init_ex_nv_address, NV_LENGTH, NULL);
>
> Not sure if you missed the previous comment, but kernel_read can return an
> error, shouldn't it be checked and fail on error?

I did miss that comment. Updated to make sure nread == NV_LENGTH.
>
> Thanks,
> Tom
>
> > +     dev_dbg(psp_master->dev, "SEV: read %ld bytes from NV file\n", nread);
> > +     filp_close(fp, NULL);
> > +
> > +     return 0;
> > +}
> > +

Sean Christopherson Nov. 9, 2021, 5:21 p.m. UTC | #3

On Tue, Nov 02, 2021, Peter Gonda wrote:
> @@ -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);

Why is this writable after the module loads?  At best, it seems like it will give
userspace an easy way to shoot itself in the foot, at worst it will lead to a
TOCTOU bug in the kernel.

> +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 * 1024)
> +static void *sev_init_ex_nv_address;

The "address" part is redundant and potentially confusing, e.g. one might expect
it to contain a physical address.

And the "NV" part is kind of a lie, this isn't in non-volatile memory (it's a
kernel buffer) and it's obviously volatile in the sense that it can be changed
by the PSP.  I get that from the PSP's perspective it's _intended_ to be NV
storage, but (a) this does not point at NV storage and (b) there is no guarantee
that the path provided by userspace points at NV storage.

Maybe "sev_init_ex_buffer" or something along those lines?

>  static inline bool sev_version_greater_or_equal(u8 maj, u8 min)
>  {
>  	struct sev_device *sev = psp_master->sev_data;
> @@ -107,6 +120,7 @@ static int sev_cmd_buffer_len(int cmd)
>  {
>  	switch (cmd) {
>  	case SEV_CMD_INIT:			return sizeof(struct sev_data_init);
> +	case SEV_CMD_INIT_EX:                   return sizeof(struct sev_data_init_ex);
>  	case SEV_CMD_PLATFORM_STATUS:		return sizeof(struct sev_user_data_status);
>  	case SEV_CMD_PEK_CSR:			return sizeof(struct sev_data_pek_csr);
>  	case SEV_CMD_PEK_CERT_IMPORT:		return sizeof(struct sev_data_pek_cert_import);
> @@ -152,6 +166,87 @@ static void *sev_fw_alloc(unsigned long len)
>  	return page_address(page);
>  }
>  
> +static int sev_read_nv_memory(void)

Similar to above, this is reading from a file that's backed by who knows what,
i.e. it may or may not be reading NV memory.  And if it's going to implicitly
read into the buffer, that should be reflected in the name.

> +{
> +	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)) {
> +		int ret = PTR_ERR(fp);
> +
> +		dev_err(psp_master->dev,
> +			"SEV: could not open %s for read, error %d\n",
> +			init_ex_path, ret);
> +		return ret;
> +	}
> +
> +	nread = kernel_read(fp, sev_init_ex_nv_address, NV_LENGTH, NULL);
> +	dev_dbg(psp_master->dev, "SEV: read %ld bytes from NV file\n", nread);
> +	filp_close(fp, NULL);
> +
> +	return 0;
> +}
> +
> +static void sev_write_nv_memory(void)

Same comments here.

> +{
> +	struct sev_device *sev = psp_master->sev_data;
> +	struct file *fp;
> +	loff_t offset = 0;
> +	ssize_t nwrite;
> +
> +	if (!sev_init_ex_nv_address)
> +		return;
> +
> +	fp = filp_open(init_ex_path, O_CREAT | O_WRONLY, 0600);
> +	if (IS_ERR(fp)) {
> +		dev_err(sev->dev,
> +			"SEV: could not open file for write, error %d\n",
> +			PTR_ERR(fp));
> +		return;
> +	}
> +
> +	nwrite = kernel_write(fp, sev_init_ex_nv_address, NV_LENGTH, &offset);
> +	vfs_fsync(fp, 0);
> +	filp_close(fp, NULL);
> +
> +	if (nwrite != NV_LENGTH) {
> +		dev_err(sev->dev,
> +			"SEV: failed to write %u bytes to non volatile memory area, ret %ld\n",
> +			NV_LENGTH, nwrite);
> +		return;
> +	}
> +
> +	dev_dbg(sev->dev, "SEV: write successful to NV file\n");
> +}
> +
> +static void sev_write_nv_memory_if_required(int cmd_id)

And here.

> +{
> +	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(_EX), 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:
> +		break;
> +	default:
> +		return;
> +	};
> +
> +	sev_write_nv_memory();
> +}
> +
>  static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
>  {
>  	struct psp_device *psp = psp_master;
> @@ -221,6 +316,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,
> @@ -247,22 +344,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);

I realize this is copy-pasted from existing code, by why bother with an intermediate
tmr_pa?  Just set data.tmr_address directly.

>  
> -	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();

Why defer reading the file until INIT_EX?  Why not read the data when the path
is first verified and consumed?  More comments below in the retry path.

And what happens when sev_platform_init() is reached through a path other than
sev_pci_init()?  I guess that's a question for the existing code since sev_es_tmr
will be NULL unless sev_platform_init() routes through sev_pci_init().  So either
the sev_platform_init() call from KVM's sev_guest_init() is pointless, or there's
potential for some truly bizarre and confusing behavior.

> +	if (ret)
> +		return ret;
> +
>  	if (sev_es_tmr) {
>  		u64 tmr_pa;
>  
> @@ -277,7 +394,27 @@ static int __sev_platform_init_locked(int *error)
>  		data.tmr_len = SEV_ES_TMR_SIZE;
>  	}
>  
> -	rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> +	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);
> +
> +	if (!psp || !psp->sev_data)
> +		return -ENODEV;
> +
> +	sev = psp->sev_data;
> +
> +	if (sev->state == SEV_STATE_INIT)
> +		return 0;
> +
> +	init_function = sev_init_ex_nv_address ? __sev_init_ex_locked :
> +	    __sev_init_locked;

There's no need for this to be a function pointer, and the duplicate code can be
consolidated.

static int sev_do_init_locked(int cmd, void *data, int *error)
{
	if (sev_es_tmr) {
		/*
		 * Do not include the encryption mask on the physical
		 * address of the TMR (firmware should clear it anyway).
		 */
		data.flags |= SEV_INIT_FLAGS_SEV_ES;
		data.tmr_address = __pa(sev_es_tmr);
		data.tmr_len = SEV_ES_TMR_SIZE;
	}
	return __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
}

static int __sev_init_locked(int *error)
{
	struct sev_data_init data;

	memset(&data, 0, sizeof(data));
	return sev_do_init_locked(cmd, &data, error);
}

static int __sev_init_ex_locked(int *error)
{
	struct sev_data_init_ex data;

	memset(&data, 0, sizeof(data));
	data.length = sizeof(data);
	data.nv_address = __psp_pa(sev_init_ex_nv_address);
	data.nv_len = NV_LENGTH;
	return sev_do_init_locked(SEV_CMD_INIT_EX, &data, error);
}

> +	rc = init_function(error);
>  	if (rc && *error == SEV_RET_SECURE_DATA_INVALID) {
>  		/*
>  		 * INIT command returned an integrity check failure
> @@ -286,8 +423,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_notice(sev->dev, "SEV: retrying INIT command");
> +		rc = init_function(error);

The above comment says "persistent state has been erased", but __sev_do_cmd_locked()
only writes back to the file if a relevant command was successful, which means
that rereading the userspace file in __sev_init_ex_locked() will retry INIT_EX
with the same garbage data.

IMO, the behavior should be to read the file on load and then use the kernel buffer
without ever reloading (unless this is built as a module and is unloaded and reloaded).
The writeback then becomes opportunistic in the sense that if it fails for some reason,
the kernel's internal state isn't blasted away.

>  	}
>  
>  	if (rc)
> @@ -303,7 +440,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)
> @@ -1057,6 +1194,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) {

It really seems like the the teardown path should be paranoid and do a final
writeback of the kernel's buffer.

> +		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)
> @@ -1101,6 +1244,18 @@ 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_err(sev->dev,
> +				"SEV: INIT_EX NV memory allocation failed\n");
> +			goto err;

Why does this nullify psp_master->sev_data but later failures do not?

> +		}
> +	}
> +
>  	/* 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
>

Peter Gonda Nov. 9, 2021, 8:09 p.m. UTC | #4

.

On Tue, Nov 9, 2021 at 10:21 AM Sean Christopherson <[e-mail]> wrote:
>
> On Tue, Nov 02, 2021, Peter Gonda wrote:
> > @@ -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);
>
> Why is this writable after the module loads?  At best, it seems like it will give
> userspace an easy way to shoot itself in the foot, at worst it will lead to a
> TOCTOU bug in the kernel.

Will update.

>
> > +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 * 1024)
> > +static void *sev_init_ex_nv_address;
>
> The "address" part is redundant and potentially confusing, e.g. one might expect
> it to contain a physical address.
>
> And the "NV" part is kind of a lie, this isn't in non-volatile memory (it's a
> kernel buffer) and it's obviously volatile in the sense that it can be changed
> by the PSP.  I get that from the PSP's perspective it's _intended_ to be NV
> storage, but (a) this does not point at NV storage and (b) there is no guarantee
> that the path provided by userspace points at NV storage.
>
> Maybe "sev_init_ex_buffer" or something along those lines?
>
> >  static inline bool sev_version_greater_or_equal(u8 maj, u8 min)
> >  {
> >       struct sev_device *sev = psp_master->sev_data;
> > @@ -107,6 +120,7 @@ static int sev_cmd_buffer_len(int cmd)
> >  {
> >       switch (cmd) {
> >       case SEV_CMD_INIT:                      return sizeof(struct sev_data_init);
> > +     case SEV_CMD_INIT_EX:                   return sizeof(struct sev_data_init_ex);
> >       case SEV_CMD_PLATFORM_STATUS:           return sizeof(struct sev_user_data_status);
> >       case SEV_CMD_PEK_CSR:                   return sizeof(struct sev_data_pek_csr);
> >       case SEV_CMD_PEK_CERT_IMPORT:           return sizeof(struct sev_data_pek_cert_import);
> > @@ -152,6 +166,87 @@ static void *sev_fw_alloc(unsigned long len)
> >       return page_address(page);
> >  }
> >
> > +static int sev_read_nv_memory(void)
>
> Similar to above, this is reading from a file that's backed by who knows what,
> i.e. it may or may not be reading NV memory.  And if it's going to implicitly
> read into the buffer, that should be reflected in the name.
>
> > +{
> > +     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)) {
> > +             int ret = PTR_ERR(fp);
> > +
> > +             dev_err(psp_master->dev,
> > +                     "SEV: could not open %s for read, error %d\n",
> > +                     init_ex_path, ret);
> > +             return ret;
> > +     }
> > +
> > +     nread = kernel_read(fp, sev_init_ex_nv_address, NV_LENGTH, NULL);
> > +     dev_dbg(psp_master->dev, "SEV: read %ld bytes from NV file\n", nread);
> > +     filp_close(fp, NULL);
> > +
> > +     return 0;
> > +}
> > +
> > +static void sev_write_nv_memory(void)
>
> Same comments here.
>
> > +{
> > +     struct sev_device *sev = psp_master->sev_data;
> > +     struct file *fp;
> > +     loff_t offset = 0;
> > +     ssize_t nwrite;
> > +
> > +     if (!sev_init_ex_nv_address)
> > +             return;
> > +
> > +     fp = filp_open(init_ex_path, O_CREAT | O_WRONLY, 0600);
> > +     if (IS_ERR(fp)) {
> > +             dev_err(sev->dev,
> > +                     "SEV: could not open file for write, error %d\n",
> > +                     PTR_ERR(fp));
> > +             return;
> > +     }
> > +
> > +     nwrite = kernel_write(fp, sev_init_ex_nv_address, NV_LENGTH, &offset);
> > +     vfs_fsync(fp, 0);
> > +     filp_close(fp, NULL);
> > +
> > +     if (nwrite != NV_LENGTH) {
> > +             dev_err(sev->dev,
> > +                     "SEV: failed to write %u bytes to non volatile memory area, ret %ld\n",
> > +                     NV_LENGTH, nwrite);
> > +             return;
> > +     }
> > +
> > +     dev_dbg(sev->dev, "SEV: write successful to NV file\n");
> > +}
> > +
> > +static void sev_write_nv_memory_if_required(int cmd_id)
>
> And here.

I'll update the naming.

>
> > +{
> > +     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(_EX), 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:
> > +             break;
> > +     default:
> > +             return;
> > +     };
> > +
> > +     sev_write_nv_memory();
> > +}
> > +
> >  static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
> >  {
> >       struct psp_device *psp = psp_master;
> > @@ -221,6 +316,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,
> > @@ -247,22 +344,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);
>
> I realize this is copy-pasted from existing code, by why bother with an intermediate
> tmr_pa?  Just set data.tmr_address directly.

I can fix that up in the next revison.

>
> >
> > -     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();
>
> Why defer reading the file until INIT_EX?  Why not read the data when the path
> is first verified and consumed?  More comments below in the retry path.
>
> And what happens when sev_platform_init() is reached through a path other than
> sev_pci_init()?  I guess that's a question for the existing code since sev_es_tmr
> will be NULL unless sev_platform_init() routes through sev_pci_init().  So either
> the sev_platform_init() call from KVM's sev_guest_init() is pointless, or there's
> potential for some truly bizarre and confusing behavior.

Lets consolidate this discussion below.

>
> > +     if (ret)
> > +             return ret;
> > +
> >       if (sev_es_tmr) {
> >               u64 tmr_pa;
> >
> > @@ -277,7 +394,27 @@ static int __sev_platform_init_locked(int *error)
> >               data.tmr_len = SEV_ES_TMR_SIZE;
> >       }
> >
> > -     rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> > +     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);
> > +
> > +     if (!psp || !psp->sev_data)
> > +             return -ENODEV;
> > +
> > +     sev = psp->sev_data;
> > +
> > +     if (sev->state == SEV_STATE_INIT)
> > +             return 0;
> > +
> > +     init_function = sev_init_ex_nv_address ? __sev_init_ex_locked :
> > +         __sev_init_locked;
>
> There's no need for this to be a function pointer, and the duplicate code can be
> consolidated.
>
> static int sev_do_init_locked(int cmd, void *data, int *error)
> {
>         if (sev_es_tmr) {
>                 /*
>                  * Do not include the encryption mask on the physical
>                  * address of the TMR (firmware should clear it anyway).
>                  */
>                 data.flags |= SEV_INIT_FLAGS_SEV_ES;
>                 data.tmr_address = __pa(sev_es_tmr);
>                 data.tmr_len = SEV_ES_TMR_SIZE;
>         }
>         return __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> }
>
> static int __sev_init_locked(int *error)
> {
>         struct sev_data_init data;
>
>         memset(&data, 0, sizeof(data));
>         return sev_do_init_locked(cmd, &data, error);
> }
>
> static int __sev_init_ex_locked(int *error)
> {
>         struct sev_data_init_ex data;
>
>         memset(&data, 0, sizeof(data));
>         data.length = sizeof(data);
>         data.nv_address = __psp_pa(sev_init_ex_nv_address);
>         data.nv_len = NV_LENGTH;
>         return sev_do_init_locked(SEV_CMD_INIT_EX, &data, error);
> }

I am missing how this removes the duplication of the retry code,
parameter checking, and other error checking code.. With what you have
typed out I would assume I still need to function pointer between
__sev_init_ex_locked and __sev_init_locked. Can you please elaborate
here? Also is there some reason the function pointer is not
acceptable?

>
> > +     rc = init_function(error);
> >       if (rc && *error == SEV_RET_SECURE_DATA_INVALID) {
> >               /*
> >                * INIT command returned an integrity check failure
> > @@ -286,8 +423,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_notice(sev->dev, "SEV: retrying INIT command");
> > +             rc = init_function(error);
>
> The above comment says "persistent state has been erased", but __sev_do_cmd_locked()
> only writes back to the file if a relevant command was successful, which means
> that rereading the userspace file in __sev_init_ex_locked() will retry INIT_EX
> with the same garbage data.

Ack my mistake, that comment is stale. I will update it so its correct
for the INIT and INIT_EX flows.
>
> IMO, the behavior should be to read the file on load and then use the kernel buffer
> without ever reloading (unless this is built as a module and is unloaded and reloaded).
> The writeback then becomes opportunistic in the sense that if it fails for some reason,
> the kernel's internal state isn't blasted away.

One issue here is that the file read can fail on load so we use the
late retry to guarantee we can read the file. The other point seems
like preference. Users may wish to shutdown the PSP FW, load a new
file, and INIT_EX again with that new data. Why should we preclude
them from that functionality?

>
> >       }
> >
> >       if (rc)
> > @@ -303,7 +440,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)
> > @@ -1057,6 +1194,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) {
>
> It really seems like the the teardown path should be paranoid and do a final
> writeback of the kernel's buffer.

Tom asked me to remove that in the last revision.

>
> > +             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)
> > @@ -1101,6 +1244,18 @@ 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_err(sev->dev,
> > +                             "SEV: INIT_EX NV memory allocation failed\n");
> > +                     goto err;
>
> Why does this nullify psp_master->sev_data but later failures do not?

If the buffer cannot be allocated then we cannot INIT_EX the PSP so
the operation is completely stopped similar to the error condition
with having an acceptable PSP API version.

>
> > +             }
> > +     }
> > +
> >       /* 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
> >

Sean Christopherson Nov. 9, 2021, 8:26 p.m. UTC | #5

On Tue, Nov 09, 2021, Peter Gonda wrote:
> On Tue, Nov 9, 2021 at 10:21 AM Sean Christopherson <[e-mail]> wrote:
> > There's no need for this to be a function pointer, and the duplicate code can be
> > consolidated.
> >
> > static int sev_do_init_locked(int cmd, void *data, int *error)
> > {
> >         if (sev_es_tmr) {
> >                 /*
> >                  * Do not include the encryption mask on the physical
> >                  * address of the TMR (firmware should clear it anyway).
> >                  */
> >                 data.flags |= SEV_INIT_FLAGS_SEV_ES;
> >                 data.tmr_address = __pa(sev_es_tmr);
> >                 data.tmr_len = SEV_ES_TMR_SIZE;
> >         }
> >         return __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> > }
> >
> > static int __sev_init_locked(int *error)
> > {
> >         struct sev_data_init data;
> >
> >         memset(&data, 0, sizeof(data));
> >         return sev_do_init_locked(cmd, &data, error);
> > }
> >
> > static int __sev_init_ex_locked(int *error)
> > {
> >         struct sev_data_init_ex data;
> >
> >         memset(&data, 0, sizeof(data));
> >         data.length = sizeof(data);
> >         data.nv_address = __psp_pa(sev_init_ex_nv_address);
> >         data.nv_len = NV_LENGTH;
> >         return sev_do_init_locked(SEV_CMD_INIT_EX, &data, error);
> > }
> 
> I am missing how this removes the duplication of the retry code,
> parameter checking, and other error checking code.. With what you have
> typed out I would assume I still need to function pointer between
> __sev_init_ex_locked and __sev_init_locked. Can you please elaborate
> here?

Hmm.  Ah, I got distracted between the original thought, the realization that
the two commands used different structs, and typing up the above.

> Also is there some reason the function pointer is not acceptable?

It's not unacceptable, it would just be nice to avoid, assuming the alternative
is cleaner.  But I don't think any alternative is cleaner, since as you pointed
out the above is a half-baked thought.

> > > +     rc = init_function(error);
> > >       if (rc && *error == SEV_RET_SECURE_DATA_INVALID) {
> > >               /*
> > >                * INIT command returned an integrity check failure
> > > @@ -286,8 +423,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_notice(sev->dev, "SEV: retrying INIT command");
> > > +             rc = init_function(error);
> >
> > The above comment says "persistent state has been erased", but __sev_do_cmd_locked()
> > only writes back to the file if a relevant command was successful, which means
> > that rereading the userspace file in __sev_init_ex_locked() will retry INIT_EX
> > with the same garbage data.
> 
> Ack my mistake, that comment is stale. I will update it so its correct
> for the INIT and INIT_EX flows.
> >
> > IMO, the behavior should be to read the file on load and then use the kernel buffer
> > without ever reloading (unless this is built as a module and is unloaded and reloaded).
> > The writeback then becomes opportunistic in the sense that if it fails for some reason,
> > the kernel's internal state isn't blasted away.
> 
> One issue here is that the file read can fail on load so we use the
> late retry to guarantee we can read the file.

But why continue loading if reading the file fails on load?

> The other point seems like preference. Users may wish to shutdown the PSP FW,
> load a new file, and INIT_EX again with that new data. Why should we preclude
> them from that functionality?

I don't think we should preclude that functionality, but it needs to be explicitly
tied to a userspace action, e.g. either on module load or on writing the param to
change the path.  If the latter is allowed, then it needs to be denied if the PSP
is initialized, otherwise the kernel will be in a non-coherent state and AFAICT
userspace will have a heck of a time even understanding what state has been used
to initialize the PSP.

Peter Gonda Nov. 9, 2021, 8:46 p.m. UTC | #6

On Tue, Nov 9, 2021 at 1:26 PM Sean Christopherson <[e-mail]> wrote:
>
> On Tue, Nov 09, 2021, Peter Gonda wrote:
> > On Tue, Nov 9, 2021 at 10:21 AM Sean Christopherson <[e-mail]> wrote:
> > > There's no need for this to be a function pointer, and the duplicate code can be
> > > consolidated.
> > >
> > > static int sev_do_init_locked(int cmd, void *data, int *error)
> > > {
> > >         if (sev_es_tmr) {
> > >                 /*
> > >                  * Do not include the encryption mask on the physical
> > >                  * address of the TMR (firmware should clear it anyway).
> > >                  */
> > >                 data.flags |= SEV_INIT_FLAGS_SEV_ES;
> > >                 data.tmr_address = __pa(sev_es_tmr);
> > >                 data.tmr_len = SEV_ES_TMR_SIZE;
> > >         }
> > >         return __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> > > }
> > >
> > > static int __sev_init_locked(int *error)
> > > {
> > >         struct sev_data_init data;
> > >
> > >         memset(&data, 0, sizeof(data));
> > >         return sev_do_init_locked(cmd, &data, error);
> > > }
> > >
> > > static int __sev_init_ex_locked(int *error)
> > > {
> > >         struct sev_data_init_ex data;
> > >
> > >         memset(&data, 0, sizeof(data));
> > >         data.length = sizeof(data);
> > >         data.nv_address = __psp_pa(sev_init_ex_nv_address);
> > >         data.nv_len = NV_LENGTH;
> > >         return sev_do_init_locked(SEV_CMD_INIT_EX, &data, error);
> > > }
> >
> > I am missing how this removes the duplication of the retry code,
> > parameter checking, and other error checking code.. With what you have
> > typed out I would assume I still need to function pointer between
> > __sev_init_ex_locked and __sev_init_locked. Can you please elaborate
> > here?
>
> Hmm.  Ah, I got distracted between the original thought, the realization that
> the two commands used different structs, and typing up the above.
>
> > Also is there some reason the function pointer is not acceptable?
>
> It's not unacceptable, it would just be nice to avoid, assuming the alternative
> is cleaner.  But I don't think any alternative is cleaner, since as you pointed
> out the above is a half-baked thought.

OK I'll leave as is.

>
> > > > +     rc = init_function(error);
> > > >       if (rc && *error == SEV_RET_SECURE_DATA_INVALID) {
> > > >               /*
> > > >                * INIT command returned an integrity check failure
> > > > @@ -286,8 +423,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_notice(sev->dev, "SEV: retrying INIT command");
> > > > +             rc = init_function(error);
> > >
> > > The above comment says "persistent state has been erased", but __sev_do_cmd_locked()
> > > only writes back to the file if a relevant command was successful, which means
> > > that rereading the userspace file in __sev_init_ex_locked() will retry INIT_EX
> > > with the same garbage data.
> >
> > Ack my mistake, that comment is stale. I will update it so its correct
> > for the INIT and INIT_EX flows.
> > >
> > > IMO, the behavior should be to read the file on load and then use the kernel buffer
> > > without ever reloading (unless this is built as a module and is unloaded and reloaded).
> > > The writeback then becomes opportunistic in the sense that if it fails for some reason,
> > > the kernel's internal state isn't blasted away.
> >
> > One issue here is that the file read can fail on load so we use the
> > late retry to guarantee we can read the file.
>
> But why continue loading if reading the file fails on load?
>
> > The other point seems like preference. Users may wish to shutdown the PSP FW,
> > load a new file, and INIT_EX again with that new data. Why should we preclude
> > them from that functionality?
>
> I don't think we should preclude that functionality, but it needs to be explicitly
> tied to a userspace action, e.g. either on module load or on writing the param to
> change the path.  If the latter is allowed, then it needs to be denied if the PSP
> is initialized, otherwise the kernel will be in a non-coherent state and AFAICT
> userspace will have a heck of a time even understanding what state has been used
> to initialize the PSP.

If this driver is builtin the filesystem will be unavailable during
__init. Using the existing retries already built into
sev_platform_init() also the file to be read once userspace is
running, meaning the file system is usable. As I tried to explain in
the commit message. We could remove the sev_platform_init call during
sev_pci_init since this only actually needs to be initialized when the
first command requiring it is issues (either reading some keys/certs
from the PSP or launching an SEV guest). Then userspace in both the
builtin and module usage would know running one of those commands
cause the file to be read for PSP usage. Tom any thoughts on this?

Brijesh Singh Nov. 9, 2021, 10:19 p.m. UTC | #7

On 11/9/21 2:46 PM, Peter Gonda wrote:
> On Tue, Nov 9, 2021 at 1:26 PM Sean Christopherson <[e-mail]> wrote:
>>
>> On Tue, Nov 09, 2021, Peter Gonda wrote:
>>> On Tue, Nov 9, 2021 at 10:21 AM Sean Christopherson <[e-mail]> wrote:
>>>> There's no need for this to be a function pointer, and the duplicate code can be
>>>> consolidated.
>>>>
>>>> static int sev_do_init_locked(int cmd, void *data, int *error)
>>>> {
>>>>          if (sev_es_tmr) {
>>>>                  /*
>>>>                   * Do not include the encryption mask on the physical
>>>>                   * address of the TMR (firmware should clear it anyway).
>>>>                   */
>>>>                  data.flags |= SEV_INIT_FLAGS_SEV_ES;
>>>>                  data.tmr_address = __pa(sev_es_tmr);
>>>>                  data.tmr_len = SEV_ES_TMR_SIZE;
>>>>          }
>>>>          return __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
>>>> }
>>>>
>>>> static int __sev_init_locked(int *error)
>>>> {
>>>>          struct sev_data_init data;
>>>>
>>>>          memset(&data, 0, sizeof(data));
>>>>          return sev_do_init_locked(cmd, &data, error);
>>>> }
>>>>
>>>> static int __sev_init_ex_locked(int *error)
>>>> {
>>>>          struct sev_data_init_ex data;
>>>>
>>>>          memset(&data, 0, sizeof(data));
>>>>          data.length = sizeof(data);
>>>>          data.nv_address = __psp_pa(sev_init_ex_nv_address);
>>>>          data.nv_len = NV_LENGTH;
>>>>          return sev_do_init_locked(SEV_CMD_INIT_EX, &data, error);
>>>> }
>>>
>>> I am missing how this removes the duplication of the retry code,
>>> parameter checking, and other error checking code.. With what you have
>>> typed out I would assume I still need to function pointer between
>>> __sev_init_ex_locked and __sev_init_locked. Can you please elaborate
>>> here?
>>
>> Hmm.  Ah, I got distracted between the original thought, the realization that
>> the two commands used different structs, and typing up the above.
>>
>>> Also is there some reason the function pointer is not acceptable?
>>
>> It's not unacceptable, it would just be nice to avoid, assuming the alternative
>> is cleaner.  But I don't think any alternative is cleaner, since as you pointed
>> out the above is a half-baked thought.
> 
> OK I'll leave as is.
> 
>>
>>>>> +     rc = init_function(error);
>>>>>        if (rc && *error == SEV_RET_SECURE_DATA_INVALID) {
>>>>>                /*
>>>>>                 * INIT command returned an integrity check failure
>>>>> @@ -286,8 +423,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_notice(sev->dev, "SEV: retrying INIT command");
>>>>> +             rc = init_function(error);
>>>>
>>>> The above comment says "persistent state has been erased", but __sev_do_cmd_locked()
>>>> only writes back to the file if a relevant command was successful, which means
>>>> that rereading the userspace file in __sev_init_ex_locked() will retry INIT_EX
>>>> with the same garbage data.
>>>
>>> Ack my mistake, that comment is stale. I will update it so its correct
>>> for the INIT and INIT_EX flows.
>>>>
>>>> IMO, the behavior should be to read the file on load and then use the kernel buffer
>>>> without ever reloading (unless this is built as a module and is unloaded and reloaded).
>>>> The writeback then becomes opportunistic in the sense that if it fails for some reason,
>>>> the kernel's internal state isn't blasted away.
>>>
>>> One issue here is that the file read can fail on load so we use the
>>> late retry to guarantee we can read the file.
>>
>> But why continue loading if reading the file fails on load?
>>
>>> The other point seems like preference. Users may wish to shutdown the PSP FW,
>>> load a new file, and INIT_EX again with that new data. Why should we preclude
>>> them from that functionality?
>>
>> I don't think we should preclude that functionality, but it needs to be explicitly
>> tied to a userspace action, e.g. either on module load or on writing the param to
>> change the path.  If the latter is allowed, then it needs to be denied if the PSP
>> is initialized, otherwise the kernel will be in a non-coherent state and AFAICT
>> userspace will have a heck of a time even understanding what state has been used
>> to initialize the PSP.
> 
> If this driver is builtin the filesystem will be unavailable during
> __init. Using the existing retries already built into
> sev_platform_init() also the file to be read once userspace is
> running, meaning the file system is usable. As I tried to explain in
> the commit message. We could remove the sev_platform_init call during
> sev_pci_init since this only actually needs to be initialized when the
> first command requiring it is issues (either reading some keys/certs
> from the PSP or launching an SEV guest). Then userspace in both the
> builtin and module usage would know running one of those commands
> cause the file to be read for PSP usage. Tom any thoughts on this?
> 

One thing to note is that if we do the INIT on the first command then 
the first guest launch will take a longer. The init command is not 
cheap (especially with the SNP, it may take a longer because it has to 
do all those RMP setup etc). IIRC, in my early SEV series in I was doing 
the INIT during the first command execution and based on the 
recommendation moved to do the init on probe.

Should we add a module param to control whether to do INIT on probe or 
delay until the first command ?

-Brijesh

Peter Gonda Nov. 10, 2021, 3:32 p.m. UTC | #8

On Tue, Nov 9, 2021 at 3:20 PM Brijesh Singh <[e-mail]> wrote:
>
>
>
> On 11/9/21 2:46 PM, Peter Gonda wrote:
> > On Tue, Nov 9, 2021 at 1:26 PM Sean Christopherson <[e-mail]> wrote:
> >>
> >> On Tue, Nov 09, 2021, Peter Gonda wrote:
> >>> On Tue, Nov 9, 2021 at 10:21 AM Sean Christopherson <[e-mail]> wrote:
> >>>> There's no need for this to be a function pointer, and the duplicate code can be
> >>>> consolidated.
> >>>>
> >>>> static int sev_do_init_locked(int cmd, void *data, int *error)
> >>>> {
> >>>>          if (sev_es_tmr) {
> >>>>                  /*
> >>>>                   * Do not include the encryption mask on the physical
> >>>>                   * address of the TMR (firmware should clear it anyway).
> >>>>                   */
> >>>>                  data.flags |= SEV_INIT_FLAGS_SEV_ES;
> >>>>                  data.tmr_address = __pa(sev_es_tmr);
> >>>>                  data.tmr_len = SEV_ES_TMR_SIZE;
> >>>>          }
> >>>>          return __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> >>>> }
> >>>>
> >>>> static int __sev_init_locked(int *error)
> >>>> {
> >>>>          struct sev_data_init data;
> >>>>
> >>>>          memset(&data, 0, sizeof(data));
> >>>>          return sev_do_init_locked(cmd, &data, error);
> >>>> }
> >>>>
> >>>> static int __sev_init_ex_locked(int *error)
> >>>> {
> >>>>          struct sev_data_init_ex data;
> >>>>
> >>>>          memset(&data, 0, sizeof(data));
> >>>>          data.length = sizeof(data);
> >>>>          data.nv_address = __psp_pa(sev_init_ex_nv_address);
> >>>>          data.nv_len = NV_LENGTH;
> >>>>          return sev_do_init_locked(SEV_CMD_INIT_EX, &data, error);
> >>>> }
> >>>
> >>> I am missing how this removes the duplication of the retry code,
> >>> parameter checking, and other error checking code.. With what you have
> >>> typed out I would assume I still need to function pointer between
> >>> __sev_init_ex_locked and __sev_init_locked. Can you please elaborate
> >>> here?
> >>
> >> Hmm.  Ah, I got distracted between the original thought, the realization that
> >> the two commands used different structs, and typing up the above.
> >>
> >>> Also is there some reason the function pointer is not acceptable?
> >>
> >> It's not unacceptable, it would just be nice to avoid, assuming the alternative
> >> is cleaner.  But I don't think any alternative is cleaner, since as you pointed
> >> out the above is a half-baked thought.
> >
> > OK I'll leave as is.
> >
> >>
> >>>>> +     rc = init_function(error);
> >>>>>        if (rc && *error == SEV_RET_SECURE_DATA_INVALID) {
> >>>>>                /*
> >>>>>                 * INIT command returned an integrity check failure
> >>>>> @@ -286,8 +423,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_notice(sev->dev, "SEV: retrying INIT command");
> >>>>> +             rc = init_function(error);
> >>>>
> >>>> The above comment says "persistent state has been erased", but __sev_do_cmd_locked()
> >>>> only writes back to the file if a relevant command was successful, which means
> >>>> that rereading the userspace file in __sev_init_ex_locked() will retry INIT_EX
> >>>> with the same garbage data.
> >>>
> >>> Ack my mistake, that comment is stale. I will update it so its correct
> >>> for the INIT and INIT_EX flows.
> >>>>
> >>>> IMO, the behavior should be to read the file on load and then use the kernel buffer
> >>>> without ever reloading (unless this is built as a module and is unloaded and reloaded).
> >>>> The writeback then becomes opportunistic in the sense that if it fails for some reason,
> >>>> the kernel's internal state isn't blasted away.
> >>>
> >>> One issue here is that the file read can fail on load so we use the
> >>> late retry to guarantee we can read the file.
> >>
> >> But why continue loading if reading the file fails on load?
> >>
> >>> The other point seems like preference. Users may wish to shutdown the PSP FW,
> >>> load a new file, and INIT_EX again with that new data. Why should we preclude
> >>> them from that functionality?
> >>
> >> I don't think we should preclude that functionality, but it needs to be explicitly
> >> tied to a userspace action, e.g. either on module load or on writing the param to
> >> change the path.  If the latter is allowed, then it needs to be denied if the PSP
> >> is initialized, otherwise the kernel will be in a non-coherent state and AFAICT
> >> userspace will have a heck of a time even understanding what state has been used
> >> to initialize the PSP.
> >
> > If this driver is builtin the filesystem will be unavailable during
> > __init. Using the existing retries already built into
> > sev_platform_init() also the file to be read once userspace is
> > running, meaning the file system is usable. As I tried to explain in
> > the commit message. We could remove the sev_platform_init call during
> > sev_pci_init since this only actually needs to be initialized when the
> > first command requiring it is issues (either reading some keys/certs
> > from the PSP or launching an SEV guest). Then userspace in both the
> > builtin and module usage would know running one of those commands
> > cause the file to be read for PSP usage. Tom any thoughts on this?
> >
>
> One thing to note is that if we do the INIT on the first command then
> the first guest launch will take a longer. The init command is not
> cheap (especially with the SNP, it may take a longer because it has to
> do all those RMP setup etc). IIRC, in my early SEV series in I was doing
> the INIT during the first command execution and based on the
> recommendation moved to do the init on probe.
>
> Should we add a module param to control whether to do INIT on probe or
> delay until the first command ?

Thats a good point Brijesh. I've only been testing this with SEV and
ES so haven't noticed that long setup time. I like the idea of a
module parameter to decide when to INIT, that should satisfy Sean's
concern that the user doesn't know when the INIT_EX file would be read
and that there is extra retry code (duplicated between sev_pci_init
and all the PSP commands). I'll get started on that.

>
> -Brijesh

Peter Gonda Nov. 12, 2021, 4:55 p.m. UTC | #9

On Wed, Nov 10, 2021 at 8:32 AM Peter Gonda <[e-mail]> wrote:
>
> On Tue, Nov 9, 2021 at 3:20 PM Brijesh Singh <[e-mail]> wrote:
> >
> >
> >
> > On 11/9/21 2:46 PM, Peter Gonda wrote:
> > > On Tue, Nov 9, 2021 at 1:26 PM Sean Christopherson <[e-mail]> wrote:
> > >>
> > >> On Tue, Nov 09, 2021, Peter Gonda wrote:
> > >>> On Tue, Nov 9, 2021 at 10:21 AM Sean Christopherson <[e-mail]> wrote:
> > >>>> There's no need for this to be a function pointer, and the duplicate code can be
> > >>>> consolidated.
> > >>>>
> > >>>> static int sev_do_init_locked(int cmd, void *data, int *error)
> > >>>> {
> > >>>>          if (sev_es_tmr) {
> > >>>>                  /*
> > >>>>                   * Do not include the encryption mask on the physical
> > >>>>                   * address of the TMR (firmware should clear it anyway).
> > >>>>                   */
> > >>>>                  data.flags |= SEV_INIT_FLAGS_SEV_ES;
> > >>>>                  data.tmr_address = __pa(sev_es_tmr);
> > >>>>                  data.tmr_len = SEV_ES_TMR_SIZE;
> > >>>>          }
> > >>>>          return __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> > >>>> }
> > >>>>
> > >>>> static int __sev_init_locked(int *error)
> > >>>> {
> > >>>>          struct sev_data_init data;
> > >>>>
> > >>>>          memset(&data, 0, sizeof(data));
> > >>>>          return sev_do_init_locked(cmd, &data, error);
> > >>>> }
> > >>>>
> > >>>> static int __sev_init_ex_locked(int *error)
> > >>>> {
> > >>>>          struct sev_data_init_ex data;
> > >>>>
> > >>>>          memset(&data, 0, sizeof(data));
> > >>>>          data.length = sizeof(data);
> > >>>>          data.nv_address = __psp_pa(sev_init_ex_nv_address);
> > >>>>          data.nv_len = NV_LENGTH;
> > >>>>          return sev_do_init_locked(SEV_CMD_INIT_EX, &data, error);
> > >>>> }
> > >>>
> > >>> I am missing how this removes the duplication of the retry code,
> > >>> parameter checking, and other error checking code.. With what you have
> > >>> typed out I would assume I still need to function pointer between
> > >>> __sev_init_ex_locked and __sev_init_locked. Can you please elaborate
> > >>> here?
> > >>
> > >> Hmm.  Ah, I got distracted between the original thought, the realization that
> > >> the two commands used different structs, and typing up the above.
> > >>
> > >>> Also is there some reason the function pointer is not acceptable?
> > >>
> > >> It's not unacceptable, it would just be nice to avoid, assuming the alternative
> > >> is cleaner.  But I don't think any alternative is cleaner, since as you pointed
> > >> out the above is a half-baked thought.
> > >
> > > OK I'll leave as is.
> > >
> > >>
> > >>>>> +     rc = init_function(error);
> > >>>>>        if (rc && *error == SEV_RET_SECURE_DATA_INVALID) {
> > >>>>>                /*
> > >>>>>                 * INIT command returned an integrity check failure
> > >>>>> @@ -286,8 +423,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_notice(sev->dev, "SEV: retrying INIT command");
> > >>>>> +             rc = init_function(error);
> > >>>>
> > >>>> The above comment says "persistent state has been erased", but __sev_do_cmd_locked()
> > >>>> only writes back to the file if a relevant command was successful, which means
> > >>>> that rereading the userspace file in __sev_init_ex_locked() will retry INIT_EX
> > >>>> with the same garbage data.
> > >>>
> > >>> Ack my mistake, that comment is stale. I will update it so its correct
> > >>> for the INIT and INIT_EX flows.
> > >>>>
> > >>>> IMO, the behavior should be to read the file on load and then use the kernel buffer
> > >>>> without ever reloading (unless this is built as a module and is unloaded and reloaded).
> > >>>> The writeback then becomes opportunistic in the sense that if it fails for some reason,
> > >>>> the kernel's internal state isn't blasted away.
> > >>>
> > >>> One issue here is that the file read can fail on load so we use the
> > >>> late retry to guarantee we can read the file.
> > >>
> > >> But why continue loading if reading the file fails on load?
> > >>
> > >>> The other point seems like preference. Users may wish to shutdown the PSP FW,
> > >>> load a new file, and INIT_EX again with that new data. Why should we preclude
> > >>> them from that functionality?
> > >>
> > >> I don't think we should preclude that functionality, but it needs to be explicitly
> > >> tied to a userspace action, e.g. either on module load or on writing the param to
> > >> change the path.  If the latter is allowed, then it needs to be denied if the PSP
> > >> is initialized, otherwise the kernel will be in a non-coherent state and AFAICT
> > >> userspace will have a heck of a time even understanding what state has been used
> > >> to initialize the PSP.
> > >
> > > If this driver is builtin the filesystem will be unavailable during
> > > __init. Using the existing retries already built into
> > > sev_platform_init() also the file to be read once userspace is
> > > running, meaning the file system is usable. As I tried to explain in
> > > the commit message. We could remove the sev_platform_init call during
> > > sev_pci_init since this only actually needs to be initialized when the
> > > first command requiring it is issues (either reading some keys/certs
> > > from the PSP or launching an SEV guest). Then userspace in both the
> > > builtin and module usage would know running one of those commands
> > > cause the file to be read for PSP usage. Tom any thoughts on this?
> > >
> >
> > One thing to note is that if we do the INIT on the first command then
> > the first guest launch will take a longer. The init command is not
> > cheap (especially with the SNP, it may take a longer because it has to
> > do all those RMP setup etc). IIRC, in my early SEV series in I was doing
> > the INIT during the first command execution and based on the
> > recommendation moved to do the init on probe.
> >
> > Should we add a module param to control whether to do INIT on probe or
> > delay until the first command ?
>
> Thats a good point Brijesh. I've only been testing this with SEV and
> ES so haven't noticed that long setup time. I like the idea of a
> module parameter to decide when to INIT, that should satisfy Sean's
> concern that the user doesn't know when the INIT_EX file would be read
> and that there is extra retry code (duplicated between sev_pci_init
> and all the PSP commands). I'll get started on that.

I need a little guidance on how to proceed with this. Should I have
the new module parameter 'psp_init_on_probe' just disable PSP init on
module init if false. Or should it also disable PSP init during
command flow if it's true?

I was thinking I should just have 'psp_init_on_probe' default to true,
and if false it stops the PSP init during sev_pci_init(). If I add the
second change that seems like it changes the ABI. Thoughts?

>
> >
> > -Brijesh

Marc Orr Nov. 12, 2021, 5:46 p.m. UTC | #10

On Fri, Nov 12, 2021 at 8:55 AM Peter Gonda <[e-mail]> wrote:
>
> On Wed, Nov 10, 2021 at 8:32 AM Peter Gonda <[e-mail]> wrote:
> >
> > On Tue, Nov 9, 2021 at 3:20 PM Brijesh Singh <[e-mail]> wrote:
> > >
> > >
> > >
> > > On 11/9/21 2:46 PM, Peter Gonda wrote:
> > > > On Tue, Nov 9, 2021 at 1:26 PM Sean Christopherson <[e-mail]> wrote:
> > > >>
> > > >> On Tue, Nov 09, 2021, Peter Gonda wrote:
> > > >>> On Tue, Nov 9, 2021 at 10:21 AM Sean Christopherson <[e-mail]> wrote:
> > > >>>> There's no need for this to be a function pointer, and the duplicate code can be
> > > >>>> consolidated.
> > > >>>>
> > > >>>> static int sev_do_init_locked(int cmd, void *data, int *error)
> > > >>>> {
> > > >>>>          if (sev_es_tmr) {
> > > >>>>                  /*
> > > >>>>                   * Do not include the encryption mask on the physical
> > > >>>>                   * address of the TMR (firmware should clear it anyway).
> > > >>>>                   */
> > > >>>>                  data.flags |= SEV_INIT_FLAGS_SEV_ES;
> > > >>>>                  data.tmr_address = __pa(sev_es_tmr);
> > > >>>>                  data.tmr_len = SEV_ES_TMR_SIZE;
> > > >>>>          }
> > > >>>>          return __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> > > >>>> }
> > > >>>>
> > > >>>> static int __sev_init_locked(int *error)
> > > >>>> {
> > > >>>>          struct sev_data_init data;
> > > >>>>
> > > >>>>          memset(&data, 0, sizeof(data));
> > > >>>>          return sev_do_init_locked(cmd, &data, error);
> > > >>>> }
> > > >>>>
> > > >>>> static int __sev_init_ex_locked(int *error)
> > > >>>> {
> > > >>>>          struct sev_data_init_ex data;
> > > >>>>
> > > >>>>          memset(&data, 0, sizeof(data));
> > > >>>>          data.length = sizeof(data);
> > > >>>>          data.nv_address = __psp_pa(sev_init_ex_nv_address);
> > > >>>>          data.nv_len = NV_LENGTH;
> > > >>>>          return sev_do_init_locked(SEV_CMD_INIT_EX, &data, error);
> > > >>>> }
> > > >>>
> > > >>> I am missing how this removes the duplication of the retry code,
> > > >>> parameter checking, and other error checking code.. With what you have
> > > >>> typed out I would assume I still need to function pointer between
> > > >>> __sev_init_ex_locked and __sev_init_locked. Can you please elaborate
> > > >>> here?
> > > >>
> > > >> Hmm.  Ah, I got distracted between the original thought, the realization that
> > > >> the two commands used different structs, and typing up the above.
> > > >>
> > > >>> Also is there some reason the function pointer is not acceptable?
> > > >>
> > > >> It's not unacceptable, it would just be nice to avoid, assuming the alternative
> > > >> is cleaner.  But I don't think any alternative is cleaner, since as you pointed
> > > >> out the above is a half-baked thought.
> > > >
> > > > OK I'll leave as is.
> > > >
> > > >>
> > > >>>>> +     rc = init_function(error);
> > > >>>>>        if (rc && *error == SEV_RET_SECURE_DATA_INVALID) {
> > > >>>>>                /*
> > > >>>>>                 * INIT command returned an integrity check failure
> > > >>>>> @@ -286,8 +423,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_notice(sev->dev, "SEV: retrying INIT command");
> > > >>>>> +             rc = init_function(error);
> > > >>>>
> > > >>>> The above comment says "persistent state has been erased", but __sev_do_cmd_locked()
> > > >>>> only writes back to the file if a relevant command was successful, which means
> > > >>>> that rereading the userspace file in __sev_init_ex_locked() will retry INIT_EX
> > > >>>> with the same garbage data.
> > > >>>
> > > >>> Ack my mistake, that comment is stale. I will update it so its correct
> > > >>> for the INIT and INIT_EX flows.
> > > >>>>
> > > >>>> IMO, the behavior should be to read the file on load and then use the kernel buffer
> > > >>>> without ever reloading (unless this is built as a module and is unloaded and reloaded).
> > > >>>> The writeback then becomes opportunistic in the sense that if it fails for some reason,
> > > >>>> the kernel's internal state isn't blasted away.
> > > >>>
> > > >>> One issue here is that the file read can fail on load so we use the
> > > >>> late retry to guarantee we can read the file.
> > > >>
> > > >> But why continue loading if reading the file fails on load?
> > > >>
> > > >>> The other point seems like preference. Users may wish to shutdown the PSP FW,
> > > >>> load a new file, and INIT_EX again with that new data. Why should we preclude
> > > >>> them from that functionality?
> > > >>
> > > >> I don't think we should preclude that functionality, but it needs to be explicitly
> > > >> tied to a userspace action, e.g. either on module load or on writing the param to
> > > >> change the path.  If the latter is allowed, then it needs to be denied if the PSP
> > > >> is initialized, otherwise the kernel will be in a non-coherent state and AFAICT
> > > >> userspace will have a heck of a time even understanding what state has been used
> > > >> to initialize the PSP.
> > > >
> > > > If this driver is builtin the filesystem will be unavailable during
> > > > __init. Using the existing retries already built into
> > > > sev_platform_init() also the file to be read once userspace is
> > > > running, meaning the file system is usable. As I tried to explain in
> > > > the commit message. We could remove the sev_platform_init call during
> > > > sev_pci_init since this only actually needs to be initialized when the
> > > > first command requiring it is issues (either reading some keys/certs
> > > > from the PSP or launching an SEV guest). Then userspace in both the
> > > > builtin and module usage would know running one of those commands
> > > > cause the file to be read for PSP usage. Tom any thoughts on this?
> > > >
> > >
> > > One thing to note is that if we do the INIT on the first command then
> > > the first guest launch will take a longer. The init command is not
> > > cheap (especially with the SNP, it may take a longer because it has to
> > > do all those RMP setup etc). IIRC, in my early SEV series in I was doing
> > > the INIT during the first command execution and based on the
> > > recommendation moved to do the init on probe.
> > >
> > > Should we add a module param to control whether to do INIT on probe or
> > > delay until the first command ?
> >
> > Thats a good point Brijesh. I've only been testing this with SEV and
> > ES so haven't noticed that long setup time. I like the idea of a
> > module parameter to decide when to INIT, that should satisfy Sean's
> > concern that the user doesn't know when the INIT_EX file would be read
> > and that there is extra retry code (duplicated between sev_pci_init
> > and all the PSP commands). I'll get started on that.
>
> I need a little guidance on how to proceed with this. Should I have
> the new module parameter 'psp_init_on_probe' just disable PSP init on
> module init if false. Or should it also disable PSP init during
> command flow if it's true?
>
> I was thinking I should just have 'psp_init_on_probe' default to true,
> and if false it stops the PSP init during sev_pci_init(). If I add the
> second change that seems like it changes the ABI. Thoughts?

What about doing the INIT when we load the KVM module? Does that
resolve all of these problems? By the time we load the KVM module, we
know that the file system is up, which is the original problem we were
trying to solve. And the KVM module is most likely loaded before we
run the first guest.

Peter Gonda Nov. 12, 2021, 5:49 p.m. UTC | #11

On Fri, Nov 12, 2021 at 10:46 AM Marc Orr <[e-mail]> wrote:
>
> On Fri, Nov 12, 2021 at 8:55 AM Peter Gonda <[e-mail]> wrote:
> >
> > On Wed, Nov 10, 2021 at 8:32 AM Peter Gonda <[e-mail]> wrote:
> > >
> > > On Tue, Nov 9, 2021 at 3:20 PM Brijesh Singh <[e-mail]> wrote:
> > > >
> > > >
> > > >
> > > > On 11/9/21 2:46 PM, Peter Gonda wrote:
> > > > > On Tue, Nov 9, 2021 at 1:26 PM Sean Christopherson <[e-mail]> wrote:
> > > > >>
> > > > >> On Tue, Nov 09, 2021, Peter Gonda wrote:
> > > > >>> On Tue, Nov 9, 2021 at 10:21 AM Sean Christopherson <[e-mail]> wrote:
> > > > >>>> There's no need for this to be a function pointer, and the duplicate code can be
> > > > >>>> consolidated.
> > > > >>>>
> > > > >>>> static int sev_do_init_locked(int cmd, void *data, int *error)
> > > > >>>> {
> > > > >>>>          if (sev_es_tmr) {
> > > > >>>>                  /*
> > > > >>>>                   * Do not include the encryption mask on the physical
> > > > >>>>                   * address of the TMR (firmware should clear it anyway).
> > > > >>>>                   */
> > > > >>>>                  data.flags |= SEV_INIT_FLAGS_SEV_ES;
> > > > >>>>                  data.tmr_address = __pa(sev_es_tmr);
> > > > >>>>                  data.tmr_len = SEV_ES_TMR_SIZE;
> > > > >>>>          }
> > > > >>>>          return __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> > > > >>>> }
> > > > >>>>
> > > > >>>> static int __sev_init_locked(int *error)
> > > > >>>> {
> > > > >>>>          struct sev_data_init data;
> > > > >>>>
> > > > >>>>          memset(&data, 0, sizeof(data));
> > > > >>>>          return sev_do_init_locked(cmd, &data, error);
> > > > >>>> }
> > > > >>>>
> > > > >>>> static int __sev_init_ex_locked(int *error)
> > > > >>>> {
> > > > >>>>          struct sev_data_init_ex data;
> > > > >>>>
> > > > >>>>          memset(&data, 0, sizeof(data));
> > > > >>>>          data.length = sizeof(data);
> > > > >>>>          data.nv_address = __psp_pa(sev_init_ex_nv_address);
> > > > >>>>          data.nv_len = NV_LENGTH;
> > > > >>>>          return sev_do_init_locked(SEV_CMD_INIT_EX, &data, error);
> > > > >>>> }
> > > > >>>
> > > > >>> I am missing how this removes the duplication of the retry code,
> > > > >>> parameter checking, and other error checking code.. With what you have
> > > > >>> typed out I would assume I still need to function pointer between
> > > > >>> __sev_init_ex_locked and __sev_init_locked. Can you please elaborate
> > > > >>> here?
> > > > >>
> > > > >> Hmm.  Ah, I got distracted between the original thought, the realization that
> > > > >> the two commands used different structs, and typing up the above.
> > > > >>
> > > > >>> Also is there some reason the function pointer is not acceptable?
> > > > >>
> > > > >> It's not unacceptable, it would just be nice to avoid, assuming the alternative
> > > > >> is cleaner.  But I don't think any alternative is cleaner, since as you pointed
> > > > >> out the above is a half-baked thought.
> > > > >
> > > > > OK I'll leave as is.
> > > > >
> > > > >>
> > > > >>>>> +     rc = init_function(error);
> > > > >>>>>        if (rc && *error == SEV_RET_SECURE_DATA_INVALID) {
> > > > >>>>>                /*
> > > > >>>>>                 * INIT command returned an integrity check failure
> > > > >>>>> @@ -286,8 +423,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_notice(sev->dev, "SEV: retrying INIT command");
> > > > >>>>> +             rc = init_function(error);
> > > > >>>>
> > > > >>>> The above comment says "persistent state has been erased", but __sev_do_cmd_locked()
> > > > >>>> only writes back to the file if a relevant command was successful, which means
> > > > >>>> that rereading the userspace file in __sev_init_ex_locked() will retry INIT_EX
> > > > >>>> with the same garbage data.
> > > > >>>
> > > > >>> Ack my mistake, that comment is stale. I will update it so its correct
> > > > >>> for the INIT and INIT_EX flows.
> > > > >>>>
> > > > >>>> IMO, the behavior should be to read the file on load and then use the kernel buffer
> > > > >>>> without ever reloading (unless this is built as a module and is unloaded and reloaded).
> > > > >>>> The writeback then becomes opportunistic in the sense that if it fails for some reason,
> > > > >>>> the kernel's internal state isn't blasted away.
> > > > >>>
> > > > >>> One issue here is that the file read can fail on load so we use the
> > > > >>> late retry to guarantee we can read the file.
> > > > >>
> > > > >> But why continue loading if reading the file fails on load?
> > > > >>
> > > > >>> The other point seems like preference. Users may wish to shutdown the PSP FW,
> > > > >>> load a new file, and INIT_EX again with that new data. Why should we preclude
> > > > >>> them from that functionality?
> > > > >>
> > > > >> I don't think we should preclude that functionality, but it needs to be explicitly
> > > > >> tied to a userspace action, e.g. either on module load or on writing the param to
> > > > >> change the path.  If the latter is allowed, then it needs to be denied if the PSP
> > > > >> is initialized, otherwise the kernel will be in a non-coherent state and AFAICT
> > > > >> userspace will have a heck of a time even understanding what state has been used
> > > > >> to initialize the PSP.
> > > > >
> > > > > If this driver is builtin the filesystem will be unavailable during
> > > > > __init. Using the existing retries already built into
> > > > > sev_platform_init() also the file to be read once userspace is
> > > > > running, meaning the file system is usable. As I tried to explain in
> > > > > the commit message. We could remove the sev_platform_init call during
> > > > > sev_pci_init since this only actually needs to be initialized when the
> > > > > first command requiring it is issues (either reading some keys/certs
> > > > > from the PSP or launching an SEV guest). Then userspace in both the
> > > > > builtin and module usage would know running one of those commands
> > > > > cause the file to be read for PSP usage. Tom any thoughts on this?
> > > > >
> > > >
> > > > One thing to note is that if we do the INIT on the first command then
> > > > the first guest launch will take a longer. The init command is not
> > > > cheap (especially with the SNP, it may take a longer because it has to
> > > > do all those RMP setup etc). IIRC, in my early SEV series in I was doing
> > > > the INIT during the first command execution and based on the
> > > > recommendation moved to do the init on probe.
> > > >
> > > > Should we add a module param to control whether to do INIT on probe or
> > > > delay until the first command ?
> > >
> > > Thats a good point Brijesh. I've only been testing this with SEV and
> > > ES so haven't noticed that long setup time. I like the idea of a
> > > module parameter to decide when to INIT, that should satisfy Sean's
> > > concern that the user doesn't know when the INIT_EX file would be read
> > > and that there is extra retry code (duplicated between sev_pci_init
> > > and all the PSP commands). I'll get started on that.
> >
> > I need a little guidance on how to proceed with this. Should I have
> > the new module parameter 'psp_init_on_probe' just disable PSP init on
> > module init if false. Or should it also disable PSP init during
> > command flow if it's true?
> >
> > I was thinking I should just have 'psp_init_on_probe' default to true,
> > and if false it stops the PSP init during sev_pci_init(). If I add the
> > second change that seems like it changes the ABI. Thoughts?
>
> What about doing the INIT when we load the KVM module? Does that
> resolve all of these problems? By the time we load the KVM module, we
> know that the file system is up, which is the original problem we were
> trying to solve. And the KVM module is most likely loaded before we
> run the first guest.

KVM can be compiled as Y as well right? Then KVM module init is still too early.

Marc Orr Nov. 12, 2021, 6:28 p.m. UTC | #12

On Fri, Nov 12, 2021 at 9:49 AM Peter Gonda <[e-mail]> wrote:
>
> On Fri, Nov 12, 2021 at 10:46 AM Marc Orr <[e-mail]> wrote:
> >
> > On Fri, Nov 12, 2021 at 8:55 AM Peter Gonda <[e-mail]> wrote:
> > >
> > > On Wed, Nov 10, 2021 at 8:32 AM Peter Gonda <[e-mail]> wrote:
> > > >
> > > > On Tue, Nov 9, 2021 at 3:20 PM Brijesh Singh <[e-mail]> wrote:
> > > > >
> > > > >
> > > > >
> > > > > On 11/9/21 2:46 PM, Peter Gonda wrote:
> > > > > > On Tue, Nov 9, 2021 at 1:26 PM Sean Christopherson <[e-mail]> wrote:
> > > > > >>
> > > > > >> On Tue, Nov 09, 2021, Peter Gonda wrote:
> > > > > >>> On Tue, Nov 9, 2021 at 10:21 AM Sean Christopherson <[e-mail]> wrote:
> > > > > >>>> There's no need for this to be a function pointer, and the duplicate code can be
> > > > > >>>> consolidated.
> > > > > >>>>
> > > > > >>>> static int sev_do_init_locked(int cmd, void *data, int *error)
> > > > > >>>> {
> > > > > >>>>          if (sev_es_tmr) {
> > > > > >>>>                  /*
> > > > > >>>>                   * Do not include the encryption mask on the physical
> > > > > >>>>                   * address of the TMR (firmware should clear it anyway).
> > > > > >>>>                   */
> > > > > >>>>                  data.flags |= SEV_INIT_FLAGS_SEV_ES;
> > > > > >>>>                  data.tmr_address = __pa(sev_es_tmr);
> > > > > >>>>                  data.tmr_len = SEV_ES_TMR_SIZE;
> > > > > >>>>          }
> > > > > >>>>          return __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> > > > > >>>> }
> > > > > >>>>
> > > > > >>>> static int __sev_init_locked(int *error)
> > > > > >>>> {
> > > > > >>>>          struct sev_data_init data;
> > > > > >>>>
> > > > > >>>>          memset(&data, 0, sizeof(data));
> > > > > >>>>          return sev_do_init_locked(cmd, &data, error);
> > > > > >>>> }
> > > > > >>>>
> > > > > >>>> static int __sev_init_ex_locked(int *error)
> > > > > >>>> {
> > > > > >>>>          struct sev_data_init_ex data;
> > > > > >>>>
> > > > > >>>>          memset(&data, 0, sizeof(data));
> > > > > >>>>          data.length = sizeof(data);
> > > > > >>>>          data.nv_address = __psp_pa(sev_init_ex_nv_address);
> > > > > >>>>          data.nv_len = NV_LENGTH;
> > > > > >>>>          return sev_do_init_locked(SEV_CMD_INIT_EX, &data, error);
> > > > > >>>> }
> > > > > >>>
> > > > > >>> I am missing how this removes the duplication of the retry code,
> > > > > >>> parameter checking, and other error checking code.. With what you have
> > > > > >>> typed out I would assume I still need to function pointer between
> > > > > >>> __sev_init_ex_locked and __sev_init_locked. Can you please elaborate
> > > > > >>> here?
> > > > > >>
> > > > > >> Hmm.  Ah, I got distracted between the original thought, the realization that
> > > > > >> the two commands used different structs, and typing up the above.
> > > > > >>
> > > > > >>> Also is there some reason the function pointer is not acceptable?
> > > > > >>
> > > > > >> It's not unacceptable, it would just be nice to avoid, assuming the alternative
> > > > > >> is cleaner.  But I don't think any alternative is cleaner, since as you pointed
> > > > > >> out the above is a half-baked thought.
> > > > > >
> > > > > > OK I'll leave as is.
> > > > > >
> > > > > >>
> > > > > >>>>> +     rc = init_function(error);
> > > > > >>>>>        if (rc && *error == SEV_RET_SECURE_DATA_INVALID) {
> > > > > >>>>>                /*
> > > > > >>>>>                 * INIT command returned an integrity check failure
> > > > > >>>>> @@ -286,8 +423,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_notice(sev->dev, "SEV: retrying INIT command");
> > > > > >>>>> +             rc = init_function(error);
> > > > > >>>>
> > > > > >>>> The above comment says "persistent state has been erased", but __sev_do_cmd_locked()
> > > > > >>>> only writes back to the file if a relevant command was successful, which means
> > > > > >>>> that rereading the userspace file in __sev_init_ex_locked() will retry INIT_EX
> > > > > >>>> with the same garbage data.
> > > > > >>>
> > > > > >>> Ack my mistake, that comment is stale. I will update it so its correct
> > > > > >>> for the INIT and INIT_EX flows.
> > > > > >>>>
> > > > > >>>> IMO, the behavior should be to read the file on load and then use the kernel buffer
> > > > > >>>> without ever reloading (unless this is built as a module and is unloaded and reloaded).
> > > > > >>>> The writeback then becomes opportunistic in the sense that if it fails for some reason,
> > > > > >>>> the kernel's internal state isn't blasted away.
> > > > > >>>
> > > > > >>> One issue here is that the file read can fail on load so we use the
> > > > > >>> late retry to guarantee we can read the file.
> > > > > >>
> > > > > >> But why continue loading if reading the file fails on load?
> > > > > >>
> > > > > >>> The other point seems like preference. Users may wish to shutdown the PSP FW,
> > > > > >>> load a new file, and INIT_EX again with that new data. Why should we preclude
> > > > > >>> them from that functionality?
> > > > > >>
> > > > > >> I don't think we should preclude that functionality, but it needs to be explicitly
> > > > > >> tied to a userspace action, e.g. either on module load or on writing the param to
> > > > > >> change the path.  If the latter is allowed, then it needs to be denied if the PSP
> > > > > >> is initialized, otherwise the kernel will be in a non-coherent state and AFAICT
> > > > > >> userspace will have a heck of a time even understanding what state has been used
> > > > > >> to initialize the PSP.
> > > > > >
> > > > > > If this driver is builtin the filesystem will be unavailable during
> > > > > > __init. Using the existing retries already built into
> > > > > > sev_platform_init() also the file to be read once userspace is
> > > > > > running, meaning the file system is usable. As I tried to explain in
> > > > > > the commit message. We could remove the sev_platform_init call during
> > > > > > sev_pci_init since this only actually needs to be initialized when the
> > > > > > first command requiring it is issues (either reading some keys/certs
> > > > > > from the PSP or launching an SEV guest). Then userspace in both the
> > > > > > builtin and module usage would know running one of those commands
> > > > > > cause the file to be read for PSP usage. Tom any thoughts on this?
> > > > > >
> > > > >
> > > > > One thing to note is that if we do the INIT on the first command then
> > > > > the first guest launch will take a longer. The init command is not
> > > > > cheap (especially with the SNP, it may take a longer because it has to
> > > > > do all those RMP setup etc). IIRC, in my early SEV series in I was doing
> > > > > the INIT during the first command execution and based on the
> > > > > recommendation moved to do the init on probe.
> > > > >
> > > > > Should we add a module param to control whether to do INIT on probe or
> > > > > delay until the first command ?
> > > >
> > > > Thats a good point Brijesh. I've only been testing this with SEV and
> > > > ES so haven't noticed that long setup time. I like the idea of a
> > > > module parameter to decide when to INIT, that should satisfy Sean's
> > > > concern that the user doesn't know when the INIT_EX file would be read
> > > > and that there is extra retry code (duplicated between sev_pci_init
> > > > and all the PSP commands). I'll get started on that.
> > >
> > > I need a little guidance on how to proceed with this. Should I have
> > > the new module parameter 'psp_init_on_probe' just disable PSP init on
> > > module init if false. Or should it also disable PSP init during
> > > command flow if it's true?
> > >
> > > I was thinking I should just have 'psp_init_on_probe' default to true,
> > > and if false it stops the PSP init during sev_pci_init(). If I add the
> > > second change that seems like it changes the ABI. Thoughts?
> >
> > What about doing the INIT when we load the KVM module? Does that
> > resolve all of these problems? By the time we load the KVM module, we
> > know that the file system is up, which is the original problem we were
> > trying to solve. And the KVM module is most likely loaded before we
> > run the first guest.
>
> KVM can be compiled as Y as well right? Then KVM module init is still too early.

I think even with KVM built in, it's guaranteed to load after the file system:

* KVM is loaded using `module_init()` (e.g., kvm-amd `module_init()` [1]).
* `module_init()` is defined as `__initcall()` [2].
* `__initcall()` is defined as `device_initcall()` [3].
* Finally, looking at [3] and scrolling up a few lines,
`device_init_call()`'s appear to happen after the file system init
calls.

[1] https://elixir.bootlin.com/linux/latest/source/arch/x86/kvm/svm/svm.c#L4673
[2] https://elixir.bootlin.com/linux/latest/source/include/linux/module.h#L88
[3] https://elixir.bootlin.com/linux/latest/source/include/linux/init.h#L296

Brijesh Singh Nov. 12, 2021, 11:39 p.m. UTC | #13

On 11/12/21 10:55 AM, Peter Gonda wrote:
> On Wed, Nov 10, 2021 at 8:32 AM Peter Gonda <[e-mail]> wrote:
>> On Tue, Nov 9, 2021 at 3:20 PM Brijesh Singh <[e-mail]> wrote:
>>>
>>>
>>> On 11/9/21 2:46 PM, Peter Gonda wrote:
>>>> On Tue, Nov 9, 2021 at 1:26 PM Sean Christopherson <[e-mail]> wrote:
>>>>> On Tue, Nov 09, 2021, Peter Gonda wrote:
>>>>>> On Tue, Nov 9, 2021 at 10:21 AM Sean Christopherson <[e-mail]> wrote:
>>>>>>> There's no need for this to be a function pointer, and the duplicate code can be
>>>>>>> consolidated.
>>>>>>>
>>>>>>> static int sev_do_init_locked(int cmd, void *data, int *error)
>>>>>>> {
>>>>>>>          if (sev_es_tmr) {
>>>>>>>                  /*
>>>>>>>                   * Do not include the encryption mask on the physical
>>>>>>>                   * address of the TMR (firmware should clear it anyway).
>>>>>>>                   */
>>>>>>>                  data.flags |= SEV_INIT_FLAGS_SEV_ES;
>>>>>>>                  data.tmr_address = __pa(sev_es_tmr);
>>>>>>>                  data.tmr_len = SEV_ES_TMR_SIZE;
>>>>>>>          }
>>>>>>>          return __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
>>>>>>> }
>>>>>>>
>>>>>>> static int __sev_init_locked(int *error)
>>>>>>> {
>>>>>>>          struct sev_data_init data;
>>>>>>>
>>>>>>>          memset(&data, 0, sizeof(data));
>>>>>>>          return sev_do_init_locked(cmd, &data, error);
>>>>>>> }
>>>>>>>
>>>>>>> static int __sev_init_ex_locked(int *error)
>>>>>>> {
>>>>>>>          struct sev_data_init_ex data;
>>>>>>>
>>>>>>>          memset(&data, 0, sizeof(data));
>>>>>>>          data.length = sizeof(data);
>>>>>>>          data.nv_address = __psp_pa(sev_init_ex_nv_address);
>>>>>>>          data.nv_len = NV_LENGTH;
>>>>>>>          return sev_do_init_locked(SEV_CMD_INIT_EX, &data, error);
>>>>>>> }
>>>>>> I am missing how this removes the duplication of the retry code,
>>>>>> parameter checking, and other error checking code.. With what you have
>>>>>> typed out I would assume I still need to function pointer between
>>>>>> __sev_init_ex_locked and __sev_init_locked. Can you please elaborate
>>>>>> here?
>>>>> Hmm.  Ah, I got distracted between the original thought, the realization that
>>>>> the two commands used different structs, and typing up the above.
>>>>>
>>>>>> Also is there some reason the function pointer is not acceptable?
>>>>> It's not unacceptable, it would just be nice to avoid, assuming the alternative
>>>>> is cleaner.  But I don't think any alternative is cleaner, since as you pointed
>>>>> out the above is a half-baked thought.
>>>> OK I'll leave as is.
>>>>
>>>>>>>> +     rc = init_function(error);
>>>>>>>>        if (rc && *error == SEV_RET_SECURE_DATA_INVALID) {
>>>>>>>>                /*
>>>>>>>>                 * INIT command returned an integrity check failure
>>>>>>>> @@ -286,8 +423,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_notice(sev->dev, "SEV: retrying INIT command");
>>>>>>>> +             rc = init_function(error);
>>>>>>> The above comment says "persistent state has been erased", but __sev_do_cmd_locked()
>>>>>>> only writes back to the file if a relevant command was successful, which means
>>>>>>> that rereading the userspace file in __sev_init_ex_locked() will retry INIT_EX
>>>>>>> with the same garbage data.
>>>>>> Ack my mistake, that comment is stale. I will update it so its correct
>>>>>> for the INIT and INIT_EX flows.
>>>>>>> IMO, the behavior should be to read the file on load and then use the kernel buffer
>>>>>>> without ever reloading (unless this is built as a module and is unloaded and reloaded).
>>>>>>> The writeback then becomes opportunistic in the sense that if it fails for some reason,
>>>>>>> the kernel's internal state isn't blasted away.
>>>>>> One issue here is that the file read can fail on load so we use the
>>>>>> late retry to guarantee we can read the file.
>>>>> But why continue loading if reading the file fails on load?
>>>>>
>>>>>> The other point seems like preference. Users may wish to shutdown the PSP FW,
>>>>>> load a new file, and INIT_EX again with that new data. Why should we preclude
>>>>>> them from that functionality?
>>>>> I don't think we should preclude that functionality, but it needs to be explicitly
>>>>> tied to a userspace action, e.g. either on module load or on writing the param to
>>>>> change the path.  If the latter is allowed, then it needs to be denied if the PSP
>>>>> is initialized, otherwise the kernel will be in a non-coherent state and AFAICT
>>>>> userspace will have a heck of a time even understanding what state has been used
>>>>> to initialize the PSP.
>>>> If this driver is builtin the filesystem will be unavailable during
>>>> __init. Using the existing retries already built into
>>>> sev_platform_init() also the file to be read once userspace is
>>>> running, meaning the file system is usable. As I tried to explain in
>>>> the commit message. We could remove the sev_platform_init call during
>>>> sev_pci_init since this only actually needs to be initialized when the
>>>> first command requiring it is issues (either reading some keys/certs
>>>> from the PSP or launching an SEV guest). Then userspace in both the
>>>> builtin and module usage would know running one of those commands
>>>> cause the file to be read for PSP usage. Tom any thoughts on this?
>>>>
>>> One thing to note is that if we do the INIT on the first command then
>>> the first guest launch will take a longer. The init command is not
>>> cheap (especially with the SNP, it may take a longer because it has to
>>> do all those RMP setup etc). IIRC, in my early SEV series in I was doing
>>> the INIT during the first command execution and based on the
>>> recommendation moved to do the init on probe.
>>>
>>> Should we add a module param to control whether to do INIT on probe or
>>> delay until the first command ?
>> Thats a good point Brijesh. I've only been testing this with SEV and
>> ES so haven't noticed that long setup time. I like the idea of a
>> module parameter to decide when to INIT, that should satisfy Sean's
>> concern that the user doesn't know when the INIT_EX file would be read
>> and that there is extra retry code (duplicated between sev_pci_init
>> and all the PSP commands). I'll get started on that.
> I need a little guidance on how to proceed with this. Should I have
> the new module parameter 'psp_init_on_probe' just disable PSP init on
> module init if false. Or should it also disable PSP init during
> command flow if it's true?
>
> I was thinking I should just have 'psp_init_on_probe' default to true,
> and if false it stops the PSP init during sev_pci_init(). If I add the
> second change that seems like it changes the ABI. Thoughts?
>
Good point that a module params may break the ABI. How about if we add a
new ioctl that can be used to initialize the SEV_INIT_EX. The ioctl
implementation will be similar to the PLATFORM_RESET; it will shutdown
the firmware then call INIT_EX. A platform provisioning tool may use ioctl.

-Brijesh

Peter Gonda Nov. 12, 2021, 11:44 p.m. UTC | #14

On Fri, Nov 12, 2021 at 4:39 PM Brijesh Singh <[e-mail]> wrote:
>
>
> On 11/12/21 10:55 AM, Peter Gonda wrote:
> > On Wed, Nov 10, 2021 at 8:32 AM Peter Gonda <[e-mail]> wrote:
> >> On Tue, Nov 9, 2021 at 3:20 PM Brijesh Singh <[e-mail]> wrote:
> >>>
> >>>
> >>> On 11/9/21 2:46 PM, Peter Gonda wrote:
> >>>> On Tue, Nov 9, 2021 at 1:26 PM Sean Christopherson <[e-mail]> wrote:
> >>>>> On Tue, Nov 09, 2021, Peter Gonda wrote:
> >>>>>> On Tue, Nov 9, 2021 at 10:21 AM Sean Christopherson <[e-mail]> wrote:
> >>>>>>> There's no need for this to be a function pointer, and the duplicate code can be
> >>>>>>> consolidated.
> >>>>>>>
> >>>>>>> static int sev_do_init_locked(int cmd, void *data, int *error)
> >>>>>>> {
> >>>>>>>          if (sev_es_tmr) {
> >>>>>>>                  /*
> >>>>>>>                   * Do not include the encryption mask on the physical
> >>>>>>>                   * address of the TMR (firmware should clear it anyway).
> >>>>>>>                   */
> >>>>>>>                  data.flags |= SEV_INIT_FLAGS_SEV_ES;
> >>>>>>>                  data.tmr_address = __pa(sev_es_tmr);
> >>>>>>>                  data.tmr_len = SEV_ES_TMR_SIZE;
> >>>>>>>          }
> >>>>>>>          return __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> >>>>>>> }
> >>>>>>>
> >>>>>>> static int __sev_init_locked(int *error)
> >>>>>>> {
> >>>>>>>          struct sev_data_init data;
> >>>>>>>
> >>>>>>>          memset(&data, 0, sizeof(data));
> >>>>>>>          return sev_do_init_locked(cmd, &data, error);
> >>>>>>> }
> >>>>>>>
> >>>>>>> static int __sev_init_ex_locked(int *error)
> >>>>>>> {
> >>>>>>>          struct sev_data_init_ex data;
> >>>>>>>
> >>>>>>>          memset(&data, 0, sizeof(data));
> >>>>>>>          data.length = sizeof(data);
> >>>>>>>          data.nv_address = __psp_pa(sev_init_ex_nv_address);
> >>>>>>>          data.nv_len = NV_LENGTH;
> >>>>>>>          return sev_do_init_locked(SEV_CMD_INIT_EX, &data, error);
> >>>>>>> }
> >>>>>> I am missing how this removes the duplication of the retry code,
> >>>>>> parameter checking, and other error checking code.. With what you have
> >>>>>> typed out I would assume I still need to function pointer between
> >>>>>> __sev_init_ex_locked and __sev_init_locked. Can you please elaborate
> >>>>>> here?
> >>>>> Hmm.  Ah, I got distracted between the original thought, the realization that
> >>>>> the two commands used different structs, and typing up the above.
> >>>>>
> >>>>>> Also is there some reason the function pointer is not acceptable?
> >>>>> It's not unacceptable, it would just be nice to avoid, assuming the alternative
> >>>>> is cleaner.  But I don't think any alternative is cleaner, since as you pointed
> >>>>> out the above is a half-baked thought.
> >>>> OK I'll leave as is.
> >>>>
> >>>>>>>> +     rc = init_function(error);
> >>>>>>>>        if (rc && *error == SEV_RET_SECURE_DATA_INVALID) {
> >>>>>>>>                /*
> >>>>>>>>                 * INIT command returned an integrity check failure
> >>>>>>>> @@ -286,8 +423,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_notice(sev->dev, "SEV: retrying INIT command");
> >>>>>>>> +             rc = init_function(error);
> >>>>>>> The above comment says "persistent state has been erased", but __sev_do_cmd_locked()
> >>>>>>> only writes back to the file if a relevant command was successful, which means
> >>>>>>> that rereading the userspace file in __sev_init_ex_locked() will retry INIT_EX
> >>>>>>> with the same garbage data.
> >>>>>> Ack my mistake, that comment is stale. I will update it so its correct
> >>>>>> for the INIT and INIT_EX flows.
> >>>>>>> IMO, the behavior should be to read the file on load and then use the kernel buffer
> >>>>>>> without ever reloading (unless this is built as a module and is unloaded and reloaded).
> >>>>>>> The writeback then becomes opportunistic in the sense that if it fails for some reason,
> >>>>>>> the kernel's internal state isn't blasted away.
> >>>>>> One issue here is that the file read can fail on load so we use the
> >>>>>> late retry to guarantee we can read the file.
> >>>>> But why continue loading if reading the file fails on load?
> >>>>>
> >>>>>> The other point seems like preference. Users may wish to shutdown the PSP FW,
> >>>>>> load a new file, and INIT_EX again with that new data. Why should we preclude
> >>>>>> them from that functionality?
> >>>>> I don't think we should preclude that functionality, but it needs to be explicitly
> >>>>> tied to a userspace action, e.g. either on module load or on writing the param to
> >>>>> change the path.  If the latter is allowed, then it needs to be denied if the PSP
> >>>>> is initialized, otherwise the kernel will be in a non-coherent state and AFAICT
> >>>>> userspace will have a heck of a time even understanding what state has been used
> >>>>> to initialize the PSP.
> >>>> If this driver is builtin the filesystem will be unavailable during
> >>>> __init. Using the existing retries already built into
> >>>> sev_platform_init() also the file to be read once userspace is
> >>>> running, meaning the file system is usable. As I tried to explain in
> >>>> the commit message. We could remove the sev_platform_init call during
> >>>> sev_pci_init since this only actually needs to be initialized when the
> >>>> first command requiring it is issues (either reading some keys/certs
> >>>> from the PSP or launching an SEV guest). Then userspace in both the
> >>>> builtin and module usage would know running one of those commands
> >>>> cause the file to be read for PSP usage. Tom any thoughts on this?
> >>>>
> >>> One thing to note is that if we do the INIT on the first command then
> >>> the first guest launch will take a longer. The init command is not
> >>> cheap (especially with the SNP, it may take a longer because it has to
> >>> do all those RMP setup etc). IIRC, in my early SEV series in I was doing
> >>> the INIT during the first command execution and based on the
> >>> recommendation moved to do the init on probe.
> >>>
> >>> Should we add a module param to control whether to do INIT on probe or
> >>> delay until the first command ?
> >> Thats a good point Brijesh. I've only been testing this with SEV and
> >> ES so haven't noticed that long setup time. I like the idea of a
> >> module parameter to decide when to INIT, that should satisfy Sean's
> >> concern that the user doesn't know when the INIT_EX file would be read
> >> and that there is extra retry code (duplicated between sev_pci_init
> >> and all the PSP commands). I'll get started on that.
> > I need a little guidance on how to proceed with this. Should I have
> > the new module parameter 'psp_init_on_probe' just disable PSP init on
> > module init if false. Or should it also disable PSP init during
> > command flow if it's true?
> >
> > I was thinking I should just have 'psp_init_on_probe' default to true,
> > and if false it stops the PSP init during sev_pci_init(). If I add the
> > second change that seems like it changes the ABI. Thoughts?
> >
> Good point that a module params may break the ABI. How about if we add a
> new ioctl that can be used to initialize the SEV_INIT_EX. The ioctl
> implementation will be similar to the PLATFORM_RESET; it will shutdown
> the firmware then call INIT_EX. A platform provisioning tool may use ioctl.

Would just a 'skip_psp_init_on_probe' parameter be simpler. We default
to false but if users set it, we can skip that init attempt in
sev_pci_init(). The init attempts on all other commands that require
the INIT state would then provide users with INIT_EX functionality.
They would also know exactly when INIT or INIT_EX would be attempted
based on the parameter.

Otherwise a new ioctl sounds reasonable.

>
> -Brijesh
>

Brijesh Singh Nov. 12, 2021, 11:50 p.m. UTC | #15

On 11/12/21 5:44 PM, Peter Gonda wrote:
> On Fri, Nov 12, 2021 at 4:39 PM Brijesh Singh <[e-mail]> wrote:
>>
>> On 11/12/21 10:55 AM, Peter Gonda wrote:
>>> On Wed, Nov 10, 2021 at 8:32 AM Peter Gonda <[e-mail]> wrote:
>>>> On Tue, Nov 9, 2021 at 3:20 PM Brijesh Singh <[e-mail]> wrote:
>>>>>
>>>>> On 11/9/21 2:46 PM, Peter Gonda wrote:
>>>>>> On Tue, Nov 9, 2021 at 1:26 PM Sean Christopherson <[e-mail]> wrote:
>>>>>>> On Tue, Nov 09, 2021, Peter Gonda wrote:
>>>>>>>> On Tue, Nov 9, 2021 at 10:21 AM Sean Christopherson <[e-mail]> wrote:
>>>>>>>>> There's no need for this to be a function pointer, and the duplicate code can be
>>>>>>>>> consolidated.
>>>>>>>>>
>>>>>>>>> static int sev_do_init_locked(int cmd, void *data, int *error)
>>>>>>>>> {
>>>>>>>>>          if (sev_es_tmr) {
>>>>>>>>>                  /*
>>>>>>>>>                   * Do not include the encryption mask on the physical
>>>>>>>>>                   * address of the TMR (firmware should clear it anyway).
>>>>>>>>>                   */
>>>>>>>>>                  data.flags |= SEV_INIT_FLAGS_SEV_ES;
>>>>>>>>>                  data.tmr_address = __pa(sev_es_tmr);
>>>>>>>>>                  data.tmr_len = SEV_ES_TMR_SIZE;
>>>>>>>>>          }
>>>>>>>>>          return __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> static int __sev_init_locked(int *error)
>>>>>>>>> {
>>>>>>>>>          struct sev_data_init data;
>>>>>>>>>
>>>>>>>>>          memset(&data, 0, sizeof(data));
>>>>>>>>>          return sev_do_init_locked(cmd, &data, error);
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> static int __sev_init_ex_locked(int *error)
>>>>>>>>> {
>>>>>>>>>          struct sev_data_init_ex data;
>>>>>>>>>
>>>>>>>>>          memset(&data, 0, sizeof(data));
>>>>>>>>>          data.length = sizeof(data);
>>>>>>>>>          data.nv_address = __psp_pa(sev_init_ex_nv_address);
>>>>>>>>>          data.nv_len = NV_LENGTH;
>>>>>>>>>          return sev_do_init_locked(SEV_CMD_INIT_EX, &data, error);
>>>>>>>>> }
>>>>>>>> I am missing how this removes the duplication of the retry code,
>>>>>>>> parameter checking, and other error checking code.. With what you have
>>>>>>>> typed out I would assume I still need to function pointer between
>>>>>>>> __sev_init_ex_locked and __sev_init_locked. Can you please elaborate
>>>>>>>> here?
>>>>>>> Hmm.  Ah, I got distracted between the original thought, the realization that
>>>>>>> the two commands used different structs, and typing up the above.
>>>>>>>
>>>>>>>> Also is there some reason the function pointer is not acceptable?
>>>>>>> It's not unacceptable, it would just be nice to avoid, assuming the alternative
>>>>>>> is cleaner.  But I don't think any alternative is cleaner, since as you pointed
>>>>>>> out the above is a half-baked thought.
>>>>>> OK I'll leave as is.
>>>>>>
>>>>>>>>>> +     rc = init_function(error);
>>>>>>>>>>        if (rc && *error == SEV_RET_SECURE_DATA_INVALID) {
>>>>>>>>>>                /*
>>>>>>>>>>                 * INIT command returned an integrity check failure
>>>>>>>>>> @@ -286,8 +423,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_notice(sev->dev, "SEV: retrying INIT command");
>>>>>>>>>> +             rc = init_function(error);
>>>>>>>>> The above comment says "persistent state has been erased", but __sev_do_cmd_locked()
>>>>>>>>> only writes back to the file if a relevant command was successful, which means
>>>>>>>>> that rereading the userspace file in __sev_init_ex_locked() will retry INIT_EX
>>>>>>>>> with the same garbage data.
>>>>>>>> Ack my mistake, that comment is stale. I will update it so its correct
>>>>>>>> for the INIT and INIT_EX flows.
>>>>>>>>> IMO, the behavior should be to read the file on load and then use the kernel buffer
>>>>>>>>> without ever reloading (unless this is built as a module and is unloaded and reloaded).
>>>>>>>>> The writeback then becomes opportunistic in the sense that if it fails for some reason,
>>>>>>>>> the kernel's internal state isn't blasted away.
>>>>>>>> One issue here is that the file read can fail on load so we use the
>>>>>>>> late retry to guarantee we can read the file.
>>>>>>> But why continue loading if reading the file fails on load?
>>>>>>>
>>>>>>>> The other point seems like preference. Users may wish to shutdown the PSP FW,
>>>>>>>> load a new file, and INIT_EX again with that new data. Why should we preclude
>>>>>>>> them from that functionality?
>>>>>>> I don't think we should preclude that functionality, but it needs to be explicitly
>>>>>>> tied to a userspace action, e.g. either on module load or on writing the param to
>>>>>>> change the path.  If the latter is allowed, then it needs to be denied if the PSP
>>>>>>> is initialized, otherwise the kernel will be in a non-coherent state and AFAICT
>>>>>>> userspace will have a heck of a time even understanding what state has been used
>>>>>>> to initialize the PSP.
>>>>>> If this driver is builtin the filesystem will be unavailable during
>>>>>> __init. Using the existing retries already built into
>>>>>> sev_platform_init() also the file to be read once userspace is
>>>>>> running, meaning the file system is usable. As I tried to explain in
>>>>>> the commit message. We could remove the sev_platform_init call during
>>>>>> sev_pci_init since this only actually needs to be initialized when the
>>>>>> first command requiring it is issues (either reading some keys/certs
>>>>>> from the PSP or launching an SEV guest). Then userspace in both the
>>>>>> builtin and module usage would know running one of those commands
>>>>>> cause the file to be read for PSP usage. Tom any thoughts on this?
>>>>>>
>>>>> One thing to note is that if we do the INIT on the first command then
>>>>> the first guest launch will take a longer. The init command is not
>>>>> cheap (especially with the SNP, it may take a longer because it has to
>>>>> do all those RMP setup etc). IIRC, in my early SEV series in I was doing
>>>>> the INIT during the first command execution and based on the
>>>>> recommendation moved to do the init on probe.
>>>>>
>>>>> Should we add a module param to control whether to do INIT on probe or
>>>>> delay until the first command ?
>>>> Thats a good point Brijesh. I've only been testing this with SEV and
>>>> ES so haven't noticed that long setup time. I like the idea of a
>>>> module parameter to decide when to INIT, that should satisfy Sean's
>>>> concern that the user doesn't know when the INIT_EX file would be read
>>>> and that there is extra retry code (duplicated between sev_pci_init
>>>> and all the PSP commands). I'll get started on that.
>>> I need a little guidance on how to proceed with this. Should I have
>>> the new module parameter 'psp_init_on_probe' just disable PSP init on
>>> module init if false. Or should it also disable PSP init during
>>> command flow if it's true?
>>>
>>> I was thinking I should just have 'psp_init_on_probe' default to true,
>>> and if false it stops the PSP init during sev_pci_init(). If I add the
>>> second change that seems like it changes the ABI. Thoughts?
>>>
>> Good point that a module params may break the ABI. How about if we add a
>> new ioctl that can be used to initialize the SEV_INIT_EX. The ioctl
>> implementation will be similar to the PLATFORM_RESET; it will shutdown
>> the firmware then call INIT_EX. A platform provisioning tool may use ioctl.
> Would just a 'skip_psp_init_on_probe' parameter be simpler. We default
> to false but if users set it, we can skip that init attempt in
> sev_pci_init(). The init attempts on all other commands that require
> the INIT state would then provide users with INIT_EX functionality.
> They would also know exactly when INIT or INIT_EX would be attempted
> based on the parameter.

Yes, I think that option is also acceptable. Because we are requiring
the user to explicitly say that it does not want to INIT on boot.


>
> Otherwise a new ioctl sounds reasonable.
>> -Brijesh
>>

Peter Gonda Nov. 15, 2021, 5:42 p.m. UTC | #16

On Fri, Nov 12, 2021 at 4:50 PM Brijesh Singh <[e-mail]> wrote:
>
>
> On 11/12/21 5:44 PM, Peter Gonda wrote:
> > On Fri, Nov 12, 2021 at 4:39 PM Brijesh Singh <[e-mail]> wrote:
> >>
> >> On 11/12/21 10:55 AM, Peter Gonda wrote:
> >>> On Wed, Nov 10, 2021 at 8:32 AM Peter Gonda <[e-mail]> wrote:
> >>>> On Tue, Nov 9, 2021 at 3:20 PM Brijesh Singh <[e-mail]> wrote:
> >>>>>
> >>>>> On 11/9/21 2:46 PM, Peter Gonda wrote:
> >>>>>> On Tue, Nov 9, 2021 at 1:26 PM Sean Christopherson <[e-mail]> wrote:
> >>>>>>> On Tue, Nov 09, 2021, Peter Gonda wrote:
> >>>>>>>> On Tue, Nov 9, 2021 at 10:21 AM Sean Christopherson <[e-mail]> wrote:
> >>>>>>>>> There's no need for this to be a function pointer, and the duplicate code can be
> >>>>>>>>> consolidated.
> >>>>>>>>>
> >>>>>>>>> static int sev_do_init_locked(int cmd, void *data, int *error)
> >>>>>>>>> {
> >>>>>>>>>          if (sev_es_tmr) {
> >>>>>>>>>                  /*
> >>>>>>>>>                   * Do not include the encryption mask on the physical
> >>>>>>>>>                   * address of the TMR (firmware should clear it anyway).
> >>>>>>>>>                   */
> >>>>>>>>>                  data.flags |= SEV_INIT_FLAGS_SEV_ES;
> >>>>>>>>>                  data.tmr_address = __pa(sev_es_tmr);
> >>>>>>>>>                  data.tmr_len = SEV_ES_TMR_SIZE;
> >>>>>>>>>          }
> >>>>>>>>>          return __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
> >>>>>>>>> }
> >>>>>>>>>
> >>>>>>>>> static int __sev_init_locked(int *error)
> >>>>>>>>> {
> >>>>>>>>>          struct sev_data_init data;
> >>>>>>>>>
> >>>>>>>>>          memset(&data, 0, sizeof(data));
> >>>>>>>>>          return sev_do_init_locked(cmd, &data, error);
> >>>>>>>>> }
> >>>>>>>>>
> >>>>>>>>> static int __sev_init_ex_locked(int *error)
> >>>>>>>>> {
> >>>>>>>>>          struct sev_data_init_ex data;
> >>>>>>>>>
> >>>>>>>>>          memset(&data, 0, sizeof(data));
> >>>>>>>>>          data.length = sizeof(data);
> >>>>>>>>>          data.nv_address = __psp_pa(sev_init_ex_nv_address);
> >>>>>>>>>          data.nv_len = NV_LENGTH;
> >>>>>>>>>          return sev_do_init_locked(SEV_CMD_INIT_EX, &data, error);
> >>>>>>>>> }
> >>>>>>>> I am missing how this removes the duplication of the retry code,
> >>>>>>>> parameter checking, and other error checking code.. With what you have
> >>>>>>>> typed out I would assume I still need to function pointer between
> >>>>>>>> __sev_init_ex_locked and __sev_init_locked. Can you please elaborate
> >>>>>>>> here?
> >>>>>>> Hmm.  Ah, I got distracted between the original thought, the realization that
> >>>>>>> the two commands used different structs, and typing up the above.
> >>>>>>>
> >>>>>>>> Also is there some reason the function pointer is not acceptable?
> >>>>>>> It's not unacceptable, it would just be nice to avoid, assuming the alternative
> >>>>>>> is cleaner.  But I don't think any alternative is cleaner, since as you pointed
> >>>>>>> out the above is a half-baked thought.
> >>>>>> OK I'll leave as is.
> >>>>>>
> >>>>>>>>>> +     rc = init_function(error);
> >>>>>>>>>>        if (rc && *error == SEV_RET_SECURE_DATA_INVALID) {
> >>>>>>>>>>                /*
> >>>>>>>>>>                 * INIT command returned an integrity check failure
> >>>>>>>>>> @@ -286,8 +423,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_notice(sev->dev, "SEV: retrying INIT command");
> >>>>>>>>>> +             rc = init_function(error);
> >>>>>>>>> The above comment says "persistent state has been erased", but __sev_do_cmd_locked()
> >>>>>>>>> only writes back to the file if a relevant command was successful, which means
> >>>>>>>>> that rereading the userspace file in __sev_init_ex_locked() will retry INIT_EX
> >>>>>>>>> with the same garbage data.
> >>>>>>>> Ack my mistake, that comment is stale. I will update it so its correct
> >>>>>>>> for the INIT and INIT_EX flows.
> >>>>>>>>> IMO, the behavior should be to read the file on load and then use the kernel buffer
> >>>>>>>>> without ever reloading (unless this is built as a module and is unloaded and reloaded).
> >>>>>>>>> The writeback then becomes opportunistic in the sense that if it fails for some reason,
> >>>>>>>>> the kernel's internal state isn't blasted away.
> >>>>>>>> One issue here is that the file read can fail on load so we use the
> >>>>>>>> late retry to guarantee we can read the file.
> >>>>>>> But why continue loading if reading the file fails on load?
> >>>>>>>
> >>>>>>>> The other point seems like preference. Users may wish to shutdown the PSP FW,
> >>>>>>>> load a new file, and INIT_EX again with that new data. Why should we preclude
> >>>>>>>> them from that functionality?
> >>>>>>> I don't think we should preclude that functionality, but it needs to be explicitly
> >>>>>>> tied to a userspace action, e.g. either on module load or on writing the param to
> >>>>>>> change the path.  If the latter is allowed, then it needs to be denied if the PSP
> >>>>>>> is initialized, otherwise the kernel will be in a non-coherent state and AFAICT
> >>>>>>> userspace will have a heck of a time even understanding what state has been used
> >>>>>>> to initialize the PSP.
> >>>>>> If this driver is builtin the filesystem will be unavailable during
> >>>>>> __init. Using the existing retries already built into
> >>>>>> sev_platform_init() also the file to be read once userspace is
> >>>>>> running, meaning the file system is usable. As I tried to explain in
> >>>>>> the commit message. We could remove the sev_platform_init call during
> >>>>>> sev_pci_init since this only actually needs to be initialized when the
> >>>>>> first command requiring it is issues (either reading some keys/certs
> >>>>>> from the PSP or launching an SEV guest). Then userspace in both the
> >>>>>> builtin and module usage would know running one of those commands
> >>>>>> cause the file to be read for PSP usage. Tom any thoughts on this?
> >>>>>>
> >>>>> One thing to note is that if we do the INIT on the first command then
> >>>>> the first guest launch will take a longer. The init command is not
> >>>>> cheap (especially with the SNP, it may take a longer because it has to
> >>>>> do all those RMP setup etc). IIRC, in my early SEV series in I was doing
> >>>>> the INIT during the first command execution and based on the
> >>>>> recommendation moved to do the init on probe.
> >>>>>
> >>>>> Should we add a module param to control whether to do INIT on probe or
> >>>>> delay until the first command ?
> >>>> Thats a good point Brijesh. I've only been testing this with SEV and
> >>>> ES so haven't noticed that long setup time. I like the idea of a
> >>>> module parameter to decide when to INIT, that should satisfy Sean's
> >>>> concern that the user doesn't know when the INIT_EX file would be read
> >>>> and that there is extra retry code (duplicated between sev_pci_init
> >>>> and all the PSP commands). I'll get started on that.
> >>> I need a little guidance on how to proceed with this. Should I have
> >>> the new module parameter 'psp_init_on_probe' just disable PSP init on
> >>> module init if false. Or should it also disable PSP init during
> >>> command flow if it's true?
> >>>
> >>> I was thinking I should just have 'psp_init_on_probe' default to true,
> >>> and if false it stops the PSP init during sev_pci_init(). If I add the
> >>> second change that seems like it changes the ABI. Thoughts?
> >>>
> >> Good point that a module params may break the ABI. How about if we add a
> >> new ioctl that can be used to initialize the SEV_INIT_EX. The ioctl
> >> implementation will be similar to the PLATFORM_RESET; it will shutdown
> >> the firmware then call INIT_EX. A platform provisioning tool may use ioctl.
> > Would just a 'skip_psp_init_on_probe' parameter be simpler. We default
> > to false but if users set it, we can skip that init attempt in
> > sev_pci_init(). The init attempts on all other commands that require
> > the INIT state would then provide users with INIT_EX functionality.
> > They would also know exactly when INIT or INIT_EX would be attempted
> > based on the parameter.
>
> Yes, I think that option is also acceptable. Because we are requiring
> the user to explicitly say that it does not want to INIT on boot.

OK sent out a V4 with this mode param approach.

>
>
> >
> > Otherwise a new ioctl sounds reasonable.
> >> -Brijesh
> >>

Patch

diff --git a/Documentation/virt/kvm/amd-memory-encryption.rst b/Documentation/virt/kvm/amd-memory-encryption.rst
index 5c081c8c7164..1c6847fff304 100644
--- a/Documentation/virt/kvm/amd-memory-encryption.rst
+++ b/Documentation/virt/kvm/amd-memory-encryption.rst
@@ -85,6 +85,12 @@  guests, such as launching, running, snapshotting, migrating and decommissioning.
 The KVM_SEV_INIT command is used by the hypervisor to initialize the SEV platform
 context. In a typical workflow, this command should be the first command issued.
 
+The firmware can be initialized either by using its own non-volatile storage or
+the OS can manage the NV storage for the firmware using the module parameter
+``init_ex_path``. The file specified by ``init_ex_path`` must exist. To create
+a new NV storage file allocate the file with 32KB bytes of 0xFF as required by
+the SEV spec.
+
 Returns: 0 on success, -negative on error
 
 2. KVM_SEV_LAUNCH_START
diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index 00ca74dd7b3c..2264a0b76bee 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 * 1024)
+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;
@@ -107,6 +120,7 @@  static int sev_cmd_buffer_len(int cmd)
 {
 	switch (cmd) {
 	case SEV_CMD_INIT:			return sizeof(struct sev_data_init);
+	case SEV_CMD_INIT_EX:                   return sizeof(struct sev_data_init_ex);
 	case SEV_CMD_PLATFORM_STATUS:		return sizeof(struct sev_user_data_status);
 	case SEV_CMD_PEK_CSR:			return sizeof(struct sev_data_pek_csr);
 	case SEV_CMD_PEK_CERT_IMPORT:		return sizeof(struct sev_data_pek_cert_import);
@@ -152,6 +166,87 @@  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)) {
+		int ret = PTR_ERR(fp);
+
+		dev_err(psp_master->dev,
+			"SEV: could not open %s for read, error %d\n",
+			init_ex_path, ret);
+		return ret;
+	}
+
+	nread = kernel_read(fp, sev_init_ex_nv_address, NV_LENGTH, NULL);
+	dev_dbg(psp_master->dev, "SEV: read %ld bytes from NV file\n", nread);
+	filp_close(fp, NULL);
+
+	return 0;
+}
+
+static void sev_write_nv_memory(void)
+{
+	struct sev_device *sev = psp_master->sev_data;
+	struct file *fp;
+	loff_t offset = 0;
+	ssize_t nwrite;
+
+	if (!sev_init_ex_nv_address)
+		return;
+
+	fp = filp_open(init_ex_path, O_CREAT | O_WRONLY, 0600);
+	if (IS_ERR(fp)) {
+		dev_err(sev->dev,
+			"SEV: could not open file for write, error %d\n",
+			PTR_ERR(fp));
+		return;
+	}
+
+	nwrite = kernel_write(fp, sev_init_ex_nv_address, NV_LENGTH, &offset);
+	vfs_fsync(fp, 0);
+	filp_close(fp, NULL);
+
+	if (nwrite != NV_LENGTH) {
+		dev_err(sev->dev,
+			"SEV: failed to write %u bytes to non volatile memory area, ret %ld\n",
+			NV_LENGTH, nwrite);
+		return;
+	}
+
+	dev_dbg(sev->dev, "SEV: write successful to NV file\n");
+}
+
+static void sev_write_nv_memory_if_required(int cmd_id)
+{
+	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(_EX), 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:
+		break;
+	default:
+		return;
+	};
+
+	sev_write_nv_memory();
+}
+
 static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
 {
 	struct psp_device *psp = psp_master;
@@ -221,6 +316,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,
@@ -247,22 +344,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;
 
@@ -277,7 +394,27 @@  static int __sev_platform_init_locked(int *error)
 		data.tmr_len = SEV_ES_TMR_SIZE;
 	}
 
-	rc = __sev_do_cmd_locked(SEV_CMD_INIT, &data, error);
+	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);
+
+	if (!psp || !psp->sev_data)
+		return -ENODEV;
+
+	sev = psp->sev_data;
+
+	if (sev->state == SEV_STATE_INIT)
+		return 0;
+
+	init_function = sev_init_ex_nv_address ? __sev_init_ex_locked :
+	    __sev_init_locked;
+	rc = init_function(error);
 	if (rc && *error == SEV_RET_SECURE_DATA_INVALID) {
 		/*
 		 * INIT command returned an integrity check failure
@@ -286,8 +423,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_notice(sev->dev, "SEV: retrying INIT command");
+		rc = init_function(error);
 	}
 
 	if (rc)
@@ -303,7 +440,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)
@@ -1057,6 +1194,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)
@@ -1101,6 +1244,18 @@  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_err(sev->dev,
+				"SEV: INIT_EX NV memory allocation failed\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
 
 /**