Re: [PATCH v3 2/9] KVM: selftests: Add a basic SNP smoke test

["Pratik R. Sampat" <pratikrajesh.sampat@amd.com>]

Hi Sean,

On 10/30/2024 12:57 PM, Sean Christopherson wrote:
> On Wed, Oct 30, 2024, Pratik R. Sampat wrote:
>> On 10/30/2024 8:46 AM, Sean Christopherson wrote:
>>> +/* Minimum firmware version required for the SEV-SNP support */
>>> +#define SNP_FW_REQ_VER_MAJOR   1
>>> +#define SNP_FW_REQ_VER_MINOR   51
>>>
>>> Side topic, why are these hardcoded?  And where did they come from?  If they're
>>> arbitrary KVM selftests values, make that super duper clear.
>>
>> Well, it's not entirely arbitrary. This was the version that SNP GA'd
>> with first so that kind of became the minimum required version needed.
>>
>> I think the only place we've documented this is here -
>> https://github.com/AMDESE/AMDSEV/tree/snp-latest?tab=readme-ov-file#upgrade-sev-firmware.
>>
>> Maybe, I can modify the comment above to say something like -
>> Minimum general availability release firmware required for SEV-SNP support.
> 
> Hmm, so if AMD says SNP is only supported for firmware version >= 1.51, why on
> earth is that not checked and enforced by the kernel?  Relying on userspace to
> not crash the host (or worse) because of unsupported firmware is not a winning
> strategy.

We do check against the firmware level 1.51 while setting things up
first (drivers/crypto/ccp/sev-dev.c:__sev_snp_init_locked()) and we bail
out if it's otherwise. From the userspace, calls to KVM_SEV_INIT2 or any
other corresponding SNP calls should fail cleanly without any adverse
effects to the host.

From the positive selftest perspective though, we want to make sure it's
both supported and enabled, and skip the test if not.
I believe we can tell if it's supported by the platform using the MSR -
MSR_AMD64_SEV_SNP_ENABLED or the X86_FEATURE_SEV_SNP from the KVM
capabilities. However, to determine if it's enabled from the kernel, I
made this check here. Having said that, I do agree that there should
probably be a better way to expose this support to the userspace.

Thanks
Pratik