On 7/15/21 6:48 PM, Sean Christopherson wrote:

On Wed, Jul 07, 2021, Brijesh Singh wrote:

quoted

The behavior and requirement for the SEV-legacy command is altered when
the SNP firmware is in the INIT state. See SEV-SNP firmware specification
for more details.

When SNP is INIT state, all the SEV-legacy commands that cause the
firmware to write memory must be in the firmware state. The TMR memory

It'd be helpful to spell out Trusted Memory Region, I hadn't seen that
term before and for some reason my brain immediately thought "xAPIC register!".

Noted.

quoted

is allocated by the host but updated by the firmware, so, it must be
in the firmware state.  Additionally, the TMR memory must be a 2MB aligned
instead of the 1MB, and the TMR length need to be 2MB instead of 1MB.
The helper __snp_{alloc,free}_firmware_pages() can be used for allocating
and freeing the memory used by the firmware.

None of this actually states what the patch does, e.g. it's not clear whether
all allocations are being converted to 2mb or just the SNP.  Looks like it's
just SNP.  Something like this?

  Allocate the Trusted Memory Region (TMR) as a 2mb sized/aligned region when
  SNP is enabled to satisfy new requirements for SNP.  Continue allocating a
  1mb region for !SNP configuration.

Only the TMR allocation is converted to use the 2mb when SNP is enabled.

quoted

While at it, provide API that can be used by others to allocate a page
that can be used by the firmware. The immediate user for this API will
be the KVM driver. The KVM driver to need to allocate a firmware context
page during the guest creation. The context page need to be updated
by the firmware. See the SEV-SNP specification for further details.

...

quoted

@@ -1153,8 +1269,10 @@ static void sev_firmware_shutdown(struct sev_device *sev)
 		/* The TMR area was encrypted, flush it from the cache */
 		wbinvd_on_all_cpus();
 
-		free_pages((unsigned long)sev_es_tmr,
-			   get_order(SEV_ES_TMR_SIZE));
+
+		__snp_free_firmware_pages(virt_to_page(sev_es_tmr),
+					  get_order(sev_es_tmr_size),
+					  false);
 		sev_es_tmr = NULL;
 	}
 

@@ -1204,16 +1322,6 @@ void sev_pci_init(void)
 	    sev_update_firmware(sev->dev) == 0)
 		sev_get_api_version();
 
-	/* Obtain the TMR memory area for SEV-ES use */
-	tmr_page = alloc_pages(GFP_KERNEL, get_order(SEV_ES_TMR_SIZE));
-	if (tmr_page) {
-		sev_es_tmr = page_address(tmr_page);
-	} else {
-		sev_es_tmr = NULL;
-		dev_warn(sev->dev,
-			 "SEV: TMR allocation failed, SEV-ES support unavailable\n");
-	}
-
 	/*
 	 * If boot CPU supports the SNP, then first attempt to initialize
 	 * the SNP firmware.

@@ -1229,6 +1337,16 @@ void sev_pci_init(void)
 		}
 	}
 
+	/* Obtain the TMR memory area for SEV-ES use */
+	tmr_page = __snp_alloc_firmware_pages(GFP_KERNEL, get_order(sev_es_tmr_size), false);
+	if (tmr_page) {
+		sev_es_tmr = page_address(tmr_page);
+	} else {
+		sev_es_tmr = NULL;
+		dev_warn(sev->dev,
+			 "SEV: TMR allocation failed, SEV-ES support unavailable\n");
+	}

I think your patch ordering got a bit wonky.  AFAICT, the chunk that added
sev_snp_init() and friends in the previous patch 14 should have landed above
the TMR allocation, i.e. the code movement here should be unnecessary.

I was debating about it whether to include all the SNP supports in one
patch or divide it up. If I had included all legacy support new
requirement in the same patch which adds the SNP then it will be a big
patch. I had feeling that others may ask me to split it. So my approach is:

* In the first patch adds SNP support only

* Improve the legacy SEV/ES for the requirement when SNP is enabled.
Once SNP is enabled,  there are two new requirement for the legacy
SEV/ES guests

  1) TMR must be 2mb

  2) The buffer given to the firmware for the write must be in the
firmware state.

I also divided both of the new requirement in separate patches so that
its easy to review.

quoted

 	/* Initialize the platform */
 	rc = sev_platform_init(&error);
 	if (rc && (error == SEV_RET_SECURE_DATA_INVALID)) {

...

quoted

@@ -961,6 +965,13 @@ static inline int snp_guest_dbg_decrypt(struct sev_data_snp_dbg *data, int *erro
 	return -ENODEV;
 }
 
+static inline void *snp_alloc_firmware_page(gfp_t mask)
+{
+	return NULL;
+}
+
+static inline void snp_free_firmware_page(void *addr) { }

Hmm, I think we should probably bite the bullet and #ifdef and/or stub out large
swaths of svm/sev.c before adding SNP support.  sev.c is getting quite massive,
and we're accumulating more and more stubs outside of KVM because its SEV code
is compiled unconditionally.