Message ID | 20231128125959.1810039-13-nikunj@amd.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Add Secure TSC support for SNP guests | expand |
On Tue, Nov 28, 2023 at 5:02 AM Nikunj A Dadhania <nikunj@amd.com> wrote: > > The hypervisor should not be intercepting RDTSC/RDTSCP when Secure TSC > is enabled. A #VC exception will be generated if the RDTSC/RDTSCP > instructions are being intercepted. If this should occur and Secure > TSC is enabled, terminate guest execution. > > Signed-off-by: Nikunj A Dadhania <nikunj@amd.com> > --- > arch/x86/kernel/sev-shared.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c > index ccb0915e84e1..6d9ef5897421 100644 > --- a/arch/x86/kernel/sev-shared.c > +++ b/arch/x86/kernel/sev-shared.c > @@ -991,6 +991,16 @@ static enum es_result vc_handle_rdtsc(struct ghcb *ghcb, > bool rdtscp = (exit_code == SVM_EXIT_RDTSCP); > enum es_result ret; > > + /* > + * RDTSC and RDTSCP should not be intercepted when Secure TSC is > + * enabled. Terminate the SNP guest when the interception is enabled. > + * This file is included from kernel/sev.c and boot/compressed/sev.c, > + * use sev_status here as cc_platform_has() is not available when > + * compiling boot/compressed/sev.c. > + */ > + if (sev_status & MSR_AMD64_SNP_SECURE_TSC) > + return ES_VMM_ERROR; Is this not a cc_platform_has situation? I don't recall how the conversation shook out for TDX's forcing X86_FEATURE_TSC_RELIABLE versus having a cc_attr_secure_tsc > + > ret = sev_es_ghcb_hv_call(ghcb, ctxt, exit_code, 0, 0); > if (ret != ES_OK) > return ret; > -- > 2.34.1 >
On 12/5/2023 10:46 PM, Dionna Amalie Glaze wrote: > On Tue, Nov 28, 2023 at 5:02 AM Nikunj A Dadhania <nikunj@amd.com> wrote: >> >> The hypervisor should not be intercepting RDTSC/RDTSCP when Secure TSC >> is enabled. A #VC exception will be generated if the RDTSC/RDTSCP >> instructions are being intercepted. If this should occur and Secure >> TSC is enabled, terminate guest execution. >> >> Signed-off-by: Nikunj A Dadhania <nikunj@amd.com> >> --- >> arch/x86/kernel/sev-shared.c | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c >> index ccb0915e84e1..6d9ef5897421 100644 >> --- a/arch/x86/kernel/sev-shared.c >> +++ b/arch/x86/kernel/sev-shared.c >> @@ -991,6 +991,16 @@ static enum es_result vc_handle_rdtsc(struct ghcb *ghcb, >> bool rdtscp = (exit_code == SVM_EXIT_RDTSCP); >> enum es_result ret; >> >> + /* >> + * RDTSC and RDTSCP should not be intercepted when Secure TSC is >> + * enabled. Terminate the SNP guest when the interception is enabled. >> + * This file is included from kernel/sev.c and boot/compressed/sev.c, >> + * use sev_status here as cc_platform_has() is not available when >> + * compiling boot/compressed/sev.c. >> + */ >> + if (sev_status & MSR_AMD64_SNP_SECURE_TSC) >> + return ES_VMM_ERROR; > > Is this not a cc_platform_has situation? I don't recall how the > conversation shook out for TDX's forcing X86_FEATURE_TSC_RELIABLE > versus having a cc_attr_secure_tsc For SNP, SecureTSC is an opt-in feature. AFAIU, for TDX the feature is turned on by default. So SNP guests need to check if the VMM has enabled the feature before moving forward with SecureTSC initializations. The idea was to have some generic name instead of AMD specific SecureTSC (cc_attr_secure_tsc), and I had sought comments from Kirill [1]. After that discussion I have added a synthetic flag for Secure TSC[2]. Regards Nikunj 1. https://lore.kernel.org/lkml/55de810b-66f9-49e3-8459-b7cac1532a0c@amd.com/ 2. https://lore.kernel.org/lkml/20231128125959.1810039-10-nikunj@amd.com/
> >> + if (sev_status & MSR_AMD64_SNP_SECURE_TSC) > >> + return ES_VMM_ERROR; > > > > Is this not a cc_platform_has situation? I don't recall how the > > conversation shook out for TDX's forcing X86_FEATURE_TSC_RELIABLE > > versus having a cc_attr_secure_tsc > > For SNP, SecureTSC is an opt-in feature. AFAIU, for TDX the feature is > turned on by default. So SNP guests need to check if the VMM has enabled > the feature before moving forward with SecureTSC initializations. > > The idea was to have some generic name instead of AMD specific SecureTSC > (cc_attr_secure_tsc), and I had sought comments from Kirill [1]. After > that discussion I have added a synthetic flag for Secure TSC[2]. > So with regards to [2], this sev_status flag check should be cpu_has_feature(X86_FEATURE_SNP_SECURE_TSC)? I'm not sure if that's available in early boot where this code is used, so if it isn't, probably that's worth a comment.
On 12/7/2023 12:15 AM, Dionna Amalie Glaze wrote: >>>> + if (sev_status & MSR_AMD64_SNP_SECURE_TSC) >>>> + return ES_VMM_ERROR; >>> >>> Is this not a cc_platform_has situation? I don't recall how the >>> conversation shook out for TDX's forcing X86_FEATURE_TSC_RELIABLE >>> versus having a cc_attr_secure_tsc >> >> For SNP, SecureTSC is an opt-in feature. AFAIU, for TDX the feature is >> turned on by default. So SNP guests need to check if the VMM has enabled >> the feature before moving forward with SecureTSC initializations. >> >> The idea was to have some generic name instead of AMD specific SecureTSC >> (cc_attr_secure_tsc), and I had sought comments from Kirill [1]. After >> that discussion I have added a synthetic flag for Secure TSC[2]. >> > > So with regards to [2], this sev_status flag check should be > cpu_has_feature(X86_FEATURE_SNP_SECURE_TSC)? I'm not sure if that's > available in early boot where this code is used, so if it isn't, > probably that's worth a comment. Right, I will update the comment. Regards Nikunj
diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c index ccb0915e84e1..6d9ef5897421 100644 --- a/arch/x86/kernel/sev-shared.c +++ b/arch/x86/kernel/sev-shared.c @@ -991,6 +991,16 @@ static enum es_result vc_handle_rdtsc(struct ghcb *ghcb, bool rdtscp = (exit_code == SVM_EXIT_RDTSCP); enum es_result ret; + /* + * RDTSC and RDTSCP should not be intercepted when Secure TSC is + * enabled. Terminate the SNP guest when the interception is enabled. + * This file is included from kernel/sev.c and boot/compressed/sev.c, + * use sev_status here as cc_platform_has() is not available when + * compiling boot/compressed/sev.c. + */ + if (sev_status & MSR_AMD64_SNP_SECURE_TSC) + return ES_VMM_ERROR; + ret = sev_es_ghcb_hv_call(ghcb, ctxt, exit_code, 0, 0); if (ret != ES_OK) return ret;
The hypervisor should not be intercepting RDTSC/RDTSCP when Secure TSC is enabled. A #VC exception will be generated if the RDTSC/RDTSCP instructions are being intercepted. If this should occur and Secure TSC is enabled, terminate guest execution. Signed-off-by: Nikunj A Dadhania <nikunj@amd.com> --- arch/x86/kernel/sev-shared.c | 10 ++++++++++ 1 file changed, 10 insertions(+)