Re: [PATCH v2 08/11] x86/sev: Add Secure TSC support for SNP guests

["Nikunj A. Dadhania" <nikunj@amd.com>]

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