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

[Jarkko Sakkinen]

On Thu, Jan 05, 2023 at 04:40:29PM -0600, Kalra, Ashish wrote:
> 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 ?

Because the symbol is exported outside the driver to be called by other
subsystems, you need to have a santiy check for the call order, as it
is a hardware constraint. Otherwise, any unconsidered change in either
side could unknowingily break the kernel.

Alternatively, you could choose not to export sev_snp_init(). It is
supported by the fact that the call in sev_guest_init() is does nothing
useful (for the reasons you already wrote).

BR, Jarkko