On 10/24/2024 1:01 PM, Xiaoyao Li wrote: > 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: >>>> 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. Not my assumption, I could not find any reference to MSR_IA32_TSC read/write in the kernel code. > >> 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, Yes, that is my understanding as well. > we at least need add some comment to call > out that these code replies on the assumption that hypervisor > intercepts MSR_IA32_TSC. Sure, I will add a comment. >>> 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. There is no MSR_IA32_TSC write in the kernel code, IMHO, so second solution is taken care. Regards, Nikunj
