From: Ashish Kalra <ashish.kalra@amd.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@amd.com <Thomas.Lendacky@amd.com>
Signed-off-by: Ashish Kalra <ashish.kalra@amd.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);
+
+ 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);
+
+ spin_unlock(&snp_hv_fixed_pages_lock);
+
+ return 0;
+}
+
+void snp_delete_hypervisor_fixed_pages_list(u64 paddr)
+{
+ 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);
+ 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;
+ 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);
+
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 */
--
2.34.1On 7/24/25 16:14, Ashish Kalra wrote:
> From: Ashish Kalra <ashish.kalra@amd.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@amd.com <Thomas.Lendacky@amd.com>
> Signed-off-by: Ashish Kalra <ashish.kalra@amd.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 */
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@amd.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@amd.com <Thomas.Lendacky@amd.com>
>> Signed-off-by: Ashish Kalra <ashish.kalra@amd.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 */
On 7/25/25 10:16, Kalra, Ashish wrote:
> 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@amd.com>
>>> +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 ?
I'd prefer to have the decision to leak the page being done in a single
place. If this ends up being used by more than just SFS, then there's
another place that needs to know to do that.
>
>>> + 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
Right, but those are resource-based items that I think result in 4K direct
map entries around them being 4K. I just want you to verify that a 2M
mapping will be split automatically by the SNP code (which I believe it
will, but we should verify).
Thanks,
Tom
> ...
© 2016 - 2025 Red Hat, Inc.