Message-ID: <ff83d1e4-5fbd-360a-22ea-10efd71ff2d9@amd.com>
Date: Fri, 25 Jul 2025 09:28:00 -0500
From: Tom Lendacky <thomas.lendacky@....com>
To: Ashish Kalra <Ashish.Kalra@....com>, john.allen@....com,
 herbert@...dor.apana.org.au, davem@...emloft.net
Cc: seanjc@...gle.com, pbonzini@...hat.com, michael.roth@....com,
 linux-crypto@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] crypto: ccp - Add new API for extending HV_Fixed
 Pages

On 7/24/25 16:14, Ashish Kalra wrote:
> From: Ashish Kalra <ashish.kalra@....com>
> 
> Implement new API to add support for extending the HV_Fixed pages list
> passed to SNP_INIT_EX.
> 
> Adds a simple list based interface to extend the HV_Fixed pages list
> for PSP sub-devices such as the SFS driver.
> 
> Suggested-by: Thomas.Lendacky@....com <Thomas.Lendacky@....com>
> Signed-off-by: Ashish Kalra <ashish.kalra@....com>
> ---
>  drivers/crypto/ccp/sev-dev.c | 88 ++++++++++++++++++++++++++++++++++++
>  drivers/crypto/ccp/sev-dev.h |  3 ++
>  2 files changed, 91 insertions(+)
> 
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index e058ba027792..c3ff40cd7a96 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -82,6 +82,14 @@ MODULE_FIRMWARE("amd/amd_sev_fam19h_model1xh.sbin"); /* 4th gen EPYC */
>  static bool psp_dead;
>  static int psp_timeout;
>  
> +struct snp_hv_fixed_pages_entry {
> +	u64 base;
> +	int npages;
> +	struct list_head list;
> +};
> +static LIST_HEAD(snp_hv_fixed_pages);
> +static DEFINE_SPINLOCK(snp_hv_fixed_pages_lock);
> +
>  /* 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
> @@ -1073,6 +1081,76 @@ static void snp_set_hsave_pa(void *arg)
>  	wrmsrq(MSR_VM_HSAVE_PA, 0);
>  }
>  
> +int snp_insert_hypervisor_fixed_pages_list(u64 paddr, int npages)
> +{
> +	struct snp_hv_fixed_pages_entry *entry;
> +
> +	spin_lock(&snp_hv_fixed_pages_lock);

Please use guard() so that you don't have to issue spin_unlock() anywhere.

> +
> +	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
> +	if (!entry) {
> +		spin_unlock(&snp_hv_fixed_pages_lock);
> +		return -ENOMEM;
> +	}
> +	entry->base = paddr;
> +	entry->npages = npages;
> +	list_add_tail(&entry->list, &snp_hv_fixed_pages);

You're creating this API that can now be called at any time. Either
restrict it to when SNP is not initialized or add support to issue
SNP_PAGE_SET_STATE.

I would suggest that you return an error for now if SNP is initialized.

> +
> +	spin_unlock(&snp_hv_fixed_pages_lock);
> +
> +	return 0;
> +}
> +
> +void snp_delete_hypervisor_fixed_pages_list(u64 paddr)
> +{

Not sure you can have this...  Once a page is marked HV_FIXED it can't be
changed unless SNP (SNPEn bit in SYS_CFG MSR) is disabled, which doesn't
happen until reboot.

So users of this interface will have to leak pages since they can't be
released back to the general allocation pool for chance they get used for
an SNP guest.

So this API should probably be deleted.

Or you change this to a driver HV_FIXED allocation/free setup where this
performs the allocation and adds the memory to the list and the free API
leaks the page.

> +	struct snp_hv_fixed_pages_entry *entry, *nentry;
> +
> +	spin_lock(&snp_hv_fixed_pages_lock);
> +	list_for_each_entry_safe(entry, nentry, &snp_hv_fixed_pages, list) {
> +		if (entry->base == paddr) {
> +			list_del(&entry->list);
> +			kfree(entry);
> +			break;
> +		}
> +	}
> +	spin_unlock(&snp_hv_fixed_pages_lock);
> +}
> +
> +static int snp_extend_hypervisor_fixed_pages(struct sev_data_range_list *range_list)
> +{
> +	struct sev_data_range *range = &range_list->ranges[range_list->num_elements];
> +	struct snp_hv_fixed_pages_entry *entry;
> +	int new_element_count, ret = 0;
> +
> +	spin_lock(&snp_hv_fixed_pages_lock);

guard()

> +	if (list_empty(&snp_hv_fixed_pages))
> +		goto out;
> +
> +	new_element_count = list_count_nodes(&snp_hv_fixed_pages) +
> +			    range_list->num_elements;
> +
> +	/*
> +	 * Ensure the list of HV_FIXED pages that will be passed to firmware
> +	 * do not exceed the page-sized argument buffer.
> +	 */
> +	if (new_element_count * sizeof(struct sev_data_range) +
> +	    sizeof(struct sev_data_range_list) > PAGE_SIZE) {
> +		ret = -E2BIG;
> +		goto out;
> +	}
> +
> +	list_for_each_entry(entry, &snp_hv_fixed_pages, list) {
> +		range->base = entry->base;
> +		range->page_count = entry->npages;

Will there be an issue if the size is not 2MB aligned? I think a PSMASH
will be done, but something to test if you are going to allow any page
alignment and page count.

> +		range++;
> +	}
> +	range_list->num_elements = new_element_count;
> +out:
> +	spin_unlock(&snp_hv_fixed_pages_lock);
> +
> +	return ret;
> +}
> +
>  static int snp_filter_reserved_mem_regions(struct resource *rs, void *arg)
>  {
>  	struct sev_data_range_list *range_list = arg;
> @@ -1163,6 +1241,16 @@ static int __sev_snp_init_locked(int *error)
>  			return rc;
>  		}
>  
> +		/*
> +		 * Extend the HV_Fixed pages list with HV_Fixed pages added from other
> +		 * PSP sub-devices such as SFS. Warn if the list can't be extended
> +		 * but continue with SNP_INIT_EX.
> +		 */
> +		rc = snp_extend_hypervisor_fixed_pages(snp_range_list);
> +		if (rc)
> +			dev_warn(sev->dev,
> +				 "SEV: SNP_INIT_EX extend HV_Fixed pages failed rc = %d\n", rc);

If you aren't going to do anything with the error other than print a
warning, this should be moved to the snp_extend_hypervisor_fixed_pages()
function and have it be a void function.

I would assume we'll see a failure in the SFS component if this fails, though.

Thanks,
Tom

> +
>  		memset(&data, 0, sizeof(data));
>  		data.init_rmp = 1;
>  		data.list_paddr_en = 1;
> diff --git a/drivers/crypto/ccp/sev-dev.h b/drivers/crypto/ccp/sev-dev.h
> index 3e4e5574e88a..444d7fffd801 100644
> --- a/drivers/crypto/ccp/sev-dev.h
> +++ b/drivers/crypto/ccp/sev-dev.h
> @@ -65,4 +65,7 @@ void sev_dev_destroy(struct psp_device *psp);
>  void sev_pci_init(void);
>  void sev_pci_exit(void);
>  
> +int snp_insert_hypervisor_fixed_pages_list(u64 paddr, int npages);
> +void snp_delete_hypervisor_fixed_pages_list(u64 paddr);
> +
>  #endif /* __SEV_DEV_H */

Powered by blists - more mailing lists