| Message ID | b05c6de0c3cd47f804fd77be60ca2d90a6d28f8d.1723490152.git.ashish.kalra@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add SEV-SNP CipherTextHiding feature support | expand |
On 8/12/24 14:42, Ashish Kalra wrote: > From: Ashish Kalra <ashish.kalra@amd.com> > > Ciphertext hiding prevents host accesses from reading the ciphertext of > SNP guest private memory. Instead of reading ciphertext, the host reads > will see constant default values (0xff). > > Ciphertext hiding separates the ASID space into SNP guest ASIDs and host > ASIDs. All SNP active guests must have an ASID less than or equal to > MAX_SNP_ASID provided to the SNP_INIT_EX command. All SEV-legacy guests > (SEV and SEV-ES) must be greater than MAX_SNP_ASID. > > This patch-set adds a new module parameter to the CCP driver defined as > psp_max_snp_asid which is a user configurable MAX_SNP_ASID to define the > system-wide maximum SNP ASID value. If this value is not set, then the > ASID space is equally divided between SEV-SNP and SEV-ES guests. Add something here to talk about how cipher text hiding needs to be enabled on SNP_INIT_EX and so the module parameters are part of the CCP driver. Not sure if having KVM invoke the CCP, at KVM module load time, to perform the SNP_INIT_EX would make sense or not. That would allow all the cipher text hiding support parameters to be in KVM. Lets see what others think. > > Signed-off-by: Ashish Kalra <ashish.kalra@amd.com> > --- > arch/x86/kvm/svm/sev.c | 24 ++++++++++++++--- > drivers/crypto/ccp/sev-dev.c | 50 ++++++++++++++++++++++++++++++++++++ > include/linux/psp-sev.h | 10 ++++++-- > 3 files changed, 79 insertions(+), 5 deletions(-) > > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c > index 532df12b43c5..954ef99a1aa8 100644 > --- a/arch/x86/kvm/svm/sev.c > +++ b/arch/x86/kvm/svm/sev.c > @@ -173,6 +173,9 @@ static void sev_misc_cg_uncharge(struct kvm_sev_info *sev) > > static int sev_asid_new(struct kvm_sev_info *sev) > { > + struct kvm_svm *svm = container_of(sev, struct kvm_svm, sev_info); > + struct kvm *kvm = &svm->kvm; Why not just add the vm-type to the input parameters? > + > /* > * SEV-enabled guests must use asid from min_sev_asid to max_sev_asid. > * SEV-ES-enabled guest can use from 1 to min_sev_asid - 1. > @@ -199,6 +202,18 @@ static int sev_asid_new(struct kvm_sev_info *sev) > > mutex_lock(&sev_bitmap_lock); > > + /* > + * When CipherTextHiding is enabled, all SNP guests must have an > + * ASID less than or equal to MAX_SNP_ASID provided on the > + * SNP_INIT_EX command and all the SEV-ES guests must have > + * an ASID greater than MAX_SNP_ASID. > + */ > + if (snp_cipher_text_hiding_en && sev->es_active) { > + if (kvm->arch.vm_type == KVM_X86_SNP_VM) > + max_asid = max_snp_asid; > + else > + min_asid = max_snp_asid + 1; > + } > again: > asid = find_next_zero_bit(sev_asid_bitmap, max_asid + 1, min_asid); > if (asid > max_asid) { > @@ -3058,14 +3073,17 @@ void __init sev_hardware_setup(void) > "unusable" : > "disabled", > min_sev_asid, max_sev_asid); > - if (boot_cpu_has(X86_FEATURE_SEV_ES)) > + if (boot_cpu_has(X86_FEATURE_SEV_ES)) { > + if (max_snp_asid >= (min_sev_asid - 1)) > + sev_es_supported = false; > pr_info("SEV-ES %s (ASIDs %u - %u)\n", > sev_es_supported ? "enabled" : "disabled", > - min_sev_asid > 1 ? 1 : 0, min_sev_asid - 1); > + min_sev_asid > 1 ? max_snp_asid ? max_snp_asid + 1 : 1 : 0, min_sev_asid - 1); Maybe put the last parameter on a separate line to make this a little easier to read? > + } > if (boot_cpu_has(X86_FEATURE_SEV_SNP)) > pr_info("SEV-SNP %s (ASIDs %u - %u)\n", > sev_snp_supported ? "enabled" : "disabled", > - min_sev_asid > 1 ? 1 : 0, min_sev_asid - 1); > + min_sev_asid > 1 ? 1 : 0, max_snp_asid ? : min_sev_asid - 1); > > sev_enabled = sev_supported; > sev_es_enabled = sev_es_supported; > diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c > index eefb481db5af..9ee81a6defc5 100644 > --- a/drivers/crypto/ccp/sev-dev.c > +++ b/drivers/crypto/ccp/sev-dev.c > @@ -73,11 +73,27 @@ static bool psp_init_on_probe = true; > module_param(psp_init_on_probe, bool, 0444); > MODULE_PARM_DESC(psp_init_on_probe, " if true, the PSP will be initialized on module init. Else the PSP will be initialized on the first command requiring it"); > > +static bool psp_cth_enabled = true; This is a static, so how about just calling this cipher_text_hiding. > +module_param(psp_cth_enabled, bool, 0444); > +MODULE_PARM_DESC(psp_cth_enabled, " if true, the PSP will enable Cipher Text Hiding"); > + > +static int psp_max_snp_asid; Also a static, so just call this max_snp_asid. > +module_param(psp_max_snp_asid, int, 0444); > +MODULE_PARM_DESC(psp_max_snp_asid, " override MAX_SNP_ASID for Cipher Text Hiding"); > + > MODULE_FIRMWARE("amd/amd_sev_fam17h_model0xh.sbin"); /* 1st gen EPYC */ > MODULE_FIRMWARE("amd/amd_sev_fam17h_model3xh.sbin"); /* 2nd gen EPYC */ > MODULE_FIRMWARE("amd/amd_sev_fam19h_model0xh.sbin"); /* 3rd gen EPYC */ > MODULE_FIRMWARE("amd/amd_sev_fam19h_model1xh.sbin"); /* 4th gen EPYC */ > > +/* Cipher Text Hiding Enabled */ > +bool snp_cipher_text_hiding_en; > +EXPORT_SYMBOL(snp_cipher_text_hiding_en); I think you drop the "_en" and just have snp_cipher_text_hiding. > + > +/* MAX_SNP_ASID */ > +unsigned int max_snp_asid; > +EXPORT_SYMBOL(max_snp_asid); This should have "snp_" to provide better namespace isolation. > + > static bool psp_dead; > static int psp_timeout; > > @@ -1053,6 +1069,36 @@ static void snp_set_hsave_pa(void *arg) > wrmsrl(MSR_VM_HSAVE_PA, 0); > } > > +static void sev_snp_enable_ciphertext_hiding(struct sev_data_snp_init_ex *data, int *error) > +{ > + struct psp_device *psp = psp_master; > + struct sev_device *sev; > + unsigned int edx; > + > + sev = psp->sev_data; > + > + /* > + * Check if CipherTextHiding feature is supported and enabled > + * in the Platform/BIOS. > + */ > + if (sev->feat_info.ecx & FEAT_CIPHERTEXTHIDING_SUPPORTED && > + sev->snp_plat_status.ciphertext_hiding_cap) { I'm not sure you need both checks. Either the platform status or the feature info check should be enough. Can you check on that? > + /* Retrieve SEV CPUID information */ > + edx = cpuid_edx(0x8000001f); > + /* Do sanity checks on user-defined MAX_SNP_ASID */ > + if (psp_max_snp_asid > (edx - 1)) { How about: if (psp_max_snp_asid >= edx) { > + dev_info(sev->dev, "user-defined MAX_SNP_ASID invalid, limiting to %d\n", max_snp_asid module parameter is not valid, limiting to %d\n > + edx - 1); > + psp_max_snp_asid = edx - 1; > + } > + max_snp_asid = psp_max_snp_asid ? : (edx - 1) / 2; Add a blank line here > + snp_cipher_text_hiding_en = 1; > + data->ciphertext_hiding_en = 1; > + data->max_snp_asid = max_snp_asid; Ditto Thanks, Tom > + dev_dbg(sev->dev, "SEV-SNP CipherTextHiding feature support enabled\n"); > + } > +} > + > static void sev_cache_snp_platform_status_and_discover_features(void) > { > struct sev_device *sev = psp_master->sev_data; > @@ -1181,6 +1227,10 @@ static int __sev_snp_init_locked(int *error) > } > > memset(&data, 0, sizeof(data)); > + > + if (psp_cth_enabled) > + sev_snp_enable_ciphertext_hiding(&data, error); > + > data.init_rmp = 1; > data.list_paddr_en = 1; > data.list_paddr = __psp_pa(snp_range_list); > diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h > index d46d73911a76..a26b43c2eab9 100644 > --- a/include/linux/psp-sev.h > +++ b/include/linux/psp-sev.h > @@ -27,6 +27,9 @@ enum sev_state { > SEV_STATE_MAX > }; > > +extern bool snp_cipher_text_hiding_en; > +extern unsigned int max_snp_asid; > + > /** > * SEV platform and guest management commands > */ > @@ -746,10 +749,13 @@ struct sev_data_snp_guest_request { > struct sev_data_snp_init_ex { > u32 init_rmp:1; > u32 list_paddr_en:1; > - u32 rsvd:30; > + u32 rapl_dis:1; > + u32 ciphertext_hiding_en:1; > + u32 rsvd:28; > u32 rsvd1; > u64 list_paddr; > - u8 rsvd2[48]; > + u16 max_snp_asid; > + u8 rsvd2[46]; > } __packed; > > /**
Hi Tom, On 8/15/2024 10:58 AM, Tom Lendacky wrote: >> + >> static bool psp_dead; >> static int psp_timeout; >> >> @@ -1053,6 +1069,36 @@ static void snp_set_hsave_pa(void *arg) >> wrmsrl(MSR_VM_HSAVE_PA, 0); >> } >> >> +static void sev_snp_enable_ciphertext_hiding(struct sev_data_snp_init_ex *data, int *error) >> +{ >> + struct psp_device *psp = psp_master; >> + struct sev_device *sev; >> + unsigned int edx; >> + >> + sev = psp->sev_data; >> + >> + /* >> + * Check if CipherTextHiding feature is supported and enabled >> + * in the Platform/BIOS. >> + */ >> + if (sev->feat_info.ecx & FEAT_CIPHERTEXTHIDING_SUPPORTED && >> + sev->snp_plat_status.ciphertext_hiding_cap) { > I'm not sure you need both checks. Either the platform status or the > feature info check should be enough. Can you check on that? > FEATURE_INFO only indicates if SEV FW is capable of ciphertext hiding and SNP_PLATFORM_STATUS ciphertext_hiding_cap bit indicates if the CTH feature is enabled in BIOS, so both checks are needed. Thanks, Ashish
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c index 532df12b43c5..954ef99a1aa8 100644 --- a/arch/x86/kvm/svm/sev.c +++ b/arch/x86/kvm/svm/sev.c @@ -173,6 +173,9 @@ static void sev_misc_cg_uncharge(struct kvm_sev_info *sev) static int sev_asid_new(struct kvm_sev_info *sev) { + struct kvm_svm *svm = container_of(sev, struct kvm_svm, sev_info); + struct kvm *kvm = &svm->kvm; + /* * SEV-enabled guests must use asid from min_sev_asid to max_sev_asid. * SEV-ES-enabled guest can use from 1 to min_sev_asid - 1. @@ -199,6 +202,18 @@ static int sev_asid_new(struct kvm_sev_info *sev) mutex_lock(&sev_bitmap_lock); + /* + * When CipherTextHiding is enabled, all SNP guests must have an + * ASID less than or equal to MAX_SNP_ASID provided on the + * SNP_INIT_EX command and all the SEV-ES guests must have + * an ASID greater than MAX_SNP_ASID. + */ + if (snp_cipher_text_hiding_en && sev->es_active) { + if (kvm->arch.vm_type == KVM_X86_SNP_VM) + max_asid = max_snp_asid; + else + min_asid = max_snp_asid + 1; + } again: asid = find_next_zero_bit(sev_asid_bitmap, max_asid + 1, min_asid); if (asid > max_asid) { @@ -3058,14 +3073,17 @@ void __init sev_hardware_setup(void) "unusable" : "disabled", min_sev_asid, max_sev_asid); - if (boot_cpu_has(X86_FEATURE_SEV_ES)) + if (boot_cpu_has(X86_FEATURE_SEV_ES)) { + if (max_snp_asid >= (min_sev_asid - 1)) + sev_es_supported = false; pr_info("SEV-ES %s (ASIDs %u - %u)\n", sev_es_supported ? "enabled" : "disabled", - min_sev_asid > 1 ? 1 : 0, min_sev_asid - 1); + min_sev_asid > 1 ? max_snp_asid ? max_snp_asid + 1 : 1 : 0, min_sev_asid - 1); + } if (boot_cpu_has(X86_FEATURE_SEV_SNP)) pr_info("SEV-SNP %s (ASIDs %u - %u)\n", sev_snp_supported ? "enabled" : "disabled", - min_sev_asid > 1 ? 1 : 0, min_sev_asid - 1); + min_sev_asid > 1 ? 1 : 0, max_snp_asid ? : min_sev_asid - 1); sev_enabled = sev_supported; sev_es_enabled = sev_es_supported; diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c index eefb481db5af..9ee81a6defc5 100644 --- a/drivers/crypto/ccp/sev-dev.c +++ b/drivers/crypto/ccp/sev-dev.c @@ -73,11 +73,27 @@ static bool psp_init_on_probe = true; module_param(psp_init_on_probe, bool, 0444); MODULE_PARM_DESC(psp_init_on_probe, " if true, the PSP will be initialized on module init. Else the PSP will be initialized on the first command requiring it"); +static bool psp_cth_enabled = true; +module_param(psp_cth_enabled, bool, 0444); +MODULE_PARM_DESC(psp_cth_enabled, " if true, the PSP will enable Cipher Text Hiding"); + +static int psp_max_snp_asid; +module_param(psp_max_snp_asid, int, 0444); +MODULE_PARM_DESC(psp_max_snp_asid, " override MAX_SNP_ASID for Cipher Text Hiding"); + MODULE_FIRMWARE("amd/amd_sev_fam17h_model0xh.sbin"); /* 1st gen EPYC */ MODULE_FIRMWARE("amd/amd_sev_fam17h_model3xh.sbin"); /* 2nd gen EPYC */ MODULE_FIRMWARE("amd/amd_sev_fam19h_model0xh.sbin"); /* 3rd gen EPYC */ MODULE_FIRMWARE("amd/amd_sev_fam19h_model1xh.sbin"); /* 4th gen EPYC */ +/* Cipher Text Hiding Enabled */ +bool snp_cipher_text_hiding_en; +EXPORT_SYMBOL(snp_cipher_text_hiding_en); + +/* MAX_SNP_ASID */ +unsigned int max_snp_asid; +EXPORT_SYMBOL(max_snp_asid); + static bool psp_dead; static int psp_timeout; @@ -1053,6 +1069,36 @@ static void snp_set_hsave_pa(void *arg) wrmsrl(MSR_VM_HSAVE_PA, 0); } +static void sev_snp_enable_ciphertext_hiding(struct sev_data_snp_init_ex *data, int *error) +{ + struct psp_device *psp = psp_master; + struct sev_device *sev; + unsigned int edx; + + sev = psp->sev_data; + + /* + * Check if CipherTextHiding feature is supported and enabled + * in the Platform/BIOS. + */ + if (sev->feat_info.ecx & FEAT_CIPHERTEXTHIDING_SUPPORTED && + sev->snp_plat_status.ciphertext_hiding_cap) { + /* Retrieve SEV CPUID information */ + edx = cpuid_edx(0x8000001f); + /* Do sanity checks on user-defined MAX_SNP_ASID */ + if (psp_max_snp_asid > (edx - 1)) { + dev_info(sev->dev, "user-defined MAX_SNP_ASID invalid, limiting to %d\n", + edx - 1); + psp_max_snp_asid = edx - 1; + } + max_snp_asid = psp_max_snp_asid ? : (edx - 1) / 2; + snp_cipher_text_hiding_en = 1; + data->ciphertext_hiding_en = 1; + data->max_snp_asid = max_snp_asid; + dev_dbg(sev->dev, "SEV-SNP CipherTextHiding feature support enabled\n"); + } +} + static void sev_cache_snp_platform_status_and_discover_features(void) { struct sev_device *sev = psp_master->sev_data; @@ -1181,6 +1227,10 @@ static int __sev_snp_init_locked(int *error) } memset(&data, 0, sizeof(data)); + + if (psp_cth_enabled) + sev_snp_enable_ciphertext_hiding(&data, error); + data.init_rmp = 1; data.list_paddr_en = 1; data.list_paddr = __psp_pa(snp_range_list); diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h index d46d73911a76..a26b43c2eab9 100644 --- a/include/linux/psp-sev.h +++ b/include/linux/psp-sev.h @@ -27,6 +27,9 @@ enum sev_state { SEV_STATE_MAX }; +extern bool snp_cipher_text_hiding_en; +extern unsigned int max_snp_asid; + /** * SEV platform and guest management commands */ @@ -746,10 +749,13 @@ struct sev_data_snp_guest_request { struct sev_data_snp_init_ex { u32 init_rmp:1; u32 list_paddr_en:1; - u32 rsvd:30; + u32 rapl_dis:1; + u32 ciphertext_hiding_en:1; + u32 rsvd:28; u32 rsvd1; u64 list_paddr; - u8 rsvd2[48]; + u16 max_snp_asid; + u8 rsvd2[46]; } __packed; /**