Messages in this thread | | | Date | Wed, 5 Apr 2023 13:07:28 +0530 | Subject | Re: [PATCH v2 08/11] x86/sev: Add Secure TSC support for SNP guests | From | "Nikunj A. Dadhania" <> |
| |
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.
> 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.
> 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
| |