From: "Nikunj A. Dadhania" <nikunj@amd.com>
To: Borislav Petkov <bp@alien8.de>
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
Date: Tue, 23 Apr 2024 10:14:08 +0530 [thread overview]
Message-ID: <e394360a-85c0-4d6f-a0e1-dff466f572f9@amd.com> (raw)
In-Reply-To: <20240422135019.GDZiZrG0sKpq0fXQ8d@fat_crate.local>
On 4/22/2024 7:20 PM, Borislav Petkov wrote:
> 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"
Sure
>> 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.
I will add empty line.
>> + 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.
Sure.
>
>> + 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?
Sure.
>> + 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.
Sure
>
>> +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.
Ok
>
>> + 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.
>
Sure
>> +
>> + /* 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.
Sure
>
> Ok, I'm going to stop here and give you a chance to work in all the
> review feedback and send a new revision.
Thank you for detailed review/feedback, will address them and send new revision.
Regards
Nikunj
next prev parent reply other threads:[~2024-04-23 4:44 UTC|newest]
Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-15 11:31 [PATCH v8 00/16] Add Secure TSC support for SNP guests Nikunj A Dadhania
2024-02-15 11:31 ` [PATCH v8 01/16] virt: sev-guest: Use AES GCM crypto library Nikunj A Dadhania
2024-02-27 18:25 ` Borislav Petkov
2024-02-15 11:31 ` [PATCH v8 02/16] virt: sev-guest: Replace dev_dbg with pr_debug Nikunj A Dadhania
2024-02-27 18:28 ` Borislav Petkov
2024-02-15 11:31 ` [PATCH v8 03/16] virt: sev-guest: Add SNP guest request structure Nikunj A Dadhania
2024-02-27 22:20 ` Tom Lendacky
2024-02-29 9:12 ` Nikunj A. Dadhania
2024-02-28 11:50 ` Borislav Petkov
2024-02-29 9:26 ` Nikunj A. Dadhania
2024-02-15 11:31 ` [PATCH v8 04/16] virt: sev-guest: Add vmpck_id to snp_guest_dev struct Nikunj A Dadhania
2024-04-09 10:23 ` Borislav Petkov
2024-04-16 5:57 ` Nikunj A. Dadhania
2024-04-16 9:06 ` Borislav Petkov
2024-04-17 4:18 ` Nikunj A. Dadhania
2024-02-15 11:31 ` [PATCH v8 05/16] x86/sev: Cache the secrets page address Nikunj A Dadhania
2024-04-16 14:45 ` Borislav Petkov
2024-04-17 5:27 ` Nikunj A. Dadhania
2024-04-17 7:59 ` Nikunj A. Dadhania
2024-02-15 11:31 ` [PATCH v8 06/16] virt: sev-guest: Move SNP Guest command mutex Nikunj A Dadhania
2024-04-22 13:00 ` Borislav Petkov
2024-04-23 4:22 ` Nikunj A. Dadhania
2024-04-23 10:28 ` Borislav Petkov
2024-04-23 10:42 ` Nikunj A. Dadhania
2024-04-23 11:21 ` Borislav Petkov
2024-02-15 11:31 ` [PATCH v8 07/16] x86/sev: Move and reorganize sev guest request api Nikunj A Dadhania
2024-04-22 13:14 ` Borislav Petkov
2024-04-23 4:26 ` Nikunj A. Dadhania
2024-04-23 13:22 ` Borislav Petkov
2024-02-15 11:31 ` [PATCH v8 08/16] x86/mm: Add generic guest initialization hook Nikunj A Dadhania
2024-04-22 13:20 ` Borislav Petkov
2024-04-23 4:34 ` Nikunj A. Dadhania
2024-02-15 11:31 ` [PATCH v8 09/16] x86/cpufeatures: Add synthetic Secure TSC bit Nikunj A Dadhania
2024-04-22 13:25 ` Borislav Petkov
2024-04-23 4:40 ` Nikunj A. Dadhania
2024-04-23 13:29 ` Borislav Petkov
2024-02-15 11:31 ` [PATCH v8 10/16] x86/sev: Add Secure TSC support for SNP guests Nikunj A Dadhania
2024-04-22 13:50 ` Borislav Petkov
2024-04-23 4:44 ` Nikunj A. Dadhania [this message]
2024-02-15 11:31 ` [PATCH v8 11/16] x86/sev: Change TSC MSR behavior for Secure TSC enabled guests Nikunj A Dadhania
2024-02-15 11:31 ` [PATCH v8 12/16] x86/sev: Prevent RDTSC/RDTSCP interception " Nikunj A Dadhania
2024-02-15 11:31 ` [PATCH v8 13/16] x86/kvmclock: Skip kvmclock when Secure TSC is available Nikunj A Dadhania
2024-02-15 11:31 ` [PATCH v8 14/16] x86/sev: Mark Secure TSC as reliable Nikunj A Dadhania
2024-02-15 11:31 ` [PATCH v8 15/16] x86/cpu/amd: Do not print FW_BUG for Secure TSC Nikunj A Dadhania
2024-02-15 11:31 ` [PATCH v8 16/16] x86/sev: Enable Secure TSC for SNP guests Nikunj A Dadhania
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e394360a-85c0-4d6f-a0e1-dff466f572f9@amd.com \
--to=nikunj@amd.com \
--cc=bp@alien8.de \
--cc=dave.hansen@linux.intel.com \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@redhat.com \
--cc=pbonzini@redhat.com \
--cc=pgonda@google.com \
--cc=seanjc@google.com \
--cc=tglx@linutronix.de \
--cc=thomas.lendacky@amd.com \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).