Re: [PATCH v6 9/9] KVM: selftests: Add a basic SEV-SNP smoke test

[Pratik Rajesh Sampat <prsampat@amd.com>]

On 2/18/25 6:54 PM, Sean Christopherson wrote:
> On Fri, Feb 14, 2025, Pratik Rajesh Sampat wrote:
>>
>>
>> On 2/11/25 8:31 PM, Sean Christopherson wrote:
>>> On Mon, Feb 03, 2025, Pratik R. Sampat wrote:
>>>> @@ -217,5 +244,20 @@ int main(int argc, char *argv[])
>>>>  		}
>>>>  	}
>>>>  
>>>> +	if (kvm_cpu_has(X86_FEATURE_SEV_SNP)) {
>>>> +		uint64_t snp_policy = snp_default_policy();
>>>> +
>>>> +		test_snp(snp_policy);
>>>> +		/* Test minimum firmware level */
>>>> +		test_snp(snp_policy | SNP_FW_VER_MAJOR(SNP_MIN_API_MAJOR) |
>>>> +			SNP_FW_VER_MINOR(SNP_MIN_API_MINOR));
>>>
>>> Ah, this is where the firmware policy stuff is used.  Refresh me, can userspace
>>> request _any_ major/minor as the min, and expect failure if the version isn't
>>> supported?  If so, the test should iterate over the major/minor combinations that
>>> are guaranteed to fail.  And if userspace can query the supported minor/major,
>>> the test should iterate over all the happy versions too. 
>>>
>>
>> Yes, any policy greater than the min policy (defined in sev-dev.c)
>> should be supported. The sad path tests were intended to be added in the
>> upcoming negative test patch series so that we could have the proper
>> infrastructure to handle and report failures.
>>
>>> Unless there's nothing interesting to test, I would move the major/minor stuff to
>>> a separate patch.
>>
>> Would you rather prefer I do the happy tests here (something like -
>> min_policy and min_policy + 1?) and defer the failure tests for the
>> next patchset? Or, I can remove policy testing from here entirely and
>> introduce it only when the sad path testing infrastructure is ready, so
>> that we can test this completely at once?
> 
> Let's do the latter.  For the initial series, do the bare minimum so that we can
> get that merged, and then focus on the min API version stuff in a separate series.
> The version testing shouldn't be terribly complex, but it doesn't seem like it's
> entirely trivial either, and I don't want it to block the base SNP support.

Sure thing, will do.
Thanks!