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

[Nikunj A Dadhania <nikunj@amd.com>]

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

> On 7/16/25 00:53, Nikunj A Dadhania wrote:
>> From: Sean Christopherson <seanjc@google.com>
>> 
>> 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
>> 
>> Add a WARN_ONCE to log the incident, as well-behaved SNP guest kernels
>> should never attempt to write to these MSRs.
>> 
>> 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.
>> 
>> Signed-off-by: Sean Christopherson <seanjc@google.com>
>> Co-developed-by: Nikunj A Dadhania <nikunj@amd.com>
>> Signed-off-by: Nikunj A Dadhania <nikunj@amd.com>
>> ---
>>  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..18be9f8bd015 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 pt_regs *regs, bool write,
>> +						  struct es_em_ctxt  *ctxt)
>
> The pt_regs pointer is part of the es_em_ctxt struct, so probably just
> change this to:
>
> static enum es_result __vc_handle_secure_tsc_msrs(struct es_em_ctxt *ctxt, bool write)
>
> and then get the regs pointer from ctxt:
>
> struct pt_regs *regs = ctxt->regs;

Ack

>
>>  {
>>  	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, but WARN to log a kernel bug.
>>  	 */
>> -	if (regs->cx == MSR_AMD64_GUEST_TSC_FREQ)
>> -		return ES_VMM_ERROR;
>> +	if (WARN_ON_ONCE(write)) {
>
> Do we want to capture individual WARNs for each MSR? I guess I'm ok with
> a single WARN for either MSR, but just asking the question.

Right, I had thought about this and concluded that a single WARN should
be fine. The chance of both the MSR being intercepted and written are
pretty low, and anyways a GP# will be generate for all the writes.

Regards,
Nikunj