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@xxxxxxx>
>>
>> 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@xxxxxxx <Thomas.Lendacky@xxxxxxx>
>> Signed-off-by: Ashish Kalra <ashish.kalra@xxxxxxx>
>> ---
>> 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 */
