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

[Nikunj A. Dadhania]

On 4/5/2023 6:54 PM, Tom Lendacky wrote:
> 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@xxxxxxx>
>>>> ---
>>
>>>> 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.

Should sev-guest also call snp_setup_psp_messaging() ? 

As per the current design:
* snp_secure_tsc_prepare populates platform_data (probably I should rename it snp_prepare_psp_messaging)
* In snp_init_platform_device() platform_data is copied using platform_device_add_data().

sev-guest driver in sev_guest_probe() uses this platform_data populated by snp_secure_tsc_prepare().

> 
> 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.

I understand what you are suggesting, let me think over this and I will come back.

> 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.

We are using snp_assign_vmpck() in the sev-guest driver that is dependent on secrets_gpa, we can get rid of that dependency I think.

>>
>>  
>>> 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?

Yes, that correct. I will implement full support for CC_ATTR_GUEST_SECURE_TSC  in this patch.

Regards
Nikunj