On Thu, Feb 15, 2024 at 05:01:22PM +0530, 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 "CPUs" > need to query TSC info from AMD Security Processor. This communication > channel is encrypted between the AMD Security Processor and the guest, > the hypervisor is just the conduit to deliver the guest messages to > the AMD Security Processor. Each message is protected with an > AEAD (AES-256 GCM). Use minimal AES GCM library to encrypt/decrypt SNP > Guest messages to communicate with the PSP. > > Use the guest enc_init hook to fetch SNP TSC info from the AMD Security > Processor and initialize the snp_tsc_scale and snp_tsc_offset. During > secondary CPU initialization set VMSA fields GUEST_TSC_SCALE (offset 2F0h) > and GUEST_TSC_OFFSET(offset 2F8h) with snp_tsc_scale and snp_tsc_offset > respectively. > > Signed-off-by: Nikunj A Dadhania <nikunj@xxxxxxx> > Tested-by: Peter Gonda <pgonda@xxxxxxxxxx> > --- > arch/x86/include/asm/sev-common.h | 1 + > arch/x86/include/asm/sev.h | 23 +++++++ > arch/x86/include/asm/svm.h | 6 +- > arch/x86/kernel/sev.c | 107 ++++++++++++++++++++++++++++-- > arch/x86/mm/mem_encrypt_amd.c | 6 ++ > 5 files changed, 134 insertions(+), 9 deletions(-) > > diff --git a/arch/x86/include/asm/sev-common.h b/arch/x86/include/asm/sev-common.h > index b463fcbd4b90..6adc8e27feeb 100644 > --- a/arch/x86/include/asm/sev-common.h > +++ b/arch/x86/include/asm/sev-common.h > @@ -159,6 +159,7 @@ struct snp_psc_desc { > #define GHCB_TERM_NOT_VMPL0 3 /* SNP guest is not running at VMPL-0 */ > #define GHCB_TERM_CPUID 4 /* CPUID-validation failure */ > #define GHCB_TERM_CPUID_HV 5 /* CPUID failure during hypervisor fallback */ > +#define GHCB_TERM_SECURE_TSC 6 /* Secure TSC initialization failed */ > > #define GHCB_RESP_CODE(v) ((v) & GHCB_MSR_INFO_MASK) > > diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h > index d950a3ac5694..16bf5afa7731 100644 > --- a/arch/x86/include/asm/sev.h > +++ b/arch/x86/include/asm/sev.h > @@ -170,6 +170,8 @@ enum msg_type { > SNP_MSG_ABSORB_RSP, > SNP_MSG_VMRK_REQ, > SNP_MSG_VMRK_RSP, <-- Pls leave an empty newline here to denote that there's a hole in the define numbers. Alternatively, you can add the missing ones too. > + SNP_MSG_TSC_INFO_REQ = 17, > + SNP_MSG_TSC_INFO_RSP, > > SNP_MSG_TYPE_MAX > }; > @@ -214,6 +216,23 @@ struct sev_guest_platform_data { > struct snp_req_data input; > }; > > +#define SNP_TSC_INFO_REQ_SZ 128 > + > +struct snp_tsc_info_req { > + /* Must be zero filled */ Instead of adding a comment which people might very likely miss, add a check for that array to warn when it is not zeroed. > + u8 rsvd[SNP_TSC_INFO_REQ_SZ]; > +} __packed; > + > +struct snp_tsc_info_resp { > + /* Status of TSC_INFO message */ The other struct members don't need a comment? > + u32 status; > + u32 rsvd1; > + u64 tsc_scale; > + u64 tsc_offset; > + u32 tsc_factor; > + u8 rsvd2[100]; > +} __packed; > + > struct snp_guest_dev { > struct device *dev; > struct miscdevice misc; > @@ -233,6 +252,7 @@ struct snp_guest_dev { > struct snp_report_req report; > struct snp_derived_key_req derived_key; > struct snp_ext_report_req ext_report; > + struct snp_tsc_info_req tsc_info; > } req; > unsigned int vmpck_id; > }; > @@ -370,6 +390,8 @@ static inline void *alloc_shared_pages(size_t sz) > > return page_address(page); > } > + > +void __init snp_secure_tsc_prepare(void); > #else > static inline void sev_es_ist_enter(struct pt_regs *regs) { } > static inline void sev_es_ist_exit(void) { } > @@ -404,6 +426,7 @@ static inline int snp_send_guest_request(struct snp_guest_dev *dev, struct snp_g > struct snp_guest_request_ioctl *rio) { return 0; } > static inline void free_shared_pages(void *buf, size_t sz) { } > static inline void *alloc_shared_pages(size_t sz) { return NULL; } > +static inline void __init snp_secure_tsc_prepare(void) { } > #endif > > #ifdef CONFIG_KVM_AMD_SEV > diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h > index 87a7b917d30e..3a8294bbd109 100644 > --- a/arch/x86/include/asm/svm.h > +++ b/arch/x86/include/asm/svm.h > @@ -410,7 +410,9 @@ struct sev_es_save_area { > u8 reserved_0x298[80]; > u32 pkru; > u32 tsc_aux; > - u8 reserved_0x2f0[24]; > + u64 tsc_scale; > + u64 tsc_offset; > + u8 reserved_0x300[8]; > u64 rcx; > u64 rdx; > u64 rbx; > @@ -542,7 +544,7 @@ static inline void __unused_size_checks(void) > BUILD_BUG_RESERVED_OFFSET(sev_es_save_area, 0x1c0); > BUILD_BUG_RESERVED_OFFSET(sev_es_save_area, 0x248); > BUILD_BUG_RESERVED_OFFSET(sev_es_save_area, 0x298); > - BUILD_BUG_RESERVED_OFFSET(sev_es_save_area, 0x2f0); > + BUILD_BUG_RESERVED_OFFSET(sev_es_save_area, 0x300); > BUILD_BUG_RESERVED_OFFSET(sev_es_save_area, 0x320); > BUILD_BUG_RESERVED_OFFSET(sev_es_save_area, 0x380); > BUILD_BUG_RESERVED_OFFSET(sev_es_save_area, 0x3f0); > diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c > index a9c1efd6d4e3..20a1e50b7638 100644 > --- a/arch/x86/kernel/sev.c > +++ b/arch/x86/kernel/sev.c > @@ -75,6 +75,10 @@ static u64 sev_hv_features __ro_after_init; > /* Secrets page physical address from the CC blob */ > static u64 secrets_pa __ro_after_init; > > +/* Secure TSC values read using TSC_INFO SNP Guest request */ > +static u64 snp_tsc_scale __ro_after_init; > +static u64 snp_tsc_offset __ro_after_init; > + > /* #VC handler runtime per-CPU data */ > struct sev_es_runtime_data { > struct ghcb ghcb_page; > @@ -956,6 +960,83 @@ void snp_guest_cmd_unlock(void) > } > EXPORT_SYMBOL_GPL(snp_guest_cmd_unlock); > > +static struct snp_guest_dev tsc_snp_dev __initdata; > + > +static int __snp_send_guest_request(struct snp_guest_dev *snp_dev, struct snp_guest_req *req, > + struct snp_guest_request_ioctl *rio); > + Pls design your code without the need for a forward declaration. > +static int __init snp_get_tsc_info(void) > +{ > + struct snp_tsc_info_req *tsc_req = &tsc_snp_dev.req.tsc_info; > + static u8 buf[SNP_TSC_INFO_REQ_SZ + AUTHTAG_LEN]; > + struct snp_guest_request_ioctl rio; > + struct snp_tsc_info_resp tsc_resp; > + struct snp_guest_req req; > + int rc, resp_len; > + > + /* > + * The intermediate response buffer is used while decrypting the > + * response payload. Make sure that it has enough space to cover the > + * authtag. > + */ > + resp_len = sizeof(tsc_resp) + AUTHTAG_LEN; > + if (sizeof(buf) < resp_len) > + return -EINVAL; Huh, those both are static buffers. Such checks are done with BUILD_BUG_ON. > + memset(tsc_req, 0, sizeof(*tsc_req)); > + memset(&req, 0, sizeof(req)); > + memset(&rio, 0, sizeof(rio)); > + memset(buf, 0, sizeof(buf)); > + > + if (!snp_assign_vmpck(&tsc_snp_dev, 0)) > + return -EINVAL; Do that before the memsetting. > + > + /* Initialize the PSP channel to send snp messages */ > + rc = snp_setup_psp_messaging(&tsc_snp_dev); > + if (rc) > + return rc; > + > + req.msg_version = MSG_HDR_VER; > + req.msg_type = SNP_MSG_TSC_INFO_REQ; > + req.vmpck_id = tsc_snp_dev.vmpck_id; > + req.req_buf = tsc_req; > + req.req_sz = sizeof(*tsc_req); > + req.resp_buf = buf; > + req.resp_sz = resp_len; > + req.exit_code = SVM_VMGEXIT_GUEST_REQUEST; > + > + rc = __snp_send_guest_request(&tsc_snp_dev, &req, &rio); The changes to *snp_send_guest_request are unrelated to the secure TSC enablement. Pls do them in a pre-patch. Ok, I'm going to stop here and give you a chance to work in all the review feedback and send a new revision. Thx. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette