lkml.org 
[lkml]   [2023]   [Apr]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v2 08/11] x86/sev: Add Secure TSC support for SNP guests
From
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
>

\
 
 \ /
  Last update: 2023-04-05 15:24    [W:0.061 / U:0.100 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site