On 2/11/25 08:03, Tom Lendacky wrote:
> On 2/11/25 02:24, Nikunj A Dadhania wrote:
>> Tom Lendacky <thomas.lendacky@xxxxxxx> writes:
>>
>>> On 2/10/25 03:22, Nikunj A Dadhania wrote:
>>>> Disallow writes to MSR_IA32_TSC for Secure TSC enabled SNP guests, as such
>>>> writes are not expected. Log the error and return #GP to the guest.
>>>
>>> Re-word this to make it a bit clearer about why this is needed. It is
>>> expected that the guest won't write to MSR_IA32_TSC or, if it does, it
>>> will ignore any writes to it and not exit to the HV. So this is catching
>>> the case where that behavior is not occurring.
>>>
>> Sure, will update.
>>
>>>>
>>>> Signed-off-by: Nikunj A Dadhania <nikunj@xxxxxxx>
>>>> ---
>>>> arch/x86/kvm/svm/svm.c | 11 +++++++++++
>>>> 1 file changed, 11 insertions(+)
>>>>
>>>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>>>> index d7a0428aa2ae..929f35a2f542 100644
>>>> --- a/arch/x86/kvm/svm/svm.c
>>>> +++ b/arch/x86/kvm/svm/svm.c
>>>> @@ -3161,6 +3161,17 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
>>>>
>>>> svm->tsc_aux = data;
>>>> break;
>>>> + case MSR_IA32_TSC:
>>>> + /*
>>>> + * If Secure TSC is enabled, KVM doesn't expect to receive
>>>> + * a VMEXIT for a TSC write, record the error and return a
>>>> + * #GP
>>>> + */
>>>> + if (vcpu->arch.guest_state_protected && snp_secure_tsc_enabled(vcpu->kvm)) {
>>>
>>> Does it matter if the VMSA has already been encrypted? Can this just be
>>>
>>> if (sev_snp_guest() && snp_secure_tsc_enabled(vcpu->kvm)) {
>>>
>>> ?
>>>
>>
>> QEMU initializes the IA32_TSC MSR to zero resulting in the below
>> error if I use the above.
>>
>> qemu-system-x86_64: error: failed to set MSR 0x10 to 0x0
>> qemu-system-x86_64: ../target/i386/kvm/kvm.c:3849: kvm_buf_set_msrs: Assertion `ret == cpu->kvm_msr_buf->nmsrs' failed.
>
> Should KVM be doing anything related to MSR_IA32_TSC for a Secure TSC
> guest, even handling this Qemu write? That Qemu write takes it through
> the kvm_synchronize_tsc() path, does it need to? I'm just wondering if
Ah, strike that, the write from Qemu is just ignored in this situation.
But doesn't that break existing support since we no longer call into
kvm_set_msr_common() for MSR_IA32_TSC? This looks like it needs to
invoke kvm_set_msr_common() if it isn't a Secure TSC guest.
Thanks,
Tom
> the Secure TSC HV support needs more handling of MSR_IA32_TSC (in both
> set and get) than what's here. Thoughts?
>
> Thanks,
> Tom
>
>>
>> Once the guest state is protected, we do not expect any writes from VMM.
>>
>> Regards,
>> Nikunj
