Received-SPF: pass (google.com: domain of linux-kernel+bounces-153496-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Mon, 22 Apr 2024 15:50:19 +0200
From: Borislav Petkov <bp@alien8.de>
To: Nikunj A Dadhania <nikunj@amd.com>
Cc: linux-kernel@vger.kernel.org, thomas.lendacky@amd.com, x86@kernel.org,
	kvm@vger.kernel.org, mingo@redhat.com, tglx@linutronix.de,
	dave.hansen@linux.intel.com, pgonda@google.com, seanjc@google.com,
	pbonzini@redhat.com
Subject: Re: [PATCH v8 10/16] x86/sev: Add Secure TSC support for SNP guests
Message-ID: <20240422135019.GDZiZrG0sKpq0fXQ8d@fat_crate.local>
References: <20240215113128.275608-1-nikunj@amd.com>
 <20240215113128.275608-11-nikunj@amd.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20240215113128.275608-11-nikunj@amd.com>

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@amd.com>
> Tested-by: Peter Gonda <pgonda@google.com>
> ---
>  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