| Message ID | 20250210092230.151034-4-nikunj@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Enable Secure TSC for SEV-SNP | expand |
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. > > Signed-off-by: Nikunj A Dadhania <nikunj@amd.com> > --- > 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)) { ? > + vcpu_unimpl(vcpu, "unimplemented IA32_TSC for secure tsc\n"); > + return 1; > + } > + break; > case MSR_IA32_DEBUGCTLMSR: > if (!lbrv) { > kvm_pr_unimpl_wrmsr(vcpu, ecx, data);
Tom Lendacky <thomas.lendacky@amd.com> 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@amd.com> >> --- >> 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. Once the guest state is protected, we do not expect any writes from VMM. Regards, Nikunj
On 2/11/25 02:24, Nikunj A Dadhania wrote: > Tom Lendacky <thomas.lendacky@amd.com> 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@amd.com> >>> --- >>> 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 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
On 2/11/25 08:03, Tom Lendacky wrote: > On 2/11/25 02:24, Nikunj A Dadhania wrote: >> Tom Lendacky <thomas.lendacky@amd.com> 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@amd.com> >>>> --- >>>> 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
On Mon, Feb 10, 2025, Tom Lendacky wrote: > 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. Unless it's architectural impossible for KVM to modify MSR_IA32_TSC, I don't see any reason for KVM to care. If the guest wants to modify TSC, that's the guest's prerogative. If KVM _can't_ honor the write, then that's something else entirely, and the changelog should pretty much write itself.
Tom Lendacky <thomas.lendacky@amd.com> writes: > On 2/11/25 08:03, Tom Lendacky wrote: >> On 2/11/25 02:24, Nikunj A Dadhania wrote: >>> Tom Lendacky <thomas.lendacky@amd.com> writes: >>> >>>> On 2/10/25 03:22, Nikunj A Dadhania wrote: > > 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. Right, I will add that. Regards Nikunj
Sean Christopherson <seanjc@google.com> writes: > On Mon, Feb 10, 2025, Tom Lendacky wrote: >> 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. > > Unless it's architectural impossible for KVM to modify MSR_IA32_TSC, I don't see > any reason for KVM to care. If the guest wants to modify TSC, that's the guest's > prerogative. > > If KVM _can't_ honor the write, then that's something else entirely, and the > changelog should pretty much write itself. How about the below changelog: KVM: SVM: Prevent writes to TSC MSR when Secure TSC is enabled Secure TSC enabled SNP guests should not write to the TSC MSR. Any such writes should be identified and ignored by the guest kernel in the #VC handler. As a safety measure, detect and disallow writes to MSR_IA32_TSC by Secure TSC enabled guests, as these writes are not expected to reach the hypervisor. Log the error and return #GP to the guest. Additionally, incorporate a check for protected guest state to allow the VMM to initialize the TSC MSR. Regards, Nikunj
On Wed, Feb 12, 2025, Nikunj A Dadhania wrote: > Sean Christopherson <seanjc@google.com> writes: > > > On Mon, Feb 10, 2025, Tom Lendacky wrote: > >> 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. > > > > Unless it's architectural impossible for KVM to modify MSR_IA32_TSC, I don't see > > any reason for KVM to care. If the guest wants to modify TSC, that's the guest's > > prerogative. > > > > If KVM _can't_ honor the write, then that's something else entirely, and the > > changelog should pretty much write itself. > > How about the below changelog: > > KVM: SVM: Prevent writes to TSC MSR when Secure TSC is enabled > > Secure TSC enabled SNP guests should not write to the TSC MSR. Any such This is a host write, not a guest write. What guest's "should" or should not do is irrelevant. > writes should be identified and ignored by the guest kernel in the #VC Again, I don't care what the guest does. Talking about #VC just adds noise. E.g. if the guest requests WRMSR emulation without ever doing WRMSR, there will be no #VC. > handler. As a safety measure, detect and disallow writes to MSR_IA32_TSC by No, KVM is not the trusted monitor. "safety measure" makes it sound like KVM is protecting the guest from a malicious VMM. That is not KVM's responsibility. > Secure TSC enabled guests, as these writes are not expected to reach the > hypervisor. Log the error and return #GP to the guest. Again, none of this ever says what actually happens if KVM tries to emulate a write to MSR_IA32_TSC. Based on what the APM says, the TSC fields in the control area are ignored. _That's_ what's relevant. The TSC value is first scaled with the GUEST_TSC_SCALE value from the VMSA and then is added to the VMSA GUEST_TSC_OFFSET value. The P0 frequency, TSC_RATIO (C001_0104h) and TSC_OFFSET (VMCB offset 50h) values are not used in the calculation.
On 2/12/2025 7:34 PM, Sean Christopherson wrote: > On Wed, Feb 12, 2025, Nikunj A Dadhania wrote: >> Sean Christopherson <seanjc@google.com> writes: >> Secure TSC enabled guests, as these writes are not expected to reach the >> hypervisor. Log the error and return #GP to the guest. > > Again, none of this ever says what actually happens if KVM tries to emulate a > write to MSR_IA32_TSC. Based on what the APM says, the TSC fields in the control > area are ignored. _That's_ what's relevant. > > The TSC value is first scaled with the GUEST_TSC_SCALE value from the VMSA and > then is added to the VMSA GUEST_TSC_OFFSET value. The P0 frequency, TSC_RATIO > (C001_0104h) and TSC_OFFSET (VMCB offset 50h) values are not used in the calculation. Sure, I will update the commit. Thanks, Nikunj
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)) { + vcpu_unimpl(vcpu, "unimplemented IA32_TSC for secure tsc\n"); + return 1; + } + break; case MSR_IA32_DEBUGCTLMSR: if (!lbrv) { kvm_pr_unimpl_wrmsr(vcpu, ecx, data);
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. Signed-off-by: Nikunj A Dadhania <nikunj@amd.com> --- arch/x86/kvm/svm/svm.c | 11 +++++++++++ 1 file changed, 11 insertions(+)