Re: [PATCH 1/2] crypto: ccp - Add new API for extending HV_Fixed Pages

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

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

> ...