Re: [PATCH v3 3/3] crypto: ccp - Add AMD Seamless Firmware Servicing (SFS) driver

[Kalra, Ashish]

Hello Tom,

On 9/8/2025 4:18 PM, Tom Lendacky wrote:
> On 8/20/25 17:19, Ashish Kalra wrote:
>> From: Ashish Kalra <ashish.kalra@xxxxxxx>
>>
>> AMD Seamless Firmware Servicing (SFS) is a secure method to allow
>> non-persistent updates to running firmware and settings without
>> requiring BIOS reflash and/or system reset.
>>
>> SFS does not address anything that runs on the x86 processors and
>> it can be used to update ASP firmware, modules, register settings
>> and update firmware for other microprocessors like TMPM, etc.
>>
>> SFS driver support adds ioctl support to communicate the SFS
>> commands to the ASP/PSP by using the TEE mailbox interface.
>>
>> The Seamless Firmware Servicing (SFS) driver is added as a
>> PSP sub-device.
>>
>> For detailed information, please look at the SFS specifications:
>> https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/specifications/58604.pdf
>>
>> Signed-off-by: Ashish Kalra <ashish.kalra@xxxxxxx>
>> ---
>>  drivers/crypto/ccp/Makefile         |   3 +-
>>  drivers/crypto/ccp/psp-dev.c        |  20 ++
>>  drivers/crypto/ccp/psp-dev.h        |   8 +-
>>  drivers/crypto/ccp/sfs.c            | 302 ++++++++++++++++++++++++++++
>>  drivers/crypto/ccp/sfs.h            |  47 +++++
>>  include/linux/psp-platform-access.h |   2 +
>>  include/uapi/linux/psp-sfs.h        |  87 ++++++++
>>  7 files changed, 467 insertions(+), 2 deletions(-)
>>  create mode 100644 drivers/crypto/ccp/sfs.c
>>  create mode 100644 drivers/crypto/ccp/sfs.h
>>  create mode 100644 include/uapi/linux/psp-sfs.h
>>
> 
>> +
>> +	/*
>> +	 * SFS command buffer must be mapped as non-cacheable.
>> +	 */
>> +	ret = set_memory_uc((unsigned long)sfs_dev->command_buf, SFS_NUM_PAGES_CMDBUF);
>> +	if (ret) {
>> +		dev_dbg(dev, "Set memory uc failed\n");
>> +		goto cleanup_cmd_buf;
>> +	}
> 

Yes, i was restoring the memory attribute before freeing, but then i realized that if the buffer
is transitioned to HV_Fixed and can't be freed but can still potentially be re-used then who will
setup the buffer to Uncacheable again, but i guess that should not be an issue as SFS driver will 
do that setup again after being unloaded/reloaded again, so i will go ahead and restore the memory
attribute here and in sfs_dev_destroy().

Thanks,
Ashish

> You should restore the memory attribute before freeing it in
> sfs_dev_destroy() and below in the cleanup.
> 
> Thanks,
> Tom
> 
>> +
>> +	dev_dbg(dev, "Command buffer 0x%px marked uncacheable\n", sfs_dev->command_buf);
>> +
>> +	psp->sfs_data = sfs_dev;
>> +	sfs_dev->dev = dev;
>> +	sfs_dev->psp = psp;
>> +
>> +	ret = sfs_misc_init(sfs_dev);
>> +	if (ret)
>> +		goto cleanup_cmd_buf;
>> +
>> +	dev_notice(sfs_dev->dev, "SFS support is available\n");
>> +
>> +	return 0;
>> +
>> +cleanup_cmd_buf:
>> +	snp_free_hv_fixed_pages(page);
>> +
>> +cleanup_dev:
>> +	psp->sfs_data = NULL;
>> +	devm_kfree(dev, sfs_dev);
>> +
>> +	return ret;
>> +}