Re: [PATCH v6 6/9] KVM: selftests: Add library support for interacting with SNP

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

On 2/19/25 10:55 AM, Sean Christopherson wrote:
> On Fri, Feb 14, 2025, Pratik Rajesh Sampat wrote:
>> On 2/11/25 8:12 PM, Sean Christopherson wrote:
>>> On Mon, Feb 03, 2025, Pratik R. Sampat wrote:
>>>> Extend the SEV library to include support for SNP ioctl() wrappers,
>>>> which aid in launching and interacting with a SEV-SNP guest.
>>>>
>>>> Tested-by: Srikanth Aithal <sraithal@amd.com>
>>>> Signed-off-by: Pratik R. Sampat <prsampat@amd.com>
>>
>> [..snip..]
>>
>>>> +/*
>>>> + * A SEV-SNP VM requires the policy reserved bit to always be set.
>>>> + * The SMT policy bit is also required to be set based on SMT being
>>>> + * available and active on the system.
>>>> + */
>>>> +static inline u64 snp_default_policy(void)
>>>> +{
>>>> +	bool smt_active = false;
>>>> +	FILE *f;
>>>> +
>>>> +	f = fopen("/sys/devices/system/cpu/smt/active", "r");
>>>
>>> Please add a helper to query if SMT is enabled.  I doubt there will ever be many
>>> users of this, but it doesn't seem like something that should buried in SNP code.
>>>
>>> Ha!  smt_possible() in tools/testing/selftests/kvm/x86/hyperv_cpuid.c is already
>>> guilty of burying a related helper, and it looks like it's a more robust version.
>>>
>>
>> You're right, a more general helper is in order here.
>>
>> Since the hyperv_cpuid selftest only seems to care about whether SMT is
>> possible (i.e., it may or may not be enabled) and we care about it being
>> enabled as well, for the flag to be set. I should make a more generic
>> variant(s) that can be accessible to both. Maybe I can implement it
>> within testing/selftests/kvm/include/x86_processor.h?
> 
> It should go in kvm_util.h, /sys/devices/system/cpu/smt/active is a generic interface
> provided by kernel/cpu.c.
> 

Sure, I will move it to the kvm_util header.

>>>> @@ -93,7 +124,7 @@ void sev_vm_launch(struct kvm_vm *vm, uint32_t policy)
>>>>  	TEST_ASSERT_EQ(status.state, SEV_GUEST_STATE_LAUNCH_UPDATE);
>>>>  
>>>>  	hash_for_each(vm->regions.slot_hash, ctr, region, slot_node)
>>>> -		encrypt_region(vm, region);
>>>> +		encrypt_region(vm, region, 0);
>>>
>>> Please add an enum/macro instead of open coding a literal '0'.  I gotta assume
>>> there's an appropriate name for page type '0'.
>>>
>>
>> For SNP, we supply this parameter to determine the page type for SNP
>> launch update defined as KVM_SEV_SNP_PAGE_TYPE_*. For SEV/SEV-ES,
>> however, the page type doesn't really get factored in and falls through
>> unaccounted in that case, so I had passed a zero to it.
>>
>> Having said that, having a literal here is quite unclean. Maybe I can
>> pass one of the existing page types to it or, better yet, define a new
>> KVM_SEV_PAGE_TYPE_[RESERVED|UNUSED] type instead for our selftest
>> header?
> 
> Ya, define something new and arbitrary.  I vote for either KVM_SEV_PAGE_TYPE_NONE
> or KVM_SEV_PAGE_TYPE_INVALID.  RESERVED suggests the page is "valid" but reserved
> for some entity.  Ditto for UNUSED; valid, but not yet claimed.

Got it.
Thank you!