Re: [PATCH v3 2/3] crypto: ccp - Add new HV-Fixed page allocation/free API.

[Tom Lendacky <thomas.lendacky@amd.com>]

On 8/20/25 17:19, Ashish Kalra wrote:
> From: Ashish Kalra <ashish.kalra@amd.com>
> 
> When SEV-SNP is active, the TEE extended command header page and
> all output buffers for TEE extended commands (such as used by Seamless
> Firmware servicing support) must be in hypervisor-fixed state,
> assigned to the hypervisor and marked immutable in the RMP entrie(s).
> 
> Add a new generic SEV API interface to allocate/free hypervisor fixed
> pages which abstracts hypervisor fixed page allocation/free for PSP
> sub devices. The API internally uses SNP_INIT_EX to transition pages
> to HV-Fixed page state.
> 
> If SNP is not enabled then the allocator is simply a wrapper over
> alloc_pages() and __free_pages().
> 
> When the sub device free the pages, they are put on a free list
> and future allocation requests will try to re-use the freed pages from
> this list. But this list is not preserved across PSP driver load/unload
> hence this free/reuse support is only supported while PSP driver is
> loaded. As HV_FIXED page state is only changed at reboot, these pages
> are leaked as they cannot be returned back to the page allocator and
> then potentially allocated to guests, which will cause SEV-SNP guests
> to fail to start or terminate when accessing the HV_FIXED page.
> 
> Suggested-by: Thomas Lendacky <Thomas.Lendacky@amd.com>
> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> ---
>  drivers/crypto/ccp/sev-dev.c | 182 +++++++++++++++++++++++++++++++++++
>  drivers/crypto/ccp/sev-dev.h |   3 +
>  2 files changed, 185 insertions(+)
> 
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index 4f000dc2e639..1560009c2f18 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -82,6 +82,21 @@ MODULE_FIRMWARE("amd/amd_sev_fam19h_model1xh.sbin"); /* 4th gen EPYC */
>  static bool psp_dead;
>  static int psp_timeout;
>  
> +enum snp_hv_fixed_pages_state {
> +	ALLOCATED,
> +	HV_FIXED,
> +};
> +
> +struct snp_hv_fixed_pages_entry {
> +	struct list_head list;
> +	struct page *page;
> +	unsigned int order;
> +	bool free;
> +	enum snp_hv_fixed_pages_state page_state;
> +};
> +
> +static LIST_HEAD(snp_hv_fixed_pages);
> +
>  /* Trusted Memory Region (TMR):
>   *   The TMR is a 1MB area that must be 1MB aligned.  Use the page allocator
>   *   to allocate the memory, which will return aligned memory for the specified
> @@ -1157,6 +1172,165 @@ static int snp_get_platform_data(struct sev_device *sev, int *error)
>  	return rc;
>  }
>  
> +/* Hypervisor Fixed pages API interface */
> +static void snp_hv_fixed_pages_state_update(struct sev_device *sev,
> +					    enum snp_hv_fixed_pages_state page_state)
> +{
> +	struct snp_hv_fixed_pages_entry *entry;
> +
> +	/* List is protected by sev_cmd_mutex */
> +	lockdep_assert_held(&sev_cmd_mutex);
> +
> +	if (list_empty(&snp_hv_fixed_pages))
> +		return;
> +
> +	list_for_each_entry(entry, &snp_hv_fixed_pages, list)
> +		entry->page_state = page_state;
> +}
> +
> +/*
> + * Allocate HV_FIXED pages in 2MB aligned sizes to ensure the whole
> + * 2MB pages are marked as HV_FIXED.
> + */
> +struct page *snp_alloc_hv_fixed_pages(unsigned int num_2mb_pages)
> +{
> +	struct psp_device *psp_master = psp_get_master_device();
> +	struct snp_hv_fixed_pages_entry *entry;
> +	struct sev_device *sev;
> +	unsigned int order;
> +	struct page *page;
> +
> +	if (!psp_master || !psp_master->sev_data)
> +		return NULL;
> +
> +	sev = psp_master->sev_data;
> +
> +	/*
> +	 * This API uses SNP_INIT_EX to transition allocated pages to HV_Fixed
> +	 * page state, fail if SNP is already initialized.
> +	 */
> +	if (sev->snp_initialized)
> +		return NULL;
> +
> +	order = get_order(PMD_SIZE * num_2mb_pages);
> +
> +	/*
> +	 * SNP_INIT_EX is protected by sev_cmd_mutex, therefore this list
> +	 * also needs to be protected using the same mutex.
> +	 */
> +	guard(mutex)(&sev_cmd_mutex);

Does the guard() need to come before the snp_initialized check (or the
snp_initialized check after taking the mutex)? An SNP_INIT_EX (which
would grab the mutex and make this call wait) can occur between the
check and obtaining the mutex.

Thanks,
Tom

> +