Messages in this thread | | | Date | Wed, 5 Apr 2023 08:24:14 -0500 | Subject | Re: [PATCH v2 08/11] x86/sev: Add Secure TSC support for SNP guests | From | Tom Lendacky <> |
| |
On 4/5/23 02:37, Nikunj A. Dadhania wrote: > On 4/4/2023 3:11 AM, Tom Lendacky wrote: >> On 3/26/23 09:46, Nikunj A Dadhania wrote: >>> Add support for Secure TSC in SNP enabled guests. Secure TSC >>> allows guest to securely use RDTSC/RDTSCP instructions as the >>> parameters being used cannot be changed by hypervisor once the >>> guest is launched. >>> >>> During the boot-up of the secondary cpus, SecureTSC enabled >>> guests need to query TSC info from Security processor (PSP). >> >> s/Security processor (PSP)/AMD Secure Processor/ >> >>> This communication channel is encrypted between the security >> >> here as well. >> >>> processor and the guest, hypervisor is just the conduit to >> >> s/hypervisor/the hypervisor/ >> > > Sure, will change commit message > >>> deliver the guest messages to the security processor. Each >>> message is protected with an AEAD (AES-256 GCM). Use minimal >>> GCM library to encrypt/decrypt SNP Guest messages to communicate >>> with the PSP. >>> >>> Moreover, 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. >> >> This seems like a separate patch. > > Sure. > >>> >>> Signed-off-by: Nikunj A Dadhania <nikunj@amd.com> >>> --- > >>> diff --git a/arch/x86/include/asm/sev-guest.h b/arch/x86/include/asm/sev-guest.h >>> index 834cdae302ad..d5ed041ce06b 100644 >>> --- a/arch/x86/include/asm/sev-guest.h >>> +++ b/arch/x86/include/asm/sev-guest.h > >>> +#define SNP_TSC_INFO_REQ_SZ 128 >>> + /* Must be zero filled */ >>> + u8 rsvd[SNP_TSC_INFO_REQ_SZ]; >>> +} __packed; >>> + >>> +struct snp_tsc_info_resp { >>> + /* Status of TSC_INFO message */ >>> + u32 status; >>> + u32 rsvd1; >>> + u64 tsc_scale; >>> + u64 tsc_offset; >>> + u64 tsc_factor; >> >> This should be a u32 ... >> > > Right, will correct. > >>> + u8 rsvd2[96]; >> >> Which then makes this 100. > > Yes. > >>> diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c >>> index 3750e545d688..280aaa1e6aad 100644 >>> --- a/arch/x86/kernel/sev.c >>> +++ b/arch/x86/kernel/sev.c > >>> +bool __init snp_secure_tsc_prepare(void) >>> +{ >>> + platform_data = kzalloc(sizeof(*platform_data), GFP_KERNEL); >>> + if (!platform_data) >>> + return false; >>> + >>> + /* Initialize the PSP channel to send snp messages */ >>> + if (snp_setup_psp_messaging(platform_data)) >>> + sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_UNSUPPORTED); >>> + >>> + if (cc_platform_has(CC_ATTR_GUEST_SECURE_TSC)) { >> >> Should this be checked before allocating memory and calling snp_setup_psp_messaging()? > > No, as we need guest messaging to work without Secure TSC as well.
My take is that the sev-guest driver should establish the key it is going to use at the time the driver is loaded. snp_setup_psp_messaging() should be called by whatever is going to use guest messaging.
Having a generic routine that is accessed by both the kernel and the driver should be the goal. Maybe it is best to only have the vmpck_id be part of any structure and then the appropriate key and sequence number are used based on that id when the request is made.
The sev_guest_platform_data struct can just hold context information (it doesn't need the secrets_gpa any more since everything is now in sev.c which knows what that value is) for whatever is using guest messaging.
> > >> Also, I notice here you use the cc_platform_has() function but in previous >> patches you check sev_status directly. > > For sev-shared.c, cc_platform_has() is not available when compiling boot/compressed. > > I will change the below call site to cc_platfrom_has() after testing. > > arch/x86/kernel/sev.c: if (regs->cx == MSR_IA32_TSC && (sev_status & MSR_AMD64_SNP_SECURE_TSC)) { > >> And you don't implement support for >> CC_ATTR_GUEST_SECURE_TSC until the last patch instead of now. > > This is to make sure that SECURE_TSC is enabled only after the last patch. > As cc_platform_has() is being used at multiple places to enable the feature.
But you terminate long before that in snp_check_features() since you don't update SNP_FEATURES_PRESENT with SECURE_TSC until the last patch, right?
Thanks, Tom
> >> You can't get here until SNP_FEATURES_PRESENT is updated. >> >>> + if (snp_get_tsc_info()) >>> + sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_UNSUPPORTED); >>> + >>> + pr_info("SecureTSC enabled\n"); >>> + } >> >> Blank line. > > Sure > > Regards > Nikunj >
| |