linux-kernel - Re: [PATCH v8 00/10] Basic SEV-SNP Selftests

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <eea8f995-cd9c-455e-82ed-a3d31b75f8eb@amd.com>
Date: Tue, 6 May 2025 12:06:03 -0500
From: "Kalra, Ashish" <ashish.kalra@....com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: "Pratik R. Sampat" <prsampat@....com>, linux-kernel@...r.kernel.org,
 x86@...nel.org, kvm@...r.kernel.org, linux-kselftest@...r.kernel.org,
 pbonzini@...hat.com, thomas.lendacky@....com, tglx@...utronix.de,
 mingo@...hat.com, bp@...en8.de, dave.hansen@...ux.intel.com,
 shuah@...nel.org, pgonda@...gle.com, nikunj@....com, pankaj.gupta@....com,
 michael.roth@....com, sraithal@....com
Subject: Re: [PATCH v8 00/10] Basic SEV-SNP Selftests

Hello Sean,

On 5/5/2025 7:56 PM, Sean Christopherson wrote:
> On Mon, May 05, 2025, Ashish Kalra wrote:
>> On 5/5/2025 6:15 PM, Sean Christopherson wrote:
>>> @@ -3067,12 +3075,6 @@ void __init sev_hardware_setup(void)
>>>  
>>>         if (!sev_enabled)
>>>                 return;
>>> -
>>> -       /*
>>> -        * Do both SNP and SEV initialization at KVM module load.
>>> -        */
>>> -       init_args.probe = true;
>>> -       sev_platform_init(&init_args);
>>>  }
>>>  
>>>  void sev_hardware_unsetup(void)
>>> --
>>>
>>> Ashish, what am I missing?
>>>
>>
>> As far as setting sev*_enabled is concerned, i believe they are more specific
>> to SNP/SEV/SEV-ES being enabled in the system, which is separate from
>> SEV_INIT/SNP_INIT (SNP_INIT success indicates that RMP been initialized, SNP
>> has to be already enabled via MSR_SYSCFG before SNP_INIT is called), though
>> SEV_INIT/SNP_INIT may fail but SEV/SNP support will still be enabled on the
>> system.
> 
> No, if SNP_INIT fails and has zero chance of succeeding, then SNP is *NOT*
> supported *by KVM*.  The platform may be configured to support SNP, but that
> matters not at all if KVM can't actually use the functionality.
>

Yes, i agree.
 
>> Additionally as SEV_INIT/SNP_INIT during sev_platform_init() have failed, so
>> any SEV/SEV-ES/SNP VM launch will fail as the firmware will return invalid
>> platform state as INITs have failed.
> 
> Yeah, and that's *awful* behavior for KVM.  Imagine if KVM did that for every
> feature, i.e. enumerated hardware support irrespective of KVM support.
> 
> The API is KVM_GET_SUPPORTED_CPUID, not KVM_GET_MOSTLY_SUPPORTED_CPUID.
> 
>> >From my understanding, sev*_enabled indicates the user support to
>>> enable/disable support for SEV/SEV-ES/SEV-SNP, 
> 
> Yes, and they're also used to reflect and enumerate KVM support:
> 
> 	if (sev_enabled) {
> 		kvm_cpu_cap_set(X86_FEATURE_SEV);
> 		kvm_caps.supported_vm_types |= BIT(KVM_X86_SEV_VM);
> 	}
> 	if (sev_es_enabled) {
> 		kvm_cpu_cap_set(X86_FEATURE_SEV_ES);
> 		kvm_caps.supported_vm_types |= BIT(KVM_X86_SEV_ES_VM);
> 	}
> 	if (sev_snp_enabled) {
> 		kvm_cpu_cap_set(X86_FEATURE_SEV_SNP);
> 		kvm_caps.supported_vm_types |= BIT(KVM_X86_SNP_VM);
> 	}
> 
>> as the sev*_enabled are the KVM module parameters, while sev*_supported
>> indicates if platform has that support enabled.
> 
> sev*_supported are completely irrelevant.  They are function local scratch variables
> that exist so that KVM doesn't clobber userspace's inputs while computing what is
> fully supported and enabled.
> 

Ok.

>> And before the SEV/SNP init support was moved to KVM from CCP module, doing
>> SEV/SNP INIT could fail but that still had KVM detecting SEV/SNP support
>> enabled, so this moving SEV/SNP init stuff to KVM module from CCP driver is
>> consistent with the previous behavior.
> 
> And one of my driving motivations for getting the initialization into KVM was to
> fix that previous behavior.

Sure, i will test your patch and post it.  

Thanks,
Ashish