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

[Sean Christopherson <seanjc@google.com>]

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.

> 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.

> 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.