Hello Tom,
On 2/20/2025 2:03 PM, Tom Lendacky wrote:
> On 2/19/25 14:55, Ashish Kalra wrote:
>> From: Ashish Kalra <ashish.kalra@xxxxxxx>
>>
>> SNP initialization is forced during PSP driver probe purely because SNP
>> can't be initialized if VMs are running. But the only in-tree user of
>> SEV/SNP functionality is KVM, and KVM depends on PSP driver for the same.
>> Forcing SEV/SNP initialization because a hypervisor could be running
>> legacy non-confidential VMs make no sense.
>>
>> This patch removes SEV/SNP initialization from the PSP driver probe
>> time and moves the requirement to initialize SEV/SNP functionality
>> to KVM if it wants to use SEV/SNP.
>>
>> Suggested-by: Sean Christopherson <seanjc@xxxxxxxxxx>
>> Reviewed-by: Alexey Kardashevskiy <aik@xxxxxxx>
>> Signed-off-by: Ashish Kalra <ashish.kalra@xxxxxxx>
>> ---
>> drivers/crypto/ccp/sev-dev.c | 25 +------------------------
>> 1 file changed, 1 insertion(+), 24 deletions(-)
>>
>> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
>> index f0f3e6d29200..99a663dbc2b6 100644
>> --- a/drivers/crypto/ccp/sev-dev.c
>> +++ b/drivers/crypto/ccp/sev-dev.c
>> @@ -1346,18 +1346,13 @@ static int _sev_platform_init_locked(struct sev_platform_init_args *args)
>> if (sev->state == SEV_STATE_INIT)
>> return 0;
>>
>> - /*
>> - * Legacy guests cannot be running while SNP_INIT(_EX) is executing,
>> - * so perform SEV-SNP initialization at probe time.
>> - */
>> rc = __sev_snp_init_locked(&args->error);
>> if (rc && rc != -ENODEV) {
>> /*
>> * Don't abort the probe if SNP INIT failed,
>> * continue to initialize the legacy SEV firmware.
>> */
>> - dev_err(sev->dev, "SEV-SNP: failed to INIT rc %d, error %#x\n",
>> - rc, args->error);
>> + dev_err(sev->dev, "SEV-SNP: failed to INIT, continue SEV INIT\n");
>
> Please don't remove the error information.
>
The error(s) are already being printed in __sev_snp_init_locked() otherwise the same
error will be printed twice, hence removing it here.
>> }
>>
>> /* Defer legacy SEV/SEV-ES support if allowed by caller/module. */
>> @@ -2505,9 +2500,7 @@ EXPORT_SYMBOL_GPL(sev_issue_cmd_external_user);
>> void sev_pci_init(void)
>> {
>> struct sev_device *sev = psp_master->sev_data;
>> - struct sev_platform_init_args args = {0};
>> u8 api_major, api_minor, build;
>> - int rc;
>>
>> if (!sev)
>> return;
>> @@ -2530,16 +2523,6 @@ void sev_pci_init(void)
>> api_major, api_minor, build,
>> sev->api_major, sev->api_minor, sev->build);
>>
>> - /* Initialize the platform */
>> - args.probe = true;
>> - rc = sev_platform_init(&args);
>> - if (rc)
>> - dev_err(sev->dev, "SEV: failed to INIT error %#x, rc %d\n",
>> - args.error, rc);
>> -
>> - dev_info(sev->dev, "SEV%s API:%d.%d build:%d\n", sev->snp_initialized ?
>> - "-SNP" : "", sev->api_major, sev->api_minor, sev->build);
>> -
>> return;
>>
>> err:
>> @@ -2550,10 +2533,4 @@ void sev_pci_init(void)
>>
>> void sev_pci_exit(void)
>> {
>> - struct sev_device *sev = psp_master->sev_data;
>> -
>> - if (!sev)
>> - return;
>> -
>> - sev_firmware_shutdown(sev);
>
> Should this remain? If there's a bug in KVM that somehow skips the
> shutdown call, then SEV will remain initialized. I think the path is
> safe to call a second time.
Ok.
Thanks,
Ashish
