Re: [PATCH v3] x86/sev: Improve handling of writes to intercepted TSC MSRs

[Tom Lendacky <thomas.lendacky@amd.com>]

On 7/22/25 02:48, 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 response
> incorrectly implies a VMM configuration error, when in fact it is the usual
> VMM configuration to intercept writes to read-only MSRs, unless explicitly
> documented.
> 
> Modify the intercepted TSC MSR #VC handling:
> * Write to GUEST_TSC_FREQ will generate a #GP instead of terminating the
>   guest
> * Write to MSR_IA32_TSC will generate a #GP instead of silently ignoring it
> 
> However, continue to terminate the guest 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.
> 
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Nikunj A Dadhania <nikunj@amd.com>

Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>

> ---
> 
> v3:
> * Drop pt_regs pointer as it is already part of the es_em_ctxt struct (Tom)
> * Added Sean's suggested-by instead of authored-by (Sean)
> * Removed WARN_ON_ONCE as kernel already warns via ex_handler_msr()
>   for bare wrmsrq() calls (Sean)
> 
>  arch/x86/coco/sev/vc-handle.c | 31 ++++++++++++++++---------------
>  1 file changed, 16 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/x86/coco/sev/vc-handle.c b/arch/x86/coco/sev/vc-handle.c
> index faf1fce89ed4..6333eb6b23c2 100644
> --- a/arch/x86/coco/sev/vc-handle.c
> +++ b/arch/x86/coco/sev/vc-handle.c
> @@ -371,29 +371,30 @@ static enum es_result __vc_handle_msr_caa(struct pt_regs *regs, bool write)
>   * executing with Secure TSC enabled, so special handling is required for
>   * accesses of MSR_IA32_TSC and MSR_AMD64_GUEST_TSC_FREQ.
>   */
> -static enum es_result __vc_handle_secure_tsc_msrs(struct pt_regs *regs, bool write)
> +static enum es_result __vc_handle_secure_tsc_msrs(struct es_em_ctxt *ctxt, bool write)
>  {
> +	struct pt_regs *regs = ctxt->regs;
>  	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. Generate
> +	 * a #GP on all writes.
>  	 */
> -	if (regs->cx == MSR_AMD64_GUEST_TSC_FREQ)
> -		return ES_VMM_ERROR;
> +	if (write) {
> +		ctxt->fi.vector = X86_TRAP_GP;
> +		ctxt->fi.error_code = 0;
> +		return ES_EXCEPTION;
> +	}
>  
>  	/*
> -	 * 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);
> @@ -416,7 +417,7 @@ 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_secure_tsc_msrs(regs, write);
> +			return __vc_handle_secure_tsc_msrs(ctxt, write);
>  		break;
>  	default:
>  		break;
> 
> base-commit: 34481698fd9c3c21425ab744e9e15dc2ce3b1b85