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

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

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