On 7/1/2025 11:26 AM, Huang, Kai wrote:
>>>
>>>> the VCPU ioctl. The desired_tsc_khz defaults to kvm->arch.default_tsc_khz.
>>>
>>> IIRC the KVM_SET_TSC_KHZ ioctl updates the kvm->arch.default_tsc_khz, and
>>> the snp_launch_start() always just uses it.
>>
>> Correct
>>
>>> The last sentence is kinda confusing since it sounds like that
>>> desired_tsc_khz is used by the SEV command and it could have a different
>>> value from kvm->arch.default_tsc_khz.
>>
>>
>> start.desired_tsc_khz is indeed used as part of SNP_LAUNCH_START command.
>>
>> How about something like the below:
>>
>> "In case, user has not set the TSC Frequency, desired_tsc_khz will default to
>> the host tsc frequency saved in kvm->arch.default_tsc_khz"
>
> Hmm.. If user has set the TSC frequency, desired_tsc_khz will still be the
> value in kvm->arch.default_tsc_khz.
>
> Not intended to nitpicking here, but how about something like:
>
> Always use kvm->arch.arch.default_tsc_khz as the TSC frequency that is
> passed to SNP guests in the SNP_LAUNCH_START command. The default value
> is the host TSC frequency. The userspace can optionally change it via
> the KVM_SET_TSC_KHZ ioctl before calling the SNP_LAUNCH_START ioctl.
Yes, this works as well.
>>
>>>
>>>> @@ -2146,6 +2158,14 @@ static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
>>>>
>>>> start.gctx_paddr = __psp_pa(sev->snp_context);
>>>> start.policy = params.policy;
>>>> +
>>>> + if (snp_secure_tsc_enabled(kvm)) {
>>>> + if (!kvm->arch.default_tsc_khz)
>>>> + return -EINVAL;
>>>> +
>>>> + start.desired_tsc_khz = kvm->arch.default_tsc_khz;
>>>> + }
>>>
>>> I didn't dig the full history so apologize if I missed anything.
>>>
>>> IIUC this code basically only sets start.desired_tsc_khz to default_tsc_khz
>>> w/o reading anything from params.desired_tsc_khz.
>>>
>>> Actually IIRC params.desired_tsc_khz isn't used at all in this patch, except
>>> it is defined in the userspace ABI structure.
>>>
>>> Do we actually need it? Since IIUC the userspace is supposed to use
>>> KVM_SET_TSC_KHZ ioctl to set the kvm->arch.default_tsc_khz before
>>> snp_launch_start() so here in snp_launch_start() we can just feed the
>>> default_tsc_khz to SEV command.
>>>
>>> Btw, in fact, I was wondering whether this patch can even compile because
>>> the 'desired_tsc_khz' was added to 'struct kvm_sev_snp_launch_start' but not
>>> 'struct sev_data_snp_launch_start', while the code:
>>>
>>> start.desired_tsc_khz = kvm->arch.default_tsc_khz;
>>>
>>> indicates it is the latter which should have this desired_tsc_khz member.
>>>
>>> Then I found it depends one commit that has already been merged to Sean's
>>> kvm-x86 tree but not in upstream yet (nor Paolo's tree):
>>>
>>> 51a4273dcab3 ("KVM: SVM: Add missing member in SNP_LAUNCH_START command
>>> structure"
>>>
>>> IMHO it would be helpful to somehow call this in the coverletter otherwise
>>> other people may get confused.
>>
>> I did mention in the v7 change log that patches are rebased on kvm-x86/next.
>> Next time I will make it more explicit.
>
> So could you confirm that we don't need the new 'desired_tsc_khz' in 'struct
> kvm_sev_snp_launch_start' as part of userspace ABI?
I agree, we can drop it from userspace ABI.
> I think this is where I got confused at the beginning.
>
> For explicitly calling out the 51a4273dcab3 as dependency, I guess it's
> perhaps just me, so feel free to ignore. Again, no intention of nitpicking
> here.
Sure.
Thanks
Nikunj
