On 12/16/2024 9:36 PM, Tom Lendacky wrote:
> On 12/3/24 03:00, Nikunj A Dadhania wrote:
>> Add support for Secure TSC in SNP-enabled guests. Secure TSC allows guests
>> to securely use RDTSC/RDTSCP instructions, ensuring that the parameters
>> used cannot be altered by the hypervisor once the guest is launched.
>>
>> Secure TSC-enabled guests need to query TSC information from the AMD
>> Security Processor. This communication channel is encrypted between the AMD
>> Security Processor and the guest, with the hypervisor acting merely as a
>> conduit to deliver the guest messages to the AMD Security Processor. Each
>> message is protected with AEAD (AES-256 GCM).
>>
>> Signed-off-by: Nikunj A Dadhania <nikunj@xxxxxxx>
>> Tested-by: Peter Gonda <pgonda@xxxxxxxxxx>
>> Reviewed-by: Tom Lendacky <thomas.lendacky@xxxxxxx>
>
> Just some minor nits if you have to respin...
Yes, I will be spinning a new version.
>
>> ---
>> arch/x86/include/asm/sev-common.h | 1 +
>> arch/x86/include/asm/sev.h | 22 ++++++
>> arch/x86/include/asm/svm.h | 6 +-
>> include/linux/cc_platform.h | 8 +++
>> arch/x86/coco/core.c | 3 +
>> arch/x86/coco/sev/core.c | 116 ++++++++++++++++++++++++++++++
>> arch/x86/mm/mem_encrypt.c | 2 +
>> 7 files changed, 156 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/sev-common.h b/arch/x86/include/asm/sev-common.h
>> index 50f5666938c0..6ef92432a5ce 100644
>> --- a/arch/x86/include/asm/sev-common.h
>> +++ b/arch/x86/include/asm/sev-common.h
>> @@ -206,6 +206,7 @@ struct snp_psc_desc {
>> #define GHCB_TERM_NO_SVSM 7 /* SVSM is not advertised in the secrets page */
>> #define GHCB_TERM_SVSM_VMPL0 8 /* SVSM is present but has set VMPL to 0 */
>> #define GHCB_TERM_SVSM_CAA 9 /* SVSM is present but CAA is not page aligned */
>> +#define GHCB_TERM_SECURE_TSC 10 /* 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 53f3048f484e..9fd02efef08e 100644
>> --- a/arch/x86/include/asm/sev.h
>> +++ b/arch/x86/include/asm/sev.h
>> @@ -146,6 +146,9 @@ enum msg_type {
>> SNP_MSG_VMRK_REQ,
>> SNP_MSG_VMRK_RSP,
>>
>> + SNP_MSG_TSC_INFO_REQ = 17,
>> + SNP_MSG_TSC_INFO_RSP,
>> +
>> SNP_MSG_TYPE_MAX
>> };
>>
>> @@ -174,6 +177,22 @@ struct snp_guest_msg {
>> u8 payload[PAGE_SIZE - sizeof(struct snp_guest_msg_hdr)];
>> } __packed;
>>
>> +#define SNP_TSC_INFO_REQ_SZ 128
>> +#define SNP_TSC_INFO_RESP_SZ 128
>> +
>> +struct snp_tsc_info_req {
>> + u8 rsvd[SNP_TSC_INFO_REQ_SZ];
>> +} __packed;
>> +
>> +struct snp_tsc_info_resp {
>> + u32 status;
>> + u32 rsvd1;
>> + u64 tsc_scale;
>> + u64 tsc_offset;
>> + u32 tsc_factor;
>> + u8 rsvd2[100];
>> +} __packed;
>> +
>> struct snp_guest_req {
>> void *req_buf;
>> size_t req_sz;
>> @@ -473,6 +492,8 @@ void snp_msg_free(struct snp_msg_desc *mdesc);
>> int snp_send_guest_request(struct snp_msg_desc *mdesc, struct snp_guest_req *req,
>> struct snp_guest_request_ioctl *rio);
>>
>> +void __init snp_secure_tsc_prepare(void);
>> +
>> #else /* !CONFIG_AMD_MEM_ENCRYPT */
>>
>> #define snp_vmpl 0
>> @@ -514,6 +535,7 @@ static inline struct snp_msg_desc *snp_msg_alloc(void) { return NULL; }
>> static inline void snp_msg_free(struct snp_msg_desc *mdesc) { }
>> static inline int snp_send_guest_request(struct snp_msg_desc *mdesc, struct snp_guest_req *req,
>> struct snp_guest_request_ioctl *rio) { return -ENODEV; }
>> +static inline void __init snp_secure_tsc_prepare(void) { }
>>
>> #endif /* CONFIG_AMD_MEM_ENCRYPT */
>>
>> diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h
>> index 2b59b9951c90..92e18798f197 100644
>> --- a/arch/x86/include/asm/svm.h
>> +++ b/arch/x86/include/asm/svm.h
>> @@ -417,7 +417,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;
>> @@ -564,7 +566,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/include/linux/cc_platform.h b/include/linux/cc_platform.h
>> index caa4b4430634..cb7103dc124f 100644
>> --- a/include/linux/cc_platform.h
>> +++ b/include/linux/cc_platform.h
>> @@ -88,6 +88,14 @@ enum cc_attr {
>> * enabled to run SEV-SNP guests.
>> */
>> CC_ATTR_HOST_SEV_SNP,
>> +
>> + /**
>> + * @CC_ATTR_GUEST_SNP_SECURE_TSC: SNP Secure TSC is active.
>> + *
>> + * The platform/OS is running as a guest/virtual machine and actively
>> + * using AMD SEV-SNP Secure TSC feature.
>> + */
>> + CC_ATTR_GUEST_SNP_SECURE_TSC,
>
> Maybe move this up above the host related attribute so that it is grouped
> with the other guest attributes.
Sure
>
>> };
>>
>> #ifdef CONFIG_ARCH_HAS_CC_PLATFORM
>> diff --git a/arch/x86/coco/core.c b/arch/x86/coco/core.c
>> index 0f81f70aca82..5b9a358a3254 100644
>> --- a/arch/x86/coco/core.c
>> +++ b/arch/x86/coco/core.c
>> @@ -100,6 +100,9 @@ static bool noinstr amd_cc_platform_has(enum cc_attr attr)
>> case CC_ATTR_HOST_SEV_SNP:
>> return cc_flags.host_sev_snp;
>>
>> + case CC_ATTR_GUEST_SNP_SECURE_TSC:
>> + return sev_status & MSR_AMD64_SNP_SECURE_TSC;
>> +
>
> Ditto here. Move this up above the host check.
Ack.
>
> Also, should this be:
>
> return (sev_status & MSR_AMD64_SEV_SNP_ENABLED) &&
> (sev_status & MSR_AMD64_SNP_SECURE_TSC);
>
> ?
Yes, we can do this and the below mentioned change.
>> default:
>> return false;
>> }
>> diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
>> index a61898c7f114..39683101b526 100644
>> --- a/arch/x86/coco/sev/core.c
>> +++ b/arch/x86/coco/sev/core.c
>> @@ -96,6 +96,14 @@ static u64 sev_hv_features __ro_after_init;
>> /* Secrets page physical address from the CC blob */
>> static u64 secrets_pa __ro_after_init;
>>
>> +/*
>> + * For Secure TSC guests, the BP fetches TSC_INFO using SNP guest messaging and
>> + * initializes snp_tsc_scale and snp_tsc_offset. These values are replicated
>> + * across the APs VMSA fields (TSC_SCALE and TSC_OFFSET).
>> + */
>> +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;
>> @@ -1277,6 +1285,12 @@ static int wakeup_cpu_via_vmgexit(u32 apic_id, unsigned long start_ip)
>> vmsa->vmpl = snp_vmpl;
>> vmsa->sev_features = sev_status >> 2;
>>
>> + /* Populate AP's TSC scale/offset to get accurate TSC values. */
>> + if (cc_platform_has(CC_ATTR_GUEST_SNP_SECURE_TSC)) {
>> + vmsa->tsc_scale = snp_tsc_scale;
>> + vmsa->tsc_offset = snp_tsc_offset;
>> + }
>> +
>> /* Switch the page over to a VMSA page now that it is initialized */
>> ret = snp_set_vmsa(vmsa, caa, apic_id, true);
>> if (ret) {
>> @@ -3127,3 +3141,105 @@ int snp_send_guest_request(struct snp_msg_desc *mdesc, struct snp_guest_req *req
>> }
>> EXPORT_SYMBOL_GPL(snp_send_guest_request);
>>
>> +static int __init snp_get_tsc_info(void)
>> +{
>> + struct snp_guest_request_ioctl *rio;
>> + struct snp_tsc_info_resp *tsc_resp;
>> + struct snp_tsc_info_req *tsc_req;
>> + struct snp_msg_desc *mdesc;
>> + struct snp_guest_req *req;
>> + unsigned char *buf;
>> + int rc = -ENOMEM;
>> +
>> + tsc_req = kzalloc(sizeof(*tsc_req), GFP_KERNEL);
>> + if (!tsc_req)
>> + return rc;
>> +
>> + tsc_resp = kzalloc(sizeof(*tsc_resp), GFP_KERNEL);
>> + if (!tsc_resp)
>> + goto e_free_tsc_req;
>> +
>> + req = kzalloc(sizeof(*req), GFP_KERNEL);
>> + if (!req)
>> + goto e_free_tsc_resp;
>> +
>> + rio = kzalloc(sizeof(*rio), GFP_KERNEL);
>> + if (!rio)
>> + goto e_free_req;
>> +
>> + /*
>> + * The intermediate response buffer is used while decrypting the
>> + * response payload. Make sure that it has enough space to cover
>> + * the authtag.
>> + */
>> + buf = kzalloc(SNP_TSC_INFO_RESP_SZ + AUTHTAG_LEN, GFP_KERNEL);
>> + if (!buf)
>> + goto e_free_rio;
>> +
>> + mdesc = snp_msg_alloc();
>> + if (IS_ERR_OR_NULL(mdesc))
>> + goto e_free_buf;
>> +
>> + rc = snp_msg_init(mdesc, snp_vmpl);
>> + if (rc)
>> + goto e_free_mdesc;
>> +
>> + req->msg_version = MSG_HDR_VER;
>> + req->msg_type = SNP_MSG_TSC_INFO_REQ;
>> + req->vmpck_id = snp_vmpl;
>> + req->req_buf = tsc_req;
>> + req->req_sz = sizeof(*tsc_req);
>> + req->resp_buf = buf;
>> + req->resp_sz = sizeof(*tsc_resp) + AUTHTAG_LEN;
>> + req->exit_code = SVM_VMGEXIT_GUEST_REQUEST;
>> +
>> + rc = snp_send_guest_request(mdesc, req, rio);
>> + if (rc)
>> + goto e_request;
>> +
>> + memcpy(tsc_resp, buf, sizeof(*tsc_resp));
>> + pr_debug("%s: response status 0x%x scale 0x%llx offset 0x%llx factor 0x%x\n",
>> + __func__, tsc_resp->status, tsc_resp->tsc_scale, tsc_resp->tsc_offset,
>> + tsc_resp->tsc_factor);
>> +
>> + if (tsc_resp->status == 0) {
>> + snp_tsc_scale = tsc_resp->tsc_scale;
>> + snp_tsc_offset = tsc_resp->tsc_offset;
>> + } else {
>> + pr_err("Failed to get TSC info, response status 0x%x\n", tsc_resp->status);
>> + rc = -EIO;
>> + }
>> +
>> +e_request:
>> + /* The response buffer contains sensitive data, explicitly clear it. */
>> + memzero_explicit(buf, sizeof(buf));
>> + memzero_explicit(tsc_resp, sizeof(*tsc_resp));
>> +e_free_mdesc:
>> + snp_msg_free(mdesc);
>> +e_free_buf:
>> + kfree(buf);
>> +e_free_rio:
>> + kfree(rio);
>> +e_free_req:
>> + kfree(req);
>> + e_free_tsc_resp:
>> + kfree(tsc_resp);
>> +e_free_tsc_req:
>> + kfree(tsc_req);
>> +
>> + return rc;
>> +}
>> +
>> +void __init snp_secure_tsc_prepare(void)
>> +{
>> + if (!cc_platform_has(CC_ATTR_GUEST_SEV_SNP) ||
>> + !cc_platform_has(CC_ATTR_GUEST_SNP_SECURE_TSC))
>
> If you make the change above, you only need to check for SNP_SECURE_TSC.
Ack
Regards
Nikunj
