Re: [PATCH RFC v7 25/64] crypto: ccp: Add support to initialize the AMD-SP for SEV-SNP

[Kalra, Ashish]

Hello Jarkko,

On 12/31/2022 9:32 AM, Jarkko Sakkinen wrote:
> On Wed, Dec 14, 2022 at 01:40:17PM -0600, Michael Roth wrote:
> > From: Brijesh Singh <brijesh.singh@xxxxxxx>
> >
> > Before SNP VMs can be launched, the platform must be appropriately
> > configured and initialized. Platform initialization is accomplished via
> > the SNP_INIT command. Make sure to do a WBINVD and issue DF_FLUSH
> > command to prepare for the first SNP guest launch after INIT.
> >
> > During the execution of SNP_INIT command, the firmware configures
> > and enables SNP security policy enforcement in many system components.
> > Some system components write to regions of memory reserved by early
> > x86 firmware (e.g. UEFI). Other system components write to regions
> > provided by the operation system, hypervisor, or x86 firmware.
> > Such system components can only write to HV-fixed pages or Default
> > pages. They will error when attempting to write to other page states
> > after SNP_INIT enables their SNP enforcement.
> >
> > Starting in SNP firmware v1.52, the SNP_INIT_EX command takes a list of
> > system physical address ranges to convert into the HV-fixed page states
> > during the RMP initialization. If INIT_RMP is 1, hypervisors should
> > provide all system physical address ranges that the hypervisor will
> > never assign to a guest until the next RMP re-initialization.
> > For instance, the memory that UEFI reserves should be included in the
> > range list. This allows system components that occasionally write to
> > memory (e.g. logging to UEFI reserved regions) to not fail due to
> > RMP initialization and SNP enablement.
> >
> > Co-developed-by: Ashish Kalra <ashish.kalra@xxxxxxx>
> > Signed-off-by: Ashish Kalra <ashish.kalra@xxxxxxx>
> > Signed-off-by: Brijesh Singh <brijesh.singh@xxxxxxx>
> > Signed-off-by: Michael Roth <michael.roth@xxxxxxx>
> > ---
> >   drivers/crypto/ccp/sev-dev.c | 225 +++++++++++++++++++++++++++++++++++
> >   drivers/crypto/ccp/sev-dev.h |   2 +
> >   include/linux/psp-sev.h      |  17 +++
> >   3 files changed, 244 insertions(+)
> >
> > diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> > index 9d84720a41d7..af20420bd6c2 100644
> > --- a/drivers/crypto/ccp/sev-dev.c
> > +++ b/drivers/crypto/ccp/sev-dev.c
> > @@ -26,6 +26,7 @@
> >   #include <linux/fs_struct.h>
> >   
> >   #include <asm/smp.h>
> > +#include <asm/e820/types.h>
> >   
> >   #include "psp-dev.h"
> >   #include "sev-dev.h"
> > @@ -34,6 +35,10 @@
> >   #define SEV_FW_FILE		"amd/sev.fw"
> >   #define SEV_FW_NAME_SIZE	64
> >   
> > +/* Minimum firmware version required for the SEV-SNP support */
> > +#define SNP_MIN_API_MAJOR	1
> > +#define SNP_MIN_API_MINOR	51
> > +
> >   static DEFINE_MUTEX(sev_cmd_mutex);
> >   static struct sev_misc_dev *misc_dev;
> >   
> > @@ -76,6 +81,13 @@ static void *sev_es_tmr;
> >   #define NV_LENGTH (32 * 1024)
> >   static void *sev_init_ex_buffer;
> >   
> > +/*
> > + * SEV_DATA_RANGE_LIST:
> > + *   Array containing range of pages that firmware transitions to HV-fixed
> > + *   page state.
> > + */
> > +struct sev_data_range_list *snp_range_list;
> > +
> >   static inline bool sev_version_greater_or_equal(u8 maj, u8 min)
> >   {
> >   	struct sev_device *sev = psp_master->sev_data;
> > @@ -830,6 +842,186 @@ static int sev_update_firmware(struct device *dev)
> >   	return ret;
> >   }
> >   
> > +static void snp_set_hsave_pa(void *arg)
> > +{
> > +	wrmsrl(MSR_VM_HSAVE_PA, 0);
> > +}
> > +
> > +static int snp_filter_reserved_mem_regions(struct resource *rs, void *arg)
> > +{
> > +	struct sev_data_range_list *range_list = arg;
> > +	struct sev_data_range *range = &range_list->ranges[range_list->num_elements];
> > +	size_t size;
> > +
> > +	if ((range_list->num_elements * sizeof(struct sev_data_range) +
> > +	     sizeof(struct sev_data_range_list)) > PAGE_SIZE)
> > +		return -E2BIG;
> > +
> > +	switch (rs->desc) {
> > +	case E820_TYPE_RESERVED:
> > +	case E820_TYPE_PMEM:
> > +	case E820_TYPE_ACPI:
> > +		range->base = rs->start & PAGE_MASK;
> > +		size = (rs->end + 1) - rs->start;
> > +		range->page_count = size >> PAGE_SHIFT;
> > +		range_list->num_elements++;
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int __sev_snp_init_locked(int *error)
> > +{
> > +	struct psp_device *psp = psp_master;
> > +	struct sev_data_snp_init_ex data;
> > +	struct sev_device *sev;
> > +	int rc = 0;
> > +
> > +	if (!psp || !psp->sev_data)
> > +		return -ENODEV;
> > +
> > +	sev = psp->sev_data;
> > +
> > +	if (sev->snp_initialized)
> > +		return 0;
>
> Shouldn't this follow this check:
>
>          if (sev->state == SEV_STATE_INIT) {
>                  /* debug printk about possible incorrect call order */
>                  return -ENODEV;
>          }
>
> It is game over for SNP, if SEV_CMD_INIT{_EX} got first, which means that
> this should not proceed.


But, how will SEV_CMD_INIT_EX happen before as sev_pci_init() which is 
invoked during CCP module load/initialization, will first try to do 
sev_snp_init() if SNP is supported, before it invokes 
sev_platform_init() to do SEV firmware initialization ?

Thanks,
Ashish