| Message ID | 20241203090045.942078-7-nikunj@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Secure TSC support for SNP guests | expand |
On Tue, Dec 03, 2024 at 02:30:38PM +0530, Nikunj A Dadhania wrote: > diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c > index af28fb962309..59c5e716fdd1 100644 > --- a/arch/x86/coco/sev/core.c > +++ b/arch/x86/coco/sev/core.c > @@ -1473,6 +1473,14 @@ static enum es_result vc_handle_msr(struct ghcb *ghcb, struct es_em_ctxt *ctxt) > if (regs->cx == MSR_IA32_TSC && (sev_status & MSR_AMD64_SNP_SECURE_TSC)) > return __vc_handle_msr_tsc(regs, write); > > + /* > + * GUEST_TSC_FREQ should not be intercepted when Secure TSC is > + * enabled. Terminate the SNP guest when the interception is enabled. > + */ > + if (regs->cx == MSR_AMD64_GUEST_TSC_FREQ && (sev_status & MSR_AMD64_SNP_SECURE_TSC)) > + return ES_VMM_ERROR; > + > + If you merge this logic into the switch-case, the patch becomes even easier and the code cleaner: diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c index 050170eb28e6..35d9a3bb4b06 100644 --- a/arch/x86/coco/sev/core.c +++ b/arch/x86/coco/sev/core.c @@ -1446,6 +1446,13 @@ static enum es_result __vc_handle_msr_tsc(struct pt_regs *regs, bool write) if (!(sev_status & MSR_AMD64_SNP_SECURE_TSC)) goto read_tsc; + /* + * GUEST_TSC_FREQ should not be intercepted when Secure TSC is + * enabled. Terminate the SNP guest when the interception is enabled. + */ + if (regs->cx == MSR_AMD64_GUEST_TSC_FREQ) + return ES_VMM_ERROR; + if (write) { WARN_ONCE(1, "TSC MSR writes are verboten!\n"); return ES_UNSUPPORTED; @@ -1472,6 +1479,7 @@ static enum es_result vc_handle_msr(struct ghcb *ghcb, struct es_em_ctxt *ctxt) case MSR_SVSM_CAA: return __vc_handle_msr_caa(regs, write); case MSR_IA32_TSC: + case MSR_AMD64_GUEST_TSC_FREQ: return __vc_handle_msr_tsc(regs, write); default: break;
On 12/10/2024 5:41 PM, Borislav Petkov wrote: > On Tue, Dec 03, 2024 at 02:30:38PM +0530, Nikunj A Dadhania wrote: >> diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c >> index af28fb962309..59c5e716fdd1 100644 >> --- a/arch/x86/coco/sev/core.c >> +++ b/arch/x86/coco/sev/core.c >> @@ -1473,6 +1473,14 @@ static enum es_result vc_handle_msr(struct ghcb *ghcb, struct es_em_ctxt *ctxt) >> if (regs->cx == MSR_IA32_TSC && (sev_status & MSR_AMD64_SNP_SECURE_TSC)) >> return __vc_handle_msr_tsc(regs, write); >> >> + /* >> + * GUEST_TSC_FREQ should not be intercepted when Secure TSC is >> + * enabled. Terminate the SNP guest when the interception is enabled. >> + */ >> + if (regs->cx == MSR_AMD64_GUEST_TSC_FREQ && (sev_status & MSR_AMD64_SNP_SECURE_TSC)) >> + return ES_VMM_ERROR; >> + >> + > > If you merge this logic into the switch-case, the patch becomes even easier > and the code cleaner: This is incorrect, for a non-Secure TSC guest, a read of intercepted MSR_AMD64_GUEST_TSC_FREQ will return value of rdtsc_ordered(). This is an invalid MSR when SecureTSC is not enabled. > > diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c > index 050170eb28e6..35d9a3bb4b06 100644 > --- a/arch/x86/coco/sev/core.c > +++ b/arch/x86/coco/sev/core.c > @@ -1446,6 +1446,13 @@ static enum es_result __vc_handle_msr_tsc(struct pt_regs *regs, bool write) > if (!(sev_status & MSR_AMD64_SNP_SECURE_TSC)) > goto read_tsc; > > + /* > + * GUEST_TSC_FREQ should not be intercepted when Secure TSC is > + * enabled. Terminate the SNP guest when the interception is enabled. > + */ > + if (regs->cx == MSR_AMD64_GUEST_TSC_FREQ) > + return ES_VMM_ERROR; > + > if (write) { > WARN_ONCE(1, "TSC MSR writes are verboten!\n"); > return ES_UNSUPPORTED; > @@ -1472,6 +1479,7 @@ static enum es_result vc_handle_msr(struct ghcb *ghcb, struct es_em_ctxt *ctxt) > case MSR_SVSM_CAA: > return __vc_handle_msr_caa(regs, write); > case MSR_IA32_TSC: > + case MSR_AMD64_GUEST_TSC_FREQ: > return __vc_handle_msr_tsc(regs, write); > default: > break; > Regards Nikunj
On Tue, Dec 10, 2024 at 10:43:05PM +0530, Nikunj A Dadhania wrote: > This is incorrect, for a non-Secure TSC guest, a read of intercepted > MSR_AMD64_GUEST_TSC_FREQ will return value of rdtsc_ordered(). This is an invalid > MSR when SecureTSC is not enabled. So how would you change this diff to fix this?
On 12/10/24 11:13, Nikunj A Dadhania wrote: > On 12/10/2024 5:41 PM, Borislav Petkov wrote: >> On Tue, Dec 03, 2024 at 02:30:38PM +0530, Nikunj A Dadhania wrote: >>> diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c >>> index af28fb962309..59c5e716fdd1 100644 >>> --- a/arch/x86/coco/sev/core.c >>> +++ b/arch/x86/coco/sev/core.c >>> @@ -1473,6 +1473,14 @@ static enum es_result vc_handle_msr(struct ghcb *ghcb, struct es_em_ctxt *ctxt) >>> if (regs->cx == MSR_IA32_TSC && (sev_status & MSR_AMD64_SNP_SECURE_TSC)) >>> return __vc_handle_msr_tsc(regs, write); >>> >>> + /* >>> + * GUEST_TSC_FREQ should not be intercepted when Secure TSC is >>> + * enabled. Terminate the SNP guest when the interception is enabled. >>> + */ >>> + if (regs->cx == MSR_AMD64_GUEST_TSC_FREQ && (sev_status & MSR_AMD64_SNP_SECURE_TSC)) >>> + return ES_VMM_ERROR; >>> + >>> + >> >> If you merge this logic into the switch-case, the patch becomes even easier >> and the code cleaner: > > This is incorrect, for a non-Secure TSC guest, a read of intercepted > MSR_AMD64_GUEST_TSC_FREQ will return value of rdtsc_ordered(). This is an invalid > MSR when SecureTSC is not enabled. For the non-Secure TSC guest, I still think that we should continue to use the GHCB MSR NAE event instead of switching to using rdtsc_ordered(). Thanks, Tom > >> >> diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c >> index 050170eb28e6..35d9a3bb4b06 100644 >> --- a/arch/x86/coco/sev/core.c >> +++ b/arch/x86/coco/sev/core.c >> @@ -1446,6 +1446,13 @@ static enum es_result __vc_handle_msr_tsc(struct pt_regs *regs, bool write) >> if (!(sev_status & MSR_AMD64_SNP_SECURE_TSC)) >> goto read_tsc; >> >> + /* >> + * GUEST_TSC_FREQ should not be intercepted when Secure TSC is >> + * enabled. Terminate the SNP guest when the interception is enabled. >> + */ >> + if (regs->cx == MSR_AMD64_GUEST_TSC_FREQ) >> + return ES_VMM_ERROR; >> + >> if (write) { >> WARN_ONCE(1, "TSC MSR writes are verboten!\n"); >> return ES_UNSUPPORTED; >> @@ -1472,6 +1479,7 @@ static enum es_result vc_handle_msr(struct ghcb *ghcb, struct es_em_ctxt *ctxt) >> case MSR_SVSM_CAA: >> return __vc_handle_msr_caa(regs, write); >> case MSR_IA32_TSC: >> + case MSR_AMD64_GUEST_TSC_FREQ: >> return __vc_handle_msr_tsc(regs, write); >> default: >> break; >> > > Regards > Nikunj
On 12/10/2024 10:48 PM, Borislav Petkov wrote: > On Tue, Dec 10, 2024 at 10:43:05PM +0530, Nikunj A Dadhania wrote: >> This is incorrect, for a non-Secure TSC guest, a read of intercepted >> MSR_AMD64_GUEST_TSC_FREQ will return value of rdtsc_ordered(). This is an invalid >> MSR when SecureTSC is not enabled. > > So how would you change this diff to fix this? How about the below change, this also keeps the behavior intact for non-Secure TSC guests (SEV-ES/SNP): diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c index 25a4c47f58c4..fa57adf5a2c6 100644 --- a/arch/x86/coco/sev/core.c +++ b/arch/x86/coco/sev/core.c @@ -1448,15 +1448,18 @@ static enum es_result __vc_handle_msr_tsc(struct pt_regs *regs, bool write) { u64 tsc; - if (!(sev_status & MSR_AMD64_SNP_SECURE_TSC)) - goto read_tsc; + /* + * GUEST_TSC_FREQ should not be intercepted when Secure TSC is + * enabled. Terminate the SNP guest when the interception is enabled. + */ + if (regs->cx == MSR_AMD64_GUEST_TSC_FREQ) + return ES_VMM_ERROR; if (write) { WARN_ONCE(1, "TSC MSR writes are verboten!\n"); return ES_OK; } -read_tsc: tsc = rdtsc_ordered(); regs->ax = lower_32_bits(tsc); regs->dx = upper_32_bits(tsc); @@ -1477,19 +1480,13 @@ static enum es_result vc_handle_msr(struct ghcb *ghcb, struct es_em_ctxt *ctxt) case MSR_SVSM_CAA: return __vc_handle_msr_caa(regs, write); case MSR_IA32_TSC: - return __vc_handle_msr_tsc(regs, write); + case MSR_AMD64_GUEST_TSC_FREQ: + if (sev_status & MSR_AMD64_SNP_SECURE_TSC) + return __vc_handle_msr_tsc(regs, write); default: break; } - /* - * GUEST_TSC_FREQ should not be intercepted when Secure TSC is - * enabled. Terminate the SNP guest when the interception is enabled. - */ - if (regs->cx == MSR_AMD64_GUEST_TSC_FREQ && (sev_status & MSR_AMD64_SNP_SECURE_TSC)) - return ES_VMM_ERROR; - - ghcb_set_rcx(ghcb, regs->cx); if (write) { ghcb_set_rax(ghcb, regs->ax); --- Regards Nikunj
On Thu, Dec 12, 2024 at 10:23:01AM +0530, Nikunj A. Dadhania wrote: > @@ -1477,19 +1480,13 @@ static enum es_result vc_handle_msr(struct ghcb *ghcb, struct es_em_ctxt *ctxt) > case MSR_SVSM_CAA: > return __vc_handle_msr_caa(regs, write); > case MSR_IA32_TSC: > - return __vc_handle_msr_tsc(regs, write); > + case MSR_AMD64_GUEST_TSC_FREQ: > + if (sev_status & MSR_AMD64_SNP_SECURE_TSC) > + return __vc_handle_msr_tsc(regs, write); Now push that conditional inside the function too.
On 12/17/2024 4:27 PM, Borislav Petkov wrote: > On Thu, Dec 12, 2024 at 10:23:01AM +0530, Nikunj A. Dadhania wrote: >> @@ -1477,19 +1480,13 @@ static enum es_result vc_handle_msr(struct ghcb *ghcb, struct es_em_ctxt *ctxt) >> case MSR_SVSM_CAA: >> return __vc_handle_msr_caa(regs, write); >> case MSR_IA32_TSC: >> - return __vc_handle_msr_tsc(regs, write); >> + case MSR_AMD64_GUEST_TSC_FREQ: >> + if (sev_status & MSR_AMD64_SNP_SECURE_TSC) >> + return __vc_handle_msr_tsc(regs, write); With the above change, non-Secure TSC SNP guest will return following error: $ sudo rdmsr 0xc0010134 rdmsr: CPU 0 cannot read MSR 0xc0010134 $ > Now push that conditional inside the function too. With the condition inside the function, even tough the MSR is not valid in this configuration, I am getting value 0. Is this behavior acceptable ? $ sudo rdmsr 0xc0010134 0 $ Regards, Nikunj
On Wed, Dec 18, 2024 at 10:50:07AM +0530, Nikunj A. Dadhania wrote: > With the condition inside the function, even tough the MSR is not > valid in this configuration, I am getting value 0. Is this behavior > acceptable ? The whole untested diff, should DTRT this time: diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c index 10980898c054..96a9ee93f9cb 100644 --- a/arch/x86/coco/sev/core.c +++ b/arch/x86/coco/sev/core.c @@ -1441,10 +1441,25 @@ static enum es_result __vc_handle_msr_caa(struct pt_regs *regs, bool write) */ static enum es_result __vc_handle_msr_tsc(struct pt_regs *regs, bool write) { + bool sec_tsc = sev_status & MSR_AMD64_SNP_SECURE_TSC; u64 tsc; - if (write) - return ES_OK; + /* + * The GUEST_TSC_FREQ MSR should not be intercepted when secure + * TSC is enabled so terminate the guest. For non-secure TSC + * guests, that MSR is #GP(0). + */ + if (regs->cx == MSR_AMD64_GUEST_TSC_FREQ) { + if (sec_tsc) + return ES_VMM_ERROR; + else + return ES_UNSUPPORTED; + } + + if (write) { + WARN_ONCE(1, "TSC MSR writes are verboten!\n"); + return ES_UNSUPPORTED; + } tsc = rdtsc_ordered(); regs->ax = lower_32_bits(tsc); @@ -1462,19 +1477,15 @@ static enum es_result vc_handle_msr(struct ghcb *ghcb, struct es_em_ctxt *ctxt) /* Is it a WRMSR? */ write = ctxt->insn.opcode.bytes[1] == 0x30; - if (regs->cx == MSR_SVSM_CAA) + switch(regs->cx) { + case MSR_SVSM_CAA: return __vc_handle_msr_caa(regs, write); - - if (regs->cx == MSR_IA32_TSC && (sev_status & MSR_AMD64_SNP_SECURE_TSC)) + case MSR_IA32_TSC: + case MSR_AMD64_GUEST_TSC_FREQ: return __vc_handle_msr_tsc(regs, write); - - /* - * GUEST_TSC_FREQ should not be intercepted when Secure TSC is - * enabled. Terminate the SNP guest when the interception is enabled. - */ - if (regs->cx == MSR_AMD64_GUEST_TSC_FREQ && (sev_status & MSR_AMD64_SNP_SECURE_TSC)) - return ES_VMM_ERROR; - + default: + break; + } ghcb_set_rcx(ghcb, regs->cx); if (write) {
On 12/24/2024 5:23 PM, Borislav Petkov wrote: > On Wed, Dec 18, 2024 at 10:50:07AM +0530, Nikunj A. Dadhania wrote: >> With the condition inside the function, even tough the MSR is not >> valid in this configuration, I am getting value 0. Is this behavior >> acceptable ? > > The whole untested diff, should DTRT this time: I have tested the diff and ES_UNSUPPORTED causes unexpected termination of SNP guest(without SecureTSC). $ sudo wrmsr 0x10 0 KVM: unknown exit reason 24 EAX=00000000 EBX=00000000 ECX=00000000 EDX=00a00f11 ESI=00000000 EDI=00000000 EBP=00000000 ESP=00000000 EIP=0000fff0 EFL=00000002 [-------] CPL=0 II=0 A20=1 SMM=0 HLT=0 ... $ sudo wrmsr 0xc0010134 0 KVM: unknown exit reason 24 EAX=00000000 EBX=00000000 ECX=00000000 EDX=00a00f11 ESI=00000000 EDI=00000000 EBP=00000000 ESP=00000000 ... IMO, the below change appropriately handles all the conditions well and does not affect SNP guests without SecureTSC. diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c index 84e4e64decf7..a8977c68695b 100644 --- a/arch/x86/coco/sev/core.c +++ b/arch/x86/coco/sev/core.c @@ -1428,6 +1428,40 @@ static enum es_result __vc_handle_msr_caa(struct pt_regs *regs, bool write) return ES_OK; } +/* + * TSC related accesses should not exit to the hypervisor when a guest is + * executing with SecureTSC enabled, so special handling is required for + * accesses of MSR_IA32_TSC and MSR_AMD64_GUEST_TSC_FREQ: + * + * Writes: Writing to MSR_IA32_TSC can cause subsequent reads + * of the TSC to return undefined values, so ignore all + * writes. + * Reads: Reads of MSR_IA32_TSC should return the current TSC + * value, use the value returned by RDTSC. + */ +static enum es_result __vc_handle_msr_tsc(struct pt_regs *regs, bool write) +{ + u64 tsc; + + /* + * GUEST_TSC_FREQ should not be intercepted when Secure TSC is + * enabled. Terminate the SNP guest when the interception is enabled. + */ + if (regs->cx == MSR_AMD64_GUEST_TSC_FREQ) + return ES_VMM_ERROR; + + if (write) { + WARN_ONCE(1, "TSC MSR writes are verboten!\n"); + return ES_OK; + } + + tsc = rdtsc_ordered(); + regs->ax = lower_32_bits(tsc); + regs->dx = upper_32_bits(tsc); + + return ES_OK; +} + static enum es_result vc_handle_msr(struct ghcb *ghcb, struct es_em_ctxt *ctxt) { struct pt_regs *regs = ctxt->regs; @@ -1437,8 +1471,16 @@ static enum es_result vc_handle_msr(struct ghcb *ghcb, struct es_em_ctxt *ctxt) /* Is it a WRMSR? */ write = ctxt->insn.opcode.bytes[1] == 0x30; - if (regs->cx == MSR_SVSM_CAA) + switch (regs->cx) { + case MSR_SVSM_CAA: return __vc_handle_msr_caa(regs, write); + case MSR_IA32_TSC: + case MSR_AMD64_GUEST_TSC_FREQ: + if (sev_status & MSR_AMD64_SNP_SECURE_TSC) + return __vc_handle_msr_tsc(regs, write); + default: + break; + } ghcb_set_rcx(ghcb, regs->cx); if (write) {
On Wed, Jan 01, 2025 at 02:14:38PM +0530, Nikunj A. Dadhania wrote: > @@ -1437,8 +1471,16 @@ static enum es_result vc_handle_msr(struct ghcb *ghcb, struct es_em_ctxt *ctxt) > /* Is it a WRMSR? */ > write = ctxt->insn.opcode.bytes[1] == 0x30; > > - if (regs->cx == MSR_SVSM_CAA) > + switch (regs->cx) { > + case MSR_SVSM_CAA: > return __vc_handle_msr_caa(regs, write); > + case MSR_IA32_TSC: > + case MSR_AMD64_GUEST_TSC_FREQ: > + if (sev_status & MSR_AMD64_SNP_SECURE_TSC) > + return __vc_handle_msr_tsc(regs, write); Again: move all the logic inside __vc_handle_msr_tsc().
On 1/1/2025 9:40 PM, Borislav Petkov wrote: > On Wed, Jan 01, 2025 at 02:14:38PM +0530, Nikunj A. Dadhania wrote: >> @@ -1437,8 +1471,16 @@ static enum es_result vc_handle_msr(struct ghcb *ghcb, struct es_em_ctxt *ctxt) >> /* Is it a WRMSR? */ >> write = ctxt->insn.opcode.bytes[1] == 0x30; >> >> - if (regs->cx == MSR_SVSM_CAA) >> + switch (regs->cx) { >> + case MSR_SVSM_CAA: >> return __vc_handle_msr_caa(regs, write); >> + case MSR_IA32_TSC: >> + case MSR_AMD64_GUEST_TSC_FREQ: >> + if (sev_status & MSR_AMD64_SNP_SECURE_TSC) >> + return __vc_handle_msr_tsc(regs, write); > > Again: move all the logic inside __vc_handle_msr_tsc(). > Boris, I have mentioned in my previous reply why this is incorrect for non-SecureTSC guests with examples and had asked for your confirmation if it is an acceptable behaviour[1] and Tom has also objected to the change of behavior for non-SecureTSC guest[2]. I think we are dragging this a little too far and the implementation[3] that I gave is good without any side effects. Regards Nikunj 1: https://lore.kernel.org/all/7a5de2be-4e79-409a-90f2-398815fc59c7@amd.com 2: https://lore.kernel.org/all/1510fe7f-1c10-aea7-75be-37c5c58d6a05@amd.com 3: https://lore.kernel.org/all/a28dfd0a-c0ab-490f-bc1a-945182d07790@amd.com
On Thu, Jan 02, 2025 at 10:33:26AM +0530, Nikunj A. Dadhania wrote: > I think we are dragging this a little too far What does that mean? Is there some time limit I don't know about, on how long patches should be reviewed on the mailing list? > and the implementation[3] that I gave is good without any side effects. Well, apparently not good enough - otherwise I won't say anything, would I? And I wouldn't review your patches on my holiday, would I? Geez. Now lemme try again, this time in greater detail with the hope it is more clear. If you handle TSC MSRs and then you have a function __vc_handle_msr_tsc() then *all* handling should happen there! There should not be if-conditionals in the switch-case which makes following the code flow harder. That's why I'm asking you to push the conditional inside. So that everything is concentrated in a single function! But there's this thing with handling TSC MSRs and non-STSC guests and that needs special, later handling and decision. So *that* needs to be made obvious. As in: I will handle the TSC MSRs for STSC guests and the other flow for non-STSC guests should remain. For now. And make that goddamn explicit. One possible way to do that is this: diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c index 6235286a0eda..61100532c259 100644 --- a/arch/x86/coco/sev/core.c +++ b/arch/x86/coco/sev/core.c @@ -1439,7 +1439,7 @@ static enum es_result __vc_handle_msr_caa(struct pt_regs *regs, bool write) * Reads: Reads of MSR_IA32_TSC should return the current TSC * value, use the value returned by RDTSC. */ -static enum es_result __vc_handle_msr_tsc(struct pt_regs *regs, bool write) +static enum es_result __vc_handle_secure_tsc_msrs(struct pt_regs *regs, bool write) { u64 tsc; @@ -1477,7 +1477,9 @@ static enum es_result vc_handle_msr(struct ghcb *ghcb, struct es_em_ctxt *ctxt) case MSR_IA32_TSC: case MSR_AMD64_GUEST_TSC_FREQ: if (sev_status & MSR_AMD64_SNP_SECURE_TSC) - return __vc_handle_msr_tsc(regs, write); + return __vc_handle_secure_tsc_msrs(regs, write); + else + break; default: break; } --- You can still push the if-conditional inside the function but that'll need more surgery as you need a different retval to tell vc_handle_msr() to fall-through to the GHCB HV call instead of returning, yadda, yadda so this version above is shorter. And it can be revisited later, when we decide what we wanna do with TSC MSRs on !STSC guests. IOW, the code is still clear and there's enough breadcrumbs left to know what needs to happen there in the future. Versus: lemme drop my enablement patches and disappear and the maintainers can mop up after me. Who cares! :-( I hope this makes it a lot more clear now. And again, if this takes too long for you, just lemme know: I have absolutely no problem if someone else who's faster reviews your code - I have more than enough TODOs on my plate so not dealing with this would be more than welcome for me.
On 1/2/2025 2:37 PM, Borislav Petkov wrote: > On Thu, Jan 02, 2025 at 10:33:26AM +0530, Nikunj A. Dadhania wrote: > As in: I will handle the TSC MSRs for STSC guests and the other flow for > non-STSC guests should remain. For now. > > And make that goddamn explicit. > > One possible way to do that is this: I agree, if renaming helps to make it explicit, this is perfect. Thanks. > > diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c > index 6235286a0eda..61100532c259 100644 > --- a/arch/x86/coco/sev/core.c > +++ b/arch/x86/coco/sev/core.c > @@ -1439,7 +1439,7 @@ static enum es_result __vc_handle_msr_caa(struct pt_regs *regs, bool write) > * Reads: Reads of MSR_IA32_TSC should return the current TSC > * value, use the value returned by RDTSC. > */ > -static enum es_result __vc_handle_msr_tsc(struct pt_regs *regs, bool write) > +static enum es_result __vc_handle_secure_tsc_msrs(struct pt_regs *regs, bool write) > { > u64 tsc; > > @@ -1477,7 +1477,9 @@ static enum es_result vc_handle_msr(struct ghcb *ghcb, struct es_em_ctxt *ctxt) > case MSR_IA32_TSC: > case MSR_AMD64_GUEST_TSC_FREQ: > if (sev_status & MSR_AMD64_SNP_SECURE_TSC) > - return __vc_handle_msr_tsc(regs, write); > + return __vc_handle_secure_tsc_msrs(regs, write); > + else > + break; > default: > break; > } > --- Regards, Nikunj
On 1/2/25 03:30, Nikunj A. Dadhania wrote: > On 1/2/2025 2:37 PM, Borislav Petkov wrote: >> On Thu, Jan 02, 2025 at 10:33:26AM +0530, Nikunj A. Dadhania wrote: > >> As in: I will handle the TSC MSRs for STSC guests and the other flow for >> non-STSC guests should remain. For now. >> >> And make that goddamn explicit. >> >> One possible way to do that is this: > > I agree, if renaming helps to make it explicit, this is perfect. Thanks. > >> >> diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c >> index 6235286a0eda..61100532c259 100644 >> --- a/arch/x86/coco/sev/core.c >> +++ b/arch/x86/coco/sev/core.c >> @@ -1439,7 +1439,7 @@ static enum es_result __vc_handle_msr_caa(struct pt_regs *regs, bool write) >> * Reads: Reads of MSR_IA32_TSC should return the current TSC >> * value, use the value returned by RDTSC. >> */ >> -static enum es_result __vc_handle_msr_tsc(struct pt_regs *regs, bool write) >> +static enum es_result __vc_handle_secure_tsc_msrs(struct pt_regs *regs, bool write) >> { >> u64 tsc; >> >> @@ -1477,7 +1477,9 @@ static enum es_result vc_handle_msr(struct ghcb *ghcb, struct es_em_ctxt *ctxt) >> case MSR_IA32_TSC: >> case MSR_AMD64_GUEST_TSC_FREQ: >> if (sev_status & MSR_AMD64_SNP_SECURE_TSC) >> - return __vc_handle_msr_tsc(regs, write); >> + return __vc_handle_secure_tsc_msrs(regs, write); >> + else >> + break; There's a return as part of the if, so no reason for the else. Just put the break in the normal place and it reads much clearer. Thanks, Tom >> default: >> break; >> } >> --- > > Regards, > Nikunj
On Thu, Jan 02, 2025 at 08:45:36AM -0600, Tom Lendacky wrote: > >> @@ -1477,7 +1477,9 @@ static enum es_result vc_handle_msr(struct ghcb *ghcb, struct es_em_ctxt *ctxt) > >> case MSR_IA32_TSC: > >> case MSR_AMD64_GUEST_TSC_FREQ: > >> if (sev_status & MSR_AMD64_SNP_SECURE_TSC) > >> - return __vc_handle_msr_tsc(regs, write); > >> + return __vc_handle_secure_tsc_msrs(regs, write); > >> + else > >> + break; > > There's a return as part of the if, so no reason for the else. Just put > the break in the normal place and it reads much clearer. I guess that's in the eye of the beholder. I prefer a balanced if foo else bar as it is as obvious and clear as it gets. Especially if it is interwoven in a switch-case like here.
diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h index 3ae84c3b8e6d..233be13cc21f 100644 --- a/arch/x86/include/asm/msr-index.h +++ b/arch/x86/include/asm/msr-index.h @@ -608,6 +608,7 @@ #define MSR_AMD_PERF_CTL 0xc0010062 #define MSR_AMD_PERF_STATUS 0xc0010063 #define MSR_AMD_PSTATE_DEF_BASE 0xc0010064 +#define MSR_AMD64_GUEST_TSC_FREQ 0xc0010134 #define MSR_AMD64_OSVW_ID_LENGTH 0xc0010140 #define MSR_AMD64_OSVW_STATUS 0xc0010141 #define MSR_AMD_PPIN_CTL 0xc00102f0 diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c index af28fb962309..59c5e716fdd1 100644 --- a/arch/x86/coco/sev/core.c +++ b/arch/x86/coco/sev/core.c @@ -1473,6 +1473,14 @@ static enum es_result vc_handle_msr(struct ghcb *ghcb, struct es_em_ctxt *ctxt) if (regs->cx == MSR_IA32_TSC && (sev_status & MSR_AMD64_SNP_SECURE_TSC)) return __vc_handle_msr_tsc(regs, write); + /* + * GUEST_TSC_FREQ should not be intercepted when Secure TSC is + * enabled. Terminate the SNP guest when the interception is enabled. + */ + if (regs->cx == MSR_AMD64_GUEST_TSC_FREQ && (sev_status & MSR_AMD64_SNP_SECURE_TSC)) + return ES_VMM_ERROR; + + ghcb_set_rcx(ghcb, regs->cx); if (write) { ghcb_set_rax(ghcb, regs->ax);