linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] x86/sev: Improve handling of writes to intercepted GUEST_TSC_FREQ
@ 2025-07-11  4:12 Nikunj A Dadhania
  2025-07-14 10:44 ` Borislav Petkov
  0 siblings, 1 reply; 15+ messages in thread
From: Nikunj A Dadhania @ 2025-07-11  4:12 UTC (permalink / raw)
  To: linux-kernel, bp, x86
  Cc: seanjc, tglx, mingo, dave.hansen, thomas.lendacky, santosh.shukla,
	Nikunj A Dadhania

From: Sean Christopherson <seanjc@google.com>

For Secure TSC enabled guests, don't panic when a guest writes to
intercepted GUEST_TSC_FREQ MSR. Instead, ignore writes to GUEST_TSC_FREQ,
similar to MSR_IA32_TSC, and log a warning instead.

Only terminate the guest when reading from intercepted GUEST_TSC_FREQ MSR
with Secure TSC enabled, as this indicates an unexpected hypervisor
configuration.

Signed-off-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Nikunj A Dadhania <nikunj@amd.com>
---
 arch/x86/coco/sev/vc-handle.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/arch/x86/coco/sev/vc-handle.c b/arch/x86/coco/sev/vc-handle.c
index faf1fce89ed4..581e34083321 100644
--- a/arch/x86/coco/sev/vc-handle.c
+++ b/arch/x86/coco/sev/vc-handle.c
@@ -376,24 +376,21 @@ static enum es_result __vc_handle_secure_tsc_msrs(struct pt_regs *regs, bool wri
 	u64 tsc;
 
 	/*
-	 * GUEST_TSC_FREQ should not be intercepted when Secure TSC is enabled.
-	 * Terminate the SNP guest when the interception is enabled.
+	 * Writing to MSR_IA32_TSC can cause subsequent reads of the TSC to
+	 * return undefined values, and GUEST_TSC_FREQ is read-only.  Ignore
+	 * all writes, but WARN to log the kernel bug.
 	 */
-	if (regs->cx == MSR_AMD64_GUEST_TSC_FREQ)
-		return ES_VMM_ERROR;
+	if (WARN_ON_ONCE(write))
+		return ES_OK;
 
 	/*
-	 * 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_ordered().
+	 * GUEST_TSC_FREQ read should not be intercepted when Secure TSC is
+	 * enabled. Terminate the SNP guest when the interception is enabled.
 	 */
-	if (write) {
-		WARN_ONCE(1, "TSC MSR writes are verboten!\n");
-		return ES_OK;
-	}
+	if (regs->cx == MSR_AMD64_GUEST_TSC_FREQ)
+		return ES_VMM_ERROR;
 
+	/* Reads of MSR_IA32_TSC should return the current TSC value. */
 	tsc = rdtsc_ordered();
 	regs->ax = lower_32_bits(tsc);
 	regs->dx = upper_32_bits(tsc);
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH] x86/sev: Improve handling of writes to intercepted GUEST_TSC_FREQ
  2025-07-11  4:12 [PATCH] x86/sev: Improve handling of writes to intercepted GUEST_TSC_FREQ Nikunj A Dadhania
@ 2025-07-14 10:44 ` Borislav Petkov
  2025-07-14 14:24   ` Sean Christopherson
  0 siblings, 1 reply; 15+ messages in thread
From: Borislav Petkov @ 2025-07-14 10:44 UTC (permalink / raw)
  To: Nikunj A Dadhania
  Cc: linux-kernel, x86, seanjc, tglx, mingo, dave.hansen,
	thomas.lendacky, santosh.shukla

On Fri, Jul 11, 2025 at 09:42:00AM +0530, Nikunj A Dadhania wrote:
> From: Sean Christopherson <seanjc@google.com>
> 
> For Secure TSC enabled guests, don't panic when a guest writes to
> intercepted GUEST_TSC_FREQ MSR. Instead, ignore writes to GUEST_TSC_FREQ,
> similar to MSR_IA32_TSC, and log a warning instead.

Why?

Nothing should poke at the TSC MSR and those who do, deserve what they get.

> Only terminate the guest when reading from intercepted GUEST_TSC_FREQ MSR
> with Secure TSC enabled, as this indicates an unexpected hypervisor
> configuration.

Huh, this sounds weird.

What are we "fixing" here?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] x86/sev: Improve handling of writes to intercepted GUEST_TSC_FREQ
  2025-07-14 10:44 ` Borislav Petkov
@ 2025-07-14 14:24   ` Sean Christopherson
  2025-07-14 14:59     ` Tom Lendacky
  0 siblings, 1 reply; 15+ messages in thread
From: Sean Christopherson @ 2025-07-14 14:24 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Nikunj A Dadhania, linux-kernel, x86, tglx, mingo, dave.hansen,
	thomas.lendacky, santosh.shukla

On Mon, Jul 14, 2025, Borislav Petkov wrote:
> On Fri, Jul 11, 2025 at 09:42:00AM +0530, Nikunj A Dadhania wrote:
> > From: Sean Christopherson <seanjc@google.com>
> > 
> > For Secure TSC enabled guests, don't panic when a guest writes to
> > intercepted GUEST_TSC_FREQ MSR. Instead, ignore writes to GUEST_TSC_FREQ,
> > similar to MSR_IA32_TSC, and log a warning instead.
> 
> Why?
> 
> Nothing should poke at the TSC MSR and those who do, deserve what they get.
> 
> > Only terminate the guest when reading from intercepted GUEST_TSC_FREQ MSR
> > with Secure TSC enabled, as this indicates an unexpected hypervisor
> > configuration.
> 
> Huh, this sounds weird.
> 
> What are we "fixing" here?

Returning ES_VMM_ERROR is misleading/wrong, and panicking doesn't match how the
kernel handles every other "bad" WRMSR.  How's this for a changelog?

  For Secure TSC enabled guests, don't panic if the kernel hits a #VC due
  to attempting to write to GUEST_TSC_FREQ, and instead WARN and drop the
  write.  The kernel should never write GUEST_TSC_FREQ as it's read-only,
  but panicking with ES_VMM_ERROR is both misleading (it's entirely
  reasonable for a VMM to intercept writes to a read-only MSR), and
  unnecessary, e.g. the kernel eats #GPs with a WARN on every other "bad"
  WRMSR.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] x86/sev: Improve handling of writes to intercepted GUEST_TSC_FREQ
  2025-07-14 14:24   ` Sean Christopherson
@ 2025-07-14 14:59     ` Tom Lendacky
  2025-07-14 15:17       ` Sean Christopherson
  0 siblings, 1 reply; 15+ messages in thread
From: Tom Lendacky @ 2025-07-14 14:59 UTC (permalink / raw)
  To: Sean Christopherson, Borislav Petkov
  Cc: Nikunj A Dadhania, linux-kernel, x86, tglx, mingo, dave.hansen,
	santosh.shukla

On 7/14/25 09:24, Sean Christopherson wrote:
> On Mon, Jul 14, 2025, Borislav Petkov wrote:
>> On Fri, Jul 11, 2025 at 09:42:00AM +0530, Nikunj A Dadhania wrote:
>>> From: Sean Christopherson <seanjc@google.com>
>>>
>>> For Secure TSC enabled guests, don't panic when a guest writes to
>>> intercepted GUEST_TSC_FREQ MSR. Instead, ignore writes to GUEST_TSC_FREQ,
>>> similar to MSR_IA32_TSC, and log a warning instead.
>>
>> Why?
>>
>> Nothing should poke at the TSC MSR and those who do, deserve what they get.
>>
>>> Only terminate the guest when reading from intercepted GUEST_TSC_FREQ MSR
>>> with Secure TSC enabled, as this indicates an unexpected hypervisor
>>> configuration.
>>
>> Huh, this sounds weird.
>>
>> What are we "fixing" here?
> 
> Returning ES_VMM_ERROR is misleading/wrong, and panicking doesn't match how the
> kernel handles every other "bad" WRMSR.  How's this for a changelog?
> 
>   For Secure TSC enabled guests, don't panic if the kernel hits a #VC due
>   to attempting to write to GUEST_TSC_FREQ, and instead WARN and drop the
>   write.  The kernel should never write GUEST_TSC_FREQ as it's read-only,
>   but panicking with ES_VMM_ERROR is both misleading (it's entirely
>   reasonable for a VMM to intercept writes to a read-only MSR), and
>   unnecessary, e.g. the kernel eats #GPs with a WARN on every other "bad"
>   WRMSR.

Maybe it should be returning ES_EXCEPTION then instead of ES_VMM_ERROR
and forward a #GP, which is what would have happened if the guest tried
to write to the read-only MSR if it wasn't being intercepted.

I'm still not a fan of intercepting writes to read-only MSRs that are
passed into the guest. If we're trying to replicate bare-metal behavior,
then allowing the write to fail with a #GP seems appropriate.

Thanks,
Tom

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] x86/sev: Improve handling of writes to intercepted GUEST_TSC_FREQ
  2025-07-14 14:59     ` Tom Lendacky
@ 2025-07-14 15:17       ` Sean Christopherson
  2025-07-14 16:16         ` Borislav Petkov
  0 siblings, 1 reply; 15+ messages in thread
From: Sean Christopherson @ 2025-07-14 15:17 UTC (permalink / raw)
  To: Tom Lendacky
  Cc: Borislav Petkov, Nikunj A Dadhania, linux-kernel, x86, tglx,
	mingo, dave.hansen, santosh.shukla

On Mon, Jul 14, 2025, Tom Lendacky wrote:
> On 7/14/25 09:24, Sean Christopherson wrote:
> > On Mon, Jul 14, 2025, Borislav Petkov wrote:
> >> On Fri, Jul 11, 2025 at 09:42:00AM +0530, Nikunj A Dadhania wrote:
> >>> From: Sean Christopherson <seanjc@google.com>
> >>>
> >>> For Secure TSC enabled guests, don't panic when a guest writes to
> >>> intercepted GUEST_TSC_FREQ MSR. Instead, ignore writes to GUEST_TSC_FREQ,
> >>> similar to MSR_IA32_TSC, and log a warning instead.
> >>
> >> Why?
> >>
> >> Nothing should poke at the TSC MSR and those who do, deserve what they get.
> >>
> >>> Only terminate the guest when reading from intercepted GUEST_TSC_FREQ MSR
> >>> with Secure TSC enabled, as this indicates an unexpected hypervisor
> >>> configuration.
> >>
> >> Huh, this sounds weird.
> >>
> >> What are we "fixing" here?
> > 
> > Returning ES_VMM_ERROR is misleading/wrong, and panicking doesn't match how the
> > kernel handles every other "bad" WRMSR.  How's this for a changelog?
> > 
> >   For Secure TSC enabled guests, don't panic if the kernel hits a #VC due
> >   to attempting to write to GUEST_TSC_FREQ, and instead WARN and drop the
> >   write.  The kernel should never write GUEST_TSC_FREQ as it's read-only,
> >   but panicking with ES_VMM_ERROR is both misleading (it's entirely
> >   reasonable for a VMM to intercept writes to a read-only MSR), and
> >   unnecessary, e.g. the kernel eats #GPs with a WARN on every other "bad"
> >   WRMSR.
> 
> Maybe it should be returning ES_EXCEPTION then instead of ES_VMM_ERROR
> and forward a #GP, which is what would have happened if the guest tried
> to write to the read-only MSR if it wasn't being intercepted.
> 
> I'm still not a fan of intercepting writes to read-only MSRs that are
> passed into the guest. If we're trying to replicate bare-metal behavior,
> then allowing the write to fail with a #GP seems appropriate.

The guest cannot dictate VMM behavior.  If the guest side wants to panic, then
so be it, panic.  But don't blame the VMM for taking a conservative approach.

If you want to dictate VMM behavior, then the required behavior needs to be
explicitly documented in an "official" spec, e.g. the GHCB.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] x86/sev: Improve handling of writes to intercepted GUEST_TSC_FREQ
  2025-07-14 15:17       ` Sean Christopherson
@ 2025-07-14 16:16         ` Borislav Petkov
  2025-07-14 16:36           ` Sean Christopherson
  0 siblings, 1 reply; 15+ messages in thread
From: Borislav Petkov @ 2025-07-14 16:16 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Tom Lendacky, Nikunj A Dadhania, linux-kernel, x86, tglx, mingo,
	dave.hansen, santosh.shukla

On Mon, Jul 14, 2025 at 08:17:13AM -0700, Sean Christopherson wrote:
> The guest cannot dictate VMM behavior.  If the guest side wants to panic, then
> so be it, panic.  But don't blame the VMM for taking a conservative approach.
> 
> If you want to dictate VMM behavior, then the required behavior needs to be
> explicitly documented in an "official" spec, e.g. the GHCB.

Ok, so you want to squash the #GP from an attempted write to a MSR into
a warning because this is how the hypervisor has been handling it already for
others. Ok, I guess this is established protocol or whatnot.

Now, why should it panic when a *read* is then attempted?

The APM says:

"Guests that run with Secure TSC enabled may read the GUEST_TSC_FREQ MSR
(C001_0134h) which returns the effective frequency in MHz of the guest view of
TSC. This MSR is read-only and attempting to write the MSR or read it when
outside of a guest with Secure TSC enabled causes a #GP(0) exception."

So what is the established protocol for reading non-existent MSRs?

Also, if secure TSC is enabled, that MSR read should succeed.

The original text in the patch:

"Only terminate the guest when reading from intercepted GUEST_TSC_FREQ MSR
with Secure TSC enabled, as this indicates an unexpected hypervisor
configuration."

doesn't make too much sense to me.

Maybe you need to explain things in detail as virt and I don't understand each
other too much yet.

:-)

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] x86/sev: Improve handling of writes to intercepted GUEST_TSC_FREQ
  2025-07-14 16:16         ` Borislav Petkov
@ 2025-07-14 16:36           ` Sean Christopherson
  2025-07-15  8:37             ` Nikunj A Dadhania
  2025-07-15  8:38             ` Borislav Petkov
  0 siblings, 2 replies; 15+ messages in thread
From: Sean Christopherson @ 2025-07-14 16:36 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Tom Lendacky, Nikunj A Dadhania, linux-kernel, x86, tglx, mingo,
	dave.hansen, santosh.shukla

On Mon, Jul 14, 2025, Borislav Petkov wrote:
> On Mon, Jul 14, 2025 at 08:17:13AM -0700, Sean Christopherson wrote:
> > The guest cannot dictate VMM behavior.  If the guest side wants to panic, then
> > so be it, panic.  But don't blame the VMM for taking a conservative approach.
> > 
> > If you want to dictate VMM behavior, then the required behavior needs to be
> > explicitly documented in an "official" spec, e.g. the GHCB.
> 
> Ok, so you want to squash the #GP from an attempted write to a MSR into
> a warning because this is how the hypervisor has been handling it already for
> others. Ok, I guess this is established protocol or whatnot.

Or as Tom suggested, return ES_EXCEPTION and let the kernel's normal machinery
WARN on the bad WRMSR.

> Now, why should it panic when a *read* is then attempted?

Because as you note below, the MSR read should succeed.  __vc_handle_secure_tsc_msrs()
is invoked if and only if secure TSC is enabled for the guest.  If RDMSR #VCs,
then the hypervisor has decided to intercept GUEST_TSC_FREQ despite enabling and
advertising secure TSC to the guest.  The guest kernel can either continue on
with degraded security (potentially dangerously so) or panic/terminate.  

> The APM says:
> 
> "Guests that run with Secure TSC enabled may read the GUEST_TSC_FREQ MSR
> (C001_0134h) which returns the effective frequency in MHz of the guest view of
> TSC. This MSR is read-only and attempting to write the MSR or read it when
> outside of a guest with Secure TSC enabled causes a #GP(0) exception."
> 
> So what is the established protocol for reading non-existent MSRs?

Looks like Linux-as-a-guest will request emulation from the hypervisor.  What
the hypervisor does is completely unknown, at least as far as the guest is
concerned.  E.g. the hypervisor could return an error (i.e. "inject" a #GP), or
it could provide garbage (on RDMSR) and drop writres.

> Also, if secure TSC is enabled, that MSR read should succeed.
> 
> The original text in the patch:
> 
> "Only terminate the guest when reading from intercepted GUEST_TSC_FREQ MSR
> with Secure TSC enabled, as this indicates an unexpected hypervisor
> configuration."
> 
> doesn't make too much sense to me.
> 
> Maybe you need to explain things in detail as virt and I don't understand each
> other too much yet.
> 
> :-)
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] x86/sev: Improve handling of writes to intercepted GUEST_TSC_FREQ
  2025-07-14 16:36           ` Sean Christopherson
@ 2025-07-15  8:37             ` Nikunj A Dadhania
  2025-07-15  8:43               ` Borislav Petkov
  2025-07-15  8:38             ` Borislav Petkov
  1 sibling, 1 reply; 15+ messages in thread
From: Nikunj A Dadhania @ 2025-07-15  8:37 UTC (permalink / raw)
  To: Sean Christopherson, Borislav Petkov
  Cc: Tom Lendacky, linux-kernel, x86, tglx, mingo, dave.hansen,
	santosh.shukla

Sean Christopherson <seanjc@google.com> writes:

> On Mon, Jul 14, 2025, Borislav Petkov wrote:
>> On Mon, Jul 14, 2025 at 08:17:13AM -0700, Sean Christopherson wrote:
>> 
>> The original text in the patch:
>> 
>> "Only terminate the guest when reading from intercepted GUEST_TSC_FREQ MSR
>> with Secure TSC enabled, as this indicates an unexpected hypervisor
>> configuration."
>> 
>> doesn't make too much sense to me.
>> 
>> Maybe you need to explain things in detail as virt and I don't understand each
>> other too much yet.

How about the below changelog:

  Currently, when a Secure TSC enabled SNP guest attempts to write to
  the intercepted GUEST_TSC_FREQ MSR (a read-only MSR), the guest kernel
  #VC handler terminates the SNP guest by returning ES_VMM_ERROR. This
  response incorrectly implies a VMM configuration error, when in fact
  it's a valid VMM configuration to intercept writes to read-only MSRs,
  unless explicitly documented.

  Modify the intercepted GUEST_TSC_FREQ MSR #VC handler to ignore writes
  instead of terminating the guest. Since GUEST_TSC_FREQ is a guest-only
  MSR, ignoring writes directly (rather than forwarding to the VMM and
  handling the resulting #GP) eliminates a round trip to the VMM. Add a
  WARN_ONCE to log the incident, as well-behaved guest kernels should
  never attempt to write to this read-only MSR.

  However, continue to terminate the guest(via ES_VMM_ERROR) when
  reading from intercepted GUEST_TSC_FREQ MSR with Secure TSC enabled,
  as intercepted reads indicate an improper VMM configuration for Secure
  TSC enabled SNP guests.

Regards,
Nikunj

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] x86/sev: Improve handling of writes to intercepted GUEST_TSC_FREQ
  2025-07-14 16:36           ` Sean Christopherson
  2025-07-15  8:37             ` Nikunj A Dadhania
@ 2025-07-15  8:38             ` Borislav Petkov
  2025-07-15  9:13               ` Nikunj A Dadhania
  1 sibling, 1 reply; 15+ messages in thread
From: Borislav Petkov @ 2025-07-15  8:38 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Tom Lendacky, Nikunj A Dadhania, linux-kernel, x86, tglx, mingo,
	dave.hansen, santosh.shukla

On Mon, Jul 14, 2025 at 09:36:04AM -0700, Sean Christopherson wrote:
> Or as Tom suggested, return ES_EXCEPTION and let the kernel's normal machinery
> WARN on the bad WRMSR.

Ack.

> Because as you note below, the MSR read should succeed.  __vc_handle_secure_tsc_msrs()
> is invoked if and only if secure TSC is enabled for the guest.  If RDMSR #VCs,
> then the hypervisor has decided to intercept GUEST_TSC_FREQ despite enabling and
> advertising secure TSC to the guest.  The guest kernel can either continue on
> with degraded security (potentially dangerously so) or panic/terminate.  

Aha, I guess we do want to panic here...

> > The APM says:
> > 
> > "Guests that run with Secure TSC enabled may read the GUEST_TSC_FREQ MSR
> > (C001_0134h) which returns the effective frequency in MHz of the guest view of
> > TSC. This MSR is read-only and attempting to write the MSR or read it when
> > outside of a guest with Secure TSC enabled causes a #GP(0) exception."
> > 
> > So what is the established protocol for reading non-existent MSRs?
> 
> Looks like Linux-as-a-guest will request emulation from the hypervisor.  What
> the hypervisor does is completely unknown, at least as far as the guest is
> concerned.  E.g. the hypervisor could return an error (i.e. "inject" a #GP), or
> it could provide garbage (on RDMSR) and drop writres.

I see.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] x86/sev: Improve handling of writes to intercepted GUEST_TSC_FREQ
  2025-07-15  8:37             ` Nikunj A Dadhania
@ 2025-07-15  8:43               ` Borislav Petkov
  2025-07-15  8:58                 ` Nikunj A Dadhania
  0 siblings, 1 reply; 15+ messages in thread
From: Borislav Petkov @ 2025-07-15  8:43 UTC (permalink / raw)
  To: Nikunj A Dadhania
  Cc: Sean Christopherson, Tom Lendacky, linux-kernel, x86, tglx, mingo,
	dave.hansen, santosh.shukla

On Tue, Jul 15, 2025 at 08:37:38AM +0000, Nikunj A Dadhania wrote:
>   Currently, when a Secure TSC enabled SNP guest attempts to write to
>   the intercepted GUEST_TSC_FREQ MSR (a read-only MSR), the guest kernel
>   #VC handler terminates the SNP guest by returning ES_VMM_ERROR. This
>   response incorrectly implies a VMM configuration error, when in fact
>   it's a valid VMM configuration to intercept writes to read-only MSRs,

Not only valid - it is the usual thing the HV does with MSRs IMHO.

>   unless explicitly documented.
> 
>   Modify the intercepted GUEST_TSC_FREQ MSR #VC handler to ignore writes
>   instead of terminating the guest. Since GUEST_TSC_FREQ is a guest-only
>   MSR, ignoring writes directly (rather than forwarding to the VMM and
>   handling the resulting #GP) eliminates a round trip to the VMM.

Probably.

But I think the main point here is that this is the default action the HV
does.

> Add a
>   WARN_ONCE to log the incident, as well-behaved guest kernels should
>   never attempt to write to this read-only MSR.
> 
>   However, continue to terminate the guest(via ES_VMM_ERROR) when

ES_EXCEPTION

>   reading from intercepted GUEST_TSC_FREQ MSR with Secure TSC enabled,
>   as intercepted reads indicate an improper VMM configuration for Secure
>   TSC enabled SNP guests.

It is getting close to the gist of what we talked yesterday tho.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] x86/sev: Improve handling of writes to intercepted GUEST_TSC_FREQ
  2025-07-15  8:43               ` Borislav Petkov
@ 2025-07-15  8:58                 ` Nikunj A Dadhania
  0 siblings, 0 replies; 15+ messages in thread
From: Nikunj A Dadhania @ 2025-07-15  8:58 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Sean Christopherson, Tom Lendacky, linux-kernel, x86, tglx, mingo,
	dave.hansen, santosh.shukla

Borislav Petkov <bp@alien8.de> writes:

> On Tue, Jul 15, 2025 at 08:37:38AM +0000, Nikunj A Dadhania wrote:
>>   Currently, when a Secure TSC enabled SNP guest attempts to write to
>>   the intercepted GUEST_TSC_FREQ MSR (a read-only MSR), the guest kernel
>>   #VC handler terminates the SNP guest by returning ES_VMM_ERROR. This
>>   response incorrectly implies a VMM configuration error, when in fact
>>   it's a valid VMM configuration to intercept writes to read-only MSRs,
>
> Not only valid - it is the usual thing the HV does with MSRs IMHO.

Right, will update

>
>>   unless explicitly documented.
>> 
>>   Modify the intercepted GUEST_TSC_FREQ MSR #VC handler to ignore writes
>>   instead of terminating the guest. Since GUEST_TSC_FREQ is a guest-only
>>   MSR, ignoring writes directly (rather than forwarding to the VMM and
>>   handling the resulting #GP) eliminates a round trip to the VMM.
>
> Probably.
>
> But I think the main point here is that this is the default action the HV
> does.

Correct, to adhere to that behaviour, I had sent the following patch
earlier [1]. If GUEST_TSC_FREQ is intercepted by VMM:

MSR read will terminate the guest, same behavior as earlier.

MSR write will be passed to the VMM and VMM will inject the GP# back.

>
>> Add a
>>   WARN_ONCE to log the incident, as well-behaved guest kernels should
>>   never attempt to write to this read-only MSR.
>> 
>>   However, continue to terminate the guest(via ES_VMM_ERROR) when
>
> ES_EXCEPTION

Are you suggesting to change the intercepted GUEST_TSC_FREQ MSR read
behaviour from panic to ES_EXCEPTION?

>
>>   reading from intercepted GUEST_TSC_FREQ MSR with Secure TSC enabled,
>>   as intercepted reads indicate an improper VMM configuration for Secure
>>   TSC enabled SNP guests.
>
> It is getting close to the gist of what we talked yesterday tho.

Ack

Regards,
Nikunj

1. https://lore.kernel.org/kvm/85h5zkuxa2.fsf@amd.com/

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] x86/sev: Improve handling of writes to intercepted GUEST_TSC_FREQ
  2025-07-15  8:38             ` Borislav Petkov
@ 2025-07-15  9:13               ` Nikunj A Dadhania
  2025-07-15  9:44                 ` Borislav Petkov
  2025-07-15 12:47                 ` Tom Lendacky
  0 siblings, 2 replies; 15+ messages in thread
From: Nikunj A Dadhania @ 2025-07-15  9:13 UTC (permalink / raw)
  To: Borislav Petkov, Sean Christopherson
  Cc: Tom Lendacky, linux-kernel, x86, tglx, mingo, dave.hansen,
	santosh.shukla

Borislav Petkov <bp@alien8.de> writes:

> On Mon, Jul 14, 2025 at 09:36:04AM -0700, Sean Christopherson wrote:
>> Or as Tom suggested, return ES_EXCEPTION and let the kernel's normal machinery
>> WARN on the bad WRMSR.
>
> Ack.

That will panic the SNP guest instead of #GP:

root@ubuntu:~# wrmsr 0xc0010134 0
[   20.804335] ------------[ cut here ]------------
[   20.804336] WARNING: arch/x86/coco/sev/vc-handle.c:383 at vc_handle_exitcode.part.0+0xc1b/0x1090, CPU#0: wrmsr/607
...
[   20.804507] SEV: Unsupported exception in #VC instruction emulation - can't continue
[   20.804508] ------------[ cut here ]------------
[   20.804508] kernel BUG at arch/x86/coco/sev/vc-handle.c:123!
[   20.804514] Oops: invalid opcode: 0000 [#1] SMP NOPTI

Regards,
Nikunj

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] x86/sev: Improve handling of writes to intercepted GUEST_TSC_FREQ
  2025-07-15  9:13               ` Nikunj A Dadhania
@ 2025-07-15  9:44                 ` Borislav Petkov
  2025-07-15 12:47                 ` Tom Lendacky
  1 sibling, 0 replies; 15+ messages in thread
From: Borislav Petkov @ 2025-07-15  9:44 UTC (permalink / raw)
  To: Nikunj A Dadhania
  Cc: Sean Christopherson, Tom Lendacky, linux-kernel, x86, tglx, mingo,
	dave.hansen, santosh.shukla

On Tue, Jul 15, 2025 at 09:13:10AM +0000, Nikunj A Dadhania wrote:
> That will panic the SNP guest instead of #GP:
> 
> root@ubuntu:~# wrmsr 0xc0010134 0
> [   20.804335] ------------[ cut here ]------------
> [   20.804336] WARNING: arch/x86/coco/sev/vc-handle.c:383 at vc_handle_exitcode.part.0+0xc1b/0x1090, CPU#0: wrmsr/607
> ...
> [   20.804507] SEV: Unsupported exception in #VC instruction emulation - can't continue
> [   20.804508] ------------[ cut here ]------------
> [   20.804508] kernel BUG at arch/x86/coco/sev/vc-handle.c:123!
> [   20.804514] Oops: invalid opcode: 0000 [#1] SMP NOPTI

You need to make trapnr become X86_TRAP_GP in vc_forward_exception() so that
the GP handler is called.

Or wait for Tom to wake up to double-check his suggestion... :-)

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] x86/sev: Improve handling of writes to intercepted GUEST_TSC_FREQ
  2025-07-15  9:13               ` Nikunj A Dadhania
  2025-07-15  9:44                 ` Borislav Petkov
@ 2025-07-15 12:47                 ` Tom Lendacky
  2025-07-16  6:09                   ` Nikunj A Dadhania
  1 sibling, 1 reply; 15+ messages in thread
From: Tom Lendacky @ 2025-07-15 12:47 UTC (permalink / raw)
  To: Nikunj A Dadhania, Borislav Petkov, Sean Christopherson
  Cc: linux-kernel, x86, tglx, mingo, dave.hansen, santosh.shukla

On 7/15/25 04:13, Nikunj A Dadhania wrote:
> Borislav Petkov <bp@alien8.de> writes:
> 
>> On Mon, Jul 14, 2025 at 09:36:04AM -0700, Sean Christopherson wrote:
>>> Or as Tom suggested, return ES_EXCEPTION and let the kernel's normal machinery
>>> WARN on the bad WRMSR.
>>
>> Ack.
> 
> That will panic the SNP guest instead of #GP:
> 
> root@ubuntu:~# wrmsr 0xc0010134 0
> [   20.804335] ------------[ cut here ]------------
> [   20.804336] WARNING: arch/x86/coco/sev/vc-handle.c:383 at vc_handle_exitcode.part.0+0xc1b/0x1090, CPU#0: wrmsr/607
> ...
> [   20.804507] SEV: Unsupported exception in #VC instruction emulation - can't continue
> [   20.804508] ------------[ cut here ]------------
> [   20.804508] kernel BUG at arch/x86/coco/sev/vc-handle.c:123!
> [   20.804514] Oops: invalid opcode: 0000 [#1] SMP NOPTI

Did you fill in the context with the #GP, i.e., ctxt->fi.vector and
ctxt->fi.error_code?

Thanks,
Tom

> 
> Regards,
> Nikunj

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] x86/sev: Improve handling of writes to intercepted GUEST_TSC_FREQ
  2025-07-15 12:47                 ` Tom Lendacky
@ 2025-07-16  6:09                   ` Nikunj A Dadhania
  0 siblings, 0 replies; 15+ messages in thread
From: Nikunj A Dadhania @ 2025-07-16  6:09 UTC (permalink / raw)
  To: Tom Lendacky, Borislav Petkov, Sean Christopherson
  Cc: linux-kernel, x86, tglx, mingo, dave.hansen, santosh.shukla

Tom Lendacky <thomas.lendacky@amd.com> writes:

> On 7/15/25 04:13, Nikunj A Dadhania wrote:
>> Borislav Petkov <bp@alien8.de> writes:
>> 
>>> On Mon, Jul 14, 2025 at 09:36:04AM -0700, Sean Christopherson wrote:
>>>> Or as Tom suggested, return ES_EXCEPTION and let the kernel's normal machinery
>>>> WARN on the bad WRMSR.
>>>
>>> Ack.
>> 
>> That will panic the SNP guest instead of #GP:
>> 
>> root@ubuntu:~# wrmsr 0xc0010134 0
>> [   20.804335] ------------[ cut here ]------------
>> [   20.804336] WARNING: arch/x86/coco/sev/vc-handle.c:383 at vc_handle_exitcode.part.0+0xc1b/0x1090, CPU#0: wrmsr/607
>> ...
>> [   20.804507] SEV: Unsupported exception in #VC instruction emulation - can't continue
>> [   20.804508] ------------[ cut here ]------------
>> [   20.804508] kernel BUG at arch/x86/coco/sev/vc-handle.c:123!
>> [   20.804514] Oops: invalid opcode: 0000 [#1] SMP NOPTI
>
> Did you fill in the context with the #GP, i.e., ctxt->fi.vector and
> ctxt->fi.error_code?

Ah OK, I didn't know that; after populating the X86_TRAP_GP, SNP guest
does not panic anymore.

+       if (WARN_ON_ONCE(write)) {
+               ctxt->fi.vector = X86_TRAP_GP;
+               ctxt->fi.error_code = 0;
+               return ES_EXCEPTION;
+       }



$ wrmsr 0xc0010134 100
wrmsr: CPU 0 cannot set MSR 0x00000001 to 0x0000000000000064

$ wrmsr 0x10 100
wrmsr: CPU 0 cannot set MSR 0x00000010 to 0x0000000000000064

I have sent an updated patch.

Regards,
Nikunj

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2025-07-16  6:10 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-11  4:12 [PATCH] x86/sev: Improve handling of writes to intercepted GUEST_TSC_FREQ Nikunj A Dadhania
2025-07-14 10:44 ` Borislav Petkov
2025-07-14 14:24   ` Sean Christopherson
2025-07-14 14:59     ` Tom Lendacky
2025-07-14 15:17       ` Sean Christopherson
2025-07-14 16:16         ` Borislav Petkov
2025-07-14 16:36           ` Sean Christopherson
2025-07-15  8:37             ` Nikunj A Dadhania
2025-07-15  8:43               ` Borislav Petkov
2025-07-15  8:58                 ` Nikunj A Dadhania
2025-07-15  8:38             ` Borislav Petkov
2025-07-15  9:13               ` Nikunj A Dadhania
2025-07-15  9:44                 ` Borislav Petkov
2025-07-15 12:47                 ` Tom Lendacky
2025-07-16  6:09                   ` Nikunj A Dadhania

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).