Message-ID: <ed5b4ee0-3827-4d9c-81eb-99f3ea219c93@amd.com>
Date: Fri, 25 Jul 2025 10:16:57 -0500
From: "Kalra, Ashish" <ashish.kalra@....com>
To: Tom Lendacky <thomas.lendacky@....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

Hello Tom,

On 7/25/2025 9:28 AM, Tom Lendacky wrote:
> 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.
> 

Ok.

>> +
>> +	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.
>

Again, as you mentioned above this API interface is restricted to use till SNP is initialized,
so i think we can still have this (to handle cases where a sub-device init failure path
needs to remove it's HV_Fixed page from the list). So probably i can have this with a
check for SNP being already initialized and returning an error if it is, allowing the
user to leak 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.
> 

I believe that SNP_INIT_EX can add HV_Fixed pages which are not 2MB size aligned.

Here is a sub list of HV_Fixed pages being passed to SNP_INIT_EX: 

[   25.940837] base 0x0, count 1
[   25.940838] base 0xa0000, count 96
[   25.940839] base 0x75b60000, count 75
[   25.940839] base 0x75c60000, count 928
[   25.940840] base 0x88965000, count 83
[   25.940841] base 0x8a40c000, count 1
[   25.940841] base 0x8e14d000, count 48187
[   25.940842] base 0x99d88000, count 235
[   25.940842] base 0x99e73000, count 1153
[   25.940843] base 0x9a2f4000, count 12043
[   25.940844] base 0x9fffa000, count 5
[   25.940844] base 0xa0000000, count 65536
[   25.940845] base 0xb4000000, count 1
[   25.940845] base 0xb5080000, count 1
[   25.940846] base 0xbe100000, count 1
[   25.940847] base 0xbf000000, count 1
[   25.940847] base 0xd0080000, count 1
[   25.940848] base 0xd1100000, count 1
[   25.940848] base 0xec400000, count 1
...
...

>> +		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.
>
Ok.
 
> I would assume we'll see a failure in the SFS component if this fails, though.

Yes, SFS or any other sub-device component will fail in this case, but i don't want to abort
SNP_INIT_EX in this case.

Thanks,
Ashish

> 
> 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