Re: [PATCH v13 04/13] x86/sev: Change TSC MSR behavior for Secure TSC enabled guests

[Xiaoyao Li <xiaoyao.li@intel.com>]

On 10/24/2024 2:24 PM, Nikunj A. Dadhania wrote:
> 
> 
> On 10/23/2024 8:55 AM, Xiaoyao Li wrote:
>> On 10/21/2024 1:51 PM, Nikunj A Dadhania wrote:
>>> Secure TSC enabled guests should not write to MSR_IA32_TSC(10H) register as
>>> the subsequent TSC value reads are undefined. MSR_IA32_TSC read/write
>>> accesses should not exit to the hypervisor for such guests.
>>>
>>> Accesses to MSR_IA32_TSC needs special handling in the #VC handler for the
>>> guests with Secure TSC enabled. Writes to MSR_IA32_TSC should be ignored,
>>> and reads of MSR_IA32_TSC should return the result of the RDTSC
>>> instruction.
>>>
>>> Signed-off-by: Nikunj A Dadhania <nikunj@amd.com>
>>> Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
>>> Tested-by: Peter Gonda <pgonda@google.com>
>>> ---
>>>    arch/x86/coco/sev/core.c | 24 ++++++++++++++++++++++++
>>>    1 file changed, 24 insertions(+)
>>>
>>> diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
>>> index 965209067f03..2ad7773458c0 100644
>>> --- a/arch/x86/coco/sev/core.c
>>> +++ b/arch/x86/coco/sev/core.c
>>> @@ -1308,6 +1308,30 @@ static enum es_result vc_handle_msr(struct ghcb *ghcb, struct es_em_ctxt *ctxt)
>>>            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:
>>> +     *
>>> +     * 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.
>>> +     */
>>
>> Why doesn't handle it by returning ES_VMM_ERROR when hypervisor intercepts
>> RD/WR of MSR_IA32_TSC? With SECURE_TSC enabled, it seems not need to be
>> intercepted.
> 
> ES_VMM_ERROR will terminate the guest, which is not the expected behaviour. As
> documented, writes to the MSR is ignored and reads are done using RDTSC.
> 
>> I think the reason is that SNP guest relies on interception to do the ignore
>> behavior for WRMSR in #VC handler because the writing leads to undefined
>> result.
> 
> For legacy and secure guests MSR_IA32_TSC is always intercepted(for both RD/WR).

We cannot make such assumption unless it's enforced by AMD HW.

> Moreover, this is a legacy MSR, RDTSC and RDTSCP is the what modern OSes should
> use. 

Again, this is your assumption and expectation only.

> The idea is the catch any writes to TSC MSR and handle them gracefully.

If SNP guest requires MSR_IA32_TSC being intercepted by hypervisor. It 
should come with a solution that guest kernel can check it certainly, 
just like the patch 5 and patch 6, that they can check the behavior of 
hypervisor.

If there is no clean way for guest to ensure MSR_IA32_TSC is intercepted 
by hypervisor, we at least need add some comment to call out that these 
code replies on the assumption that hypervisor intercepts MSR_IA32_TSC.

>> Then the question is what if the hypervisor doesn't intercept write to
>> MSR_IA32_TSC in the first place?
> 
> I have tried to disable interception of MSR_IA32_TSC, and writes are ignored by
> the HW as well. I would like to continue the current documented HW as per the APM.

I only means the writes are ignored in your testing HW. We don't know 
the result on other SNP-capable HW or future HW, unless it's documented 
in APM.

Current documented behavior is that write leads to a undefined value of 
subsequent read. So we need to avoid write. One solution is to intercept 
write and ignore it, but it depends on hypervisor to intercept it. 
Anther solution would be we fix all the place of writing MSR_IA32_TSC 
for SNP guest in linux.

> Regards,
> Nikunj