| Message ID | f2b12d3c76b4e40a85da021ee2b7eaeda1dd69f0.1726602374.git.ashish.kalra@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add SEV-SNP CipherTextHiding feature support | expand |
On Tue, Sep 17, 2024 at 2:17 PM Ashish Kalra <Ashish.Kalra@amd.com> 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 > 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. > > Ciphertext hiding needs to be enabled on SNP_INIT_EX and therefore this > new module parameter has to added to the CCP driver. > > Signed-off-by: Ashish Kalra <ashish.kalra@amd.com> > --- > arch/x86/kvm/svm/sev.c | 26 ++++++++++++++---- > drivers/crypto/ccp/sev-dev.c | 52 ++++++++++++++++++++++++++++++++++++ > include/linux/psp-sev.h | 12 +++++++-- > 3 files changed, 83 insertions(+), 7 deletions(-) > > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c > index 0b851ef937f2..a345b4111ad6 100644 > --- a/arch/x86/kvm/svm/sev.c > +++ b/arch/x86/kvm/svm/sev.c > @@ -171,7 +171,7 @@ static void sev_misc_cg_uncharge(struct kvm_sev_info *sev) > misc_cg_uncharge(type, sev->misc_cg, 1); > } > > -static int sev_asid_new(struct kvm_sev_info *sev) > +static int sev_asid_new(struct kvm_sev_info *sev, unsigned long vm_type) > { > /* > * SEV-enabled guests must use asid from min_sev_asid to max_sev_asid. > @@ -199,6 +199,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 && sev->es_active) { > + if (vm_type == KVM_X86_SNP_VM) > + max_asid = snp_max_snp_asid; > + else > + min_asid = snp_max_snp_asid + 1; > + } > again: > asid = find_next_zero_bit(sev_asid_bitmap, max_asid + 1, min_asid); > if (asid > max_asid) { > @@ -440,7 +452,7 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp, > if (vm_type == KVM_X86_SNP_VM) > sev->vmsa_features |= SVM_SEV_FEAT_SNP_ACTIVE; > > - ret = sev_asid_new(sev); > + ret = sev_asid_new(sev, vm_type); > if (ret) > goto e_no_asid; > > @@ -3059,14 +3071,18 @@ 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 (snp_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 ? snp_max_snp_asid ? snp_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, snp_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 564daf748293..77900abb1b46 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 cipher_text_hiding = true; > +module_param(cipher_text_hiding, bool, 0444); > +MODULE_PARM_DESC(cipher_text_hiding, " if true, the PSP will enable Cipher Text Hiding"); > + > +static int max_snp_asid; > +module_param(max_snp_asid, int, 0444); > +MODULE_PARM_DESC(max_snp_asid, " override MAX_SNP_ASID for Cipher Text Hiding"); My read of the spec is if Ciphertext hiding is not enabled there is no additional split in the ASID space. Am I understanding that correctly? If so, I don't think we want to enable ciphertext hiding by default because it might break whatever management of ASIDs systems already have. For instance right now we have to split SEV-ES and SEV ASIDS, and SNP guests need SEV-ES ASIDS. This change would half the # of SNP enable ASIDs on a system. Also should we move the ASID splitting code to be all in one place? Right now KVM handles it in sev_hardware_setup(). > + > 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; > +EXPORT_SYMBOL(snp_cipher_text_hiding); > + > +/* MAX_SNP_ASID */ > +unsigned int snp_max_snp_asid; > +EXPORT_SYMBOL(snp_max_snp_asid); > + > static bool psp_dead; > static int psp_timeout; > > @@ -1064,6 +1080,38 @@ 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 & SNP_CIPHER_TEXT_HIDING_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 (max_snp_asid >= edx) { > + dev_info(sev->dev, "max_snp_asid module parameter is not valid, limiting to %d\n", > + edx - 1); > + max_snp_asid = edx - 1; > + } > + snp_max_snp_asid = max_snp_asid ? : (edx - 1) / 2; > + > + snp_cipher_text_hiding = 1; > + data->ciphertext_hiding_en = 1; > + data->max_snp_asid = snp_max_snp_asid; > + > + dev_dbg(sev->dev, "SEV-SNP CipherTextHiding feature support enabled\n"); > + } > +} > + > static void snp_get_platform_data(void) > { > struct sev_device *sev = psp_master->sev_data; > @@ -1199,6 +1247,10 @@ static int __sev_snp_init_locked(int *error) > } > > memset(&data, 0, sizeof(data)); > + > + if (cipher_text_hiding) > + 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 6068a89839e1..2102248bd436 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; > +extern unsigned int snp_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; > > /** > @@ -841,6 +847,8 @@ struct snp_feature_info { > u32 edx; > } __packed; > > +#define SNP_CIPHER_TEXT_HIDING_SUPPORTED BIT(3) > + > #ifdef CONFIG_CRYPTO_DEV_SP_PSP > > /** > -- > 2.34.1 > >
Hello Peter, On 10/2/2024 9:58 AM, Peter Gonda wrote: > On Tue, Sep 17, 2024 at 2:17 PM Ashish Kalra <Ashish.Kalra@amd.com> 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 >> 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. >> >> Ciphertext hiding needs to be enabled on SNP_INIT_EX and therefore this >> new module parameter has to added to the CCP driver. >> >> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com> >> --- >> arch/x86/kvm/svm/sev.c | 26 ++++++++++++++---- >> drivers/crypto/ccp/sev-dev.c | 52 ++++++++++++++++++++++++++++++++++++ >> include/linux/psp-sev.h | 12 +++++++-- >> 3 files changed, 83 insertions(+), 7 deletions(-) >> >> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c >> index 0b851ef937f2..a345b4111ad6 100644 >> --- a/arch/x86/kvm/svm/sev.c >> +++ b/arch/x86/kvm/svm/sev.c >> @@ -171,7 +171,7 @@ static void sev_misc_cg_uncharge(struct kvm_sev_info *sev) >> misc_cg_uncharge(type, sev->misc_cg, 1); >> } >> >> -static int sev_asid_new(struct kvm_sev_info *sev) >> +static int sev_asid_new(struct kvm_sev_info *sev, unsigned long vm_type) >> { >> /* >> * SEV-enabled guests must use asid from min_sev_asid to max_sev_asid. >> @@ -199,6 +199,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 && sev->es_active) { >> + if (vm_type == KVM_X86_SNP_VM) >> + max_asid = snp_max_snp_asid; >> + else >> + min_asid = snp_max_snp_asid + 1; >> + } >> again: >> asid = find_next_zero_bit(sev_asid_bitmap, max_asid + 1, min_asid); >> if (asid > max_asid) { >> @@ -440,7 +452,7 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp, >> if (vm_type == KVM_X86_SNP_VM) >> sev->vmsa_features |= SVM_SEV_FEAT_SNP_ACTIVE; >> >> - ret = sev_asid_new(sev); >> + ret = sev_asid_new(sev, vm_type); >> if (ret) >> goto e_no_asid; >> >> @@ -3059,14 +3071,18 @@ 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 (snp_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 ? snp_max_snp_asid ? snp_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, snp_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 564daf748293..77900abb1b46 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 cipher_text_hiding = true; >> +module_param(cipher_text_hiding, bool, 0444); >> +MODULE_PARM_DESC(cipher_text_hiding, " if true, the PSP will enable Cipher Text Hiding"); >> + >> +static int max_snp_asid; >> +module_param(max_snp_asid, int, 0444); >> +MODULE_PARM_DESC(max_snp_asid, " override MAX_SNP_ASID for Cipher Text Hiding"); > My read of the spec is if Ciphertext hiding is not enabled there is no > additional split in the ASID space. Am I understanding that correctly? Yes that is correct. > If so, I don't think we want to enable ciphertext hiding by default > because it might break whatever management of ASIDs systems already > have. For instance right now we have to split SEV-ES and SEV ASIDS, > and SNP guests need SEV-ES ASIDS. This change would half the # of SNP > enable ASIDs on a system. My thought here is that we probably want to enable Ciphertext hiding by default as that should fix any security issues and concerns around SNP encryption as .Ciphertext hiding prevents host accesses from reading the ciphertext of SNP guest private memory. This patch does add a new CCP module parameter, max_snp_asid, which can be used to dedicate all SEV-ES ASIDs to SNP guests. > > Also should we move the ASID splitting code to be all in one place? > Right now KVM handles it in sev_hardware_setup(). Yes, but there is going to be a separate set of patches to move all ASID handling code to CCP module. This refactoring won't be part of the SNP ciphertext hiding support patches. Thanks, Ashish >> + >> 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; >> +EXPORT_SYMBOL(snp_cipher_text_hiding); >> + >> +/* MAX_SNP_ASID */ >> +unsigned int snp_max_snp_asid; >> +EXPORT_SYMBOL(snp_max_snp_asid); >> + >> static bool psp_dead; >> static int psp_timeout; >> >> @@ -1064,6 +1080,38 @@ 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 & SNP_CIPHER_TEXT_HIDING_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 (max_snp_asid >= edx) { >> + dev_info(sev->dev, "max_snp_asid module parameter is not valid, limiting to %d\n", >> + edx - 1); >> + max_snp_asid = edx - 1; >> + } >> + snp_max_snp_asid = max_snp_asid ? : (edx - 1) / 2; >> + >> + snp_cipher_text_hiding = 1; >> + data->ciphertext_hiding_en = 1; >> + data->max_snp_asid = snp_max_snp_asid; >> + >> + dev_dbg(sev->dev, "SEV-SNP CipherTextHiding feature support enabled\n"); >> + } >> +} >> + >> static void snp_get_platform_data(void) >> { >> struct sev_device *sev = psp_master->sev_data; >> @@ -1199,6 +1247,10 @@ static int __sev_snp_init_locked(int *error) >> } >> >> memset(&data, 0, sizeof(data)); >> + >> + if (cipher_text_hiding) >> + 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 6068a89839e1..2102248bd436 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; >> +extern unsigned int snp_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; >> >> /** >> @@ -841,6 +847,8 @@ struct snp_feature_info { >> u32 edx; >> } __packed; >> >> +#define SNP_CIPHER_TEXT_HIDING_SUPPORTED BIT(3) >> + >> #ifdef CONFIG_CRYPTO_DEV_SP_PSP >> >> /** >> -- >> 2.34.1 >> >>
On 9/17/24 15:16, 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 > 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. > > Ciphertext hiding needs to be enabled on SNP_INIT_EX and therefore this > new module parameter has to added to the CCP driver. > > Signed-off-by: Ashish Kalra <ashish.kalra@amd.com> > --- > arch/x86/kvm/svm/sev.c | 26 ++++++++++++++---- > drivers/crypto/ccp/sev-dev.c | 52 ++++++++++++++++++++++++++++++++++++ > include/linux/psp-sev.h | 12 +++++++-- > 3 files changed, 83 insertions(+), 7 deletions(-) > > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c > index 0b851ef937f2..a345b4111ad6 100644 > --- a/arch/x86/kvm/svm/sev.c > +++ b/arch/x86/kvm/svm/sev.c > @@ -171,7 +171,7 @@ static void sev_misc_cg_uncharge(struct kvm_sev_info *sev) > misc_cg_uncharge(type, sev->misc_cg, 1); > } > > -static int sev_asid_new(struct kvm_sev_info *sev) > +static int sev_asid_new(struct kvm_sev_info *sev, unsigned long vm_type) > { > /* > * SEV-enabled guests must use asid from min_sev_asid to max_sev_asid. > @@ -199,6 +199,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 && sev->es_active) { > + if (vm_type == KVM_X86_SNP_VM) > + max_asid = snp_max_snp_asid; > + else > + min_asid = snp_max_snp_asid + 1; > + } Add a blank line here. > again: > asid = find_next_zero_bit(sev_asid_bitmap, max_asid + 1, min_asid); > if (asid > max_asid) { > @@ -440,7 +452,7 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp, > if (vm_type == KVM_X86_SNP_VM) > sev->vmsa_features |= SVM_SEV_FEAT_SNP_ACTIVE; > > - ret = sev_asid_new(sev); > + ret = sev_asid_new(sev, vm_type); > if (ret) > goto e_no_asid; > > @@ -3059,14 +3071,18 @@ 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 (snp_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 ? snp_max_snp_asid ? snp_max_snp_asid + 1 : 1 : > + 0, min_sev_asid - 1); I think this might be easier to read if you align everything based on the conditions, e.g.: pr_info("SEV-ES %s (ASIDs %u - %u)\n", sev_es_supported ? "enabled" : "disabled", min_sev_asid > 1 ? snp_max_snp_asid ? snp_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, snp_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 564daf748293..77900abb1b46 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 cipher_text_hiding = true; > +module_param(cipher_text_hiding, bool, 0444); > +MODULE_PARM_DESC(cipher_text_hiding, " if true, the PSP will enable Cipher Text Hiding"); I agree with Peter that this should be false by default to maintain backward compatibility. > + > +static int max_snp_asid; > +module_param(max_snp_asid, int, 0444); > +MODULE_PARM_DESC(max_snp_asid, " override MAX_SNP_ASID for Cipher Text Hiding"); MODULE_PARM_DESC(max_snp_asid, " the maximum SNP ASID available when Cipher Text Hiding is enabled"); > + > 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; > +EXPORT_SYMBOL(snp_cipher_text_hiding); > + > +/* MAX_SNP_ASID */ > +unsigned int snp_max_snp_asid; > +EXPORT_SYMBOL(snp_max_snp_asid); > + > static bool psp_dead; > static int psp_timeout; > > @@ -1064,6 +1080,38 @@ 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 & SNP_CIPHER_TEXT_HIDING_SUPPORTED) && > + sev->snp_plat_status.ciphertext_hiding_cap) { Remove the indentation here by just doing: if (!(sev->feat_info.ecx & SNP_CIPHER_TEXT_HIDING_SUPPORTED)) return; if (!sev->snp_plat_status.ciphertext_hiding_cap) return; > + /* Retrieve SEV CPUID information */ Remove this comment and ... > + edx = cpuid_edx(0x8000001f); > + /* Do sanity checks on user-defined MAX_SNP_ASID */ ... move this comment above the cpuid_edx() call. > + if (max_snp_asid >= edx) { > + dev_info(sev->dev, "max_snp_asid module parameter is not valid, limiting to %d\n", > + edx - 1); > + max_snp_asid = edx - 1; > + } > + snp_max_snp_asid = max_snp_asid ? : (edx - 1) / 2; > + > + snp_cipher_text_hiding = 1; s/1/true/ > + data->ciphertext_hiding_en = 1; > + data->max_snp_asid = snp_max_snp_asid; > + > + dev_dbg(sev->dev, "SEV-SNP CipherTextHiding feature support enabled\n"); "SEV-SNP cipher text hiding enabled" Thanks, Tom > + } > +} > + > static void snp_get_platform_data(void) > { > struct sev_device *sev = psp_master->sev_data; > @@ -1199,6 +1247,10 @@ static int __sev_snp_init_locked(int *error) > } > > memset(&data, 0, sizeof(data)); > + > + if (cipher_text_hiding) > + 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 6068a89839e1..2102248bd436 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; > +extern unsigned int snp_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; > > /** > @@ -841,6 +847,8 @@ struct snp_feature_info { > u32 edx; > } __packed; > > +#define SNP_CIPHER_TEXT_HIDING_SUPPORTED BIT(3) > + > #ifdef CONFIG_CRYPTO_DEV_SP_PSP > > /**
On 9/17/24 15:16, 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 > 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. > > Ciphertext hiding needs to be enabled on SNP_INIT_EX and therefore this > new module parameter has to added to the CCP driver. > > Signed-off-by: Ashish Kalra <ashish.kalra@amd.com> > --- > arch/x86/kvm/svm/sev.c | 26 ++++++++++++++---- > drivers/crypto/ccp/sev-dev.c | 52 ++++++++++++++++++++++++++++++++++++ > include/linux/psp-sev.h | 12 +++++++-- > 3 files changed, 83 insertions(+), 7 deletions(-) I missed this on initial review. This change goes across multiple maintainers trees, so you should split this patch to do the CCP updates first and then the KVM updates. Thanks, Tom >
> >> +static int max_snp_asid; > >> +module_param(max_snp_asid, int, 0444); > >> +MODULE_PARM_DESC(max_snp_asid, " override MAX_SNP_ASID for Cipher Text Hiding"); > > My read of the spec is if Ciphertext hiding is not enabled there is no > > additional split in the ASID space. Am I understanding that correctly? > Yes that is correct. > > If so, I don't think we want to enable ciphertext hiding by default > > because it might break whatever management of ASIDs systems already > > have. For instance right now we have to split SEV-ES and SEV ASIDS, > > and SNP guests need SEV-ES ASIDS. This change would half the # of SNP > > enable ASIDs on a system. > > My thought here is that we probably want to enable Ciphertext hiding by default as that should fix any security issues and concerns around SNP encryption as .Ciphertext hiding prevents host accesses from reading the ciphertext of SNP guest private memory. > > This patch does add a new CCP module parameter, max_snp_asid, which can be used to dedicate all SEV-ES ASIDs to SNP guests. > > > > > Also should we move the ASID splitting code to be all in one place? > > Right now KVM handles it in sev_hardware_setup(). > > Yes, but there is going to be a separate set of patches to move all ASID handling code to CCP module. > > This refactoring won't be part of the SNP ciphertext hiding support patches. Makes sense. I see Tom has asked you to split this patch into ccp and KVM. Maybe add a line to the description so more are aware of the impending changes to asids? I tested these patches a bit with the selftests / manually by backporting to 6.11-rc7. When you send a V3 I'll redo for a tag. BTW for some reason 6.12-rc1 and kvm/queue both fail to init SNP for me, then the kernel segfaults. Not sure whats going on there...
>>>>> +static int max_snp_asid; >>>> +module_param(max_snp_asid, int, 0444); >>>> +MODULE_PARM_DESC(max_snp_asid, " override MAX_SNP_ASID for Cipher Text Hiding"); >>> My read of the spec is if Ciphertext hiding is not enabled there is no >>> additional split in the ASID space. Am I understanding that correctly? >> Yes that is correct. >>> If so, I don't think we want to enable ciphertext hiding by default >>> because it might break whatever management of ASIDs systems already >>> have. For instance right now we have to split SEV-ES and SEV ASIDS, >>> and SNP guests need SEV-ES ASIDS. This change would half the # of SNP >>> enable ASIDs on a system. >> >> My thought here is that we probably want to enable Ciphertext hiding by default as that should fix any security issues and concerns around SNP encryption as .Ciphertext hiding prevents host accesses from reading the ciphertext of SNP guest private memory. >> >> This patch does add a new CCP module parameter, max_snp_asid, which can be used to dedicate all SEV-ES ASIDs to SNP guests. >> >>> >>> Also should we move the ASID splitting code to be all in one place? >>> Right now KVM handles it in sev_hardware_setup(). >> >> Yes, but there is going to be a separate set of patches to move all ASID handling code to CCP module. >> >> This refactoring won't be part of the SNP ciphertext hiding support patches. >Makes sense. I see Tom has asked you to split this patch into ccp and KVM. >Maybe add a line to the description so more are aware of the impending >changes to asids? Sure, i will do that. >I tested these patches a bit with the selftests / manually by >backporting to 6.11-rc7. When you send a V3 I'll redo for a tag. BTW >for some reason 6.12-rc1 and kvm/queue both fail to init SNP for me, >then the kernel segfaults. Not sure whats going on there... I tested with 6.12-rc1 and i don't have any issues with SNP init and running SNP VMs on that (with and without CipherTextHiding enabled), but i am getting a lot of stack dumps especially during host boot with apparmor, surely something looks to be broken on apparmor on 6.12-rc1: [ 33.180836] BUG: kernel NULL pointer dereference, address: 000000000000001c [ 33.180842] #PF: supervisor read access in kernel mode [ 33.180843] #PF: error_code(0x0000) - not-present page [ 33.180846] PGD 16bc1b067 P4D 0 [ 33.180849] Oops: Oops: 0000 [#4] SMP NOPTI [ 33.180853] CPU: 155 UID: 0 PID: 2521 Comm: apparmor_parser Tainted: G D W 6.12.0-rc1-next-20241003-snp-host-f2a41ff576cc-dirty #19 [ 33.632606] RIP: 0010:krealloc_noprof+0x8f/0x300 [ 33.632608] Code: 8b 50 08 f6 c2 01 0f 85 14 02 00 00 0f 1f 44 00 00 80 78 33 f5 0f 85 0e 02 00 00 48 85 c0 0f 84 05 02 00 00 48 8b 48 08 66 90 <48> 63 59 1c 41 89 df 4d 39 fd 0f 87 8c 00 00 00 0f 1f 44 00 00 48 [ 33.632610] RSP: 0018:ff2e31fe0ad3f848 EFLAGS: 00010202 [ 33.632611] RAX: ff9e19414443ec00 RBX: 0000000000000001 RCX: 0000000000000000 [ 33.632613] RDX: 0000000000000000 RSI: 0000000000002beb RDI: ff2d8c4410fb0000 [ 33.632614] RBP: ff2e31fe0ad3f878 R08: 0000000000002be4 R09: 0000000000000000 [ 33.632615] R10: 00000000000093cb R11: ff2d8c4410fb2beb R12: ff2d8c4410fb0000 [ 33.632616] R13: 0000000000002beb R14: 0000000000000cc0 R15: ff2d8c446d000000 [ 33.632618] FS: 00007ff504a05740(0000) GS:ff2d8c4b2c500000(0000) knlGS:0000000000000000 [ 33.632619] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 33.632620] CR2: 000000000000001c CR3: 0000000157f2e001 CR4: 0000000000771ef0 [ 33.632622] PKRU: 55555554 [ 33.632623] note: apparmor_parser[2522] exited with irqs disabled [ 33.977961] Tainted: [D]=DIE, [W]=WARN [ 33.990019] Hardware name: AMD Corporation PURICO/PURICO, BIOS TPUT0090F 06/05/2024 [ 34.006754] RIP: 0010:krealloc_noprof+0x8f/0x300 [ 34.020151] Code: 8b 50 08 f6 c2 01 0f 85 14 02 00 00 0f 1f 44 00 00 80 78 33 f5 0f 85 0e 02 00 00 48 85 c0 0f 84 05 02 00 00 48 8b 48 08 66 90 <48> 63 59 1c 41 89 df 4d 39 fd 0f 87 8c 00 00 00 0f 1f 44 00 00 48 [ 34.058484] RSP: 0018:ff2e31fe0ad57928 EFLAGS: 00010202 [ 34.073095] RAX: ff9e194145b4c400 RBX: 0000000000000001 RCX: 0000000000000000 [ 34.089957] RDX: 0000000000000000 RSI: 00000000000057bf RDI: ff2d8c446d310000 [ 34.106827] RBP: ff2e31fe0ad57958 R08: 00000000000057b8 R09: 0000000000000000 [ 34.123733] R10: 000000000000dac1 R11: ff2d8c446d3157bf R12: ff2d8c446d310000 [ 34.140668] R13: 00000000000057bf R14: 0000000000000cc0 R15: ff2d8c446d400000 [ 34.157572] FS: 00007ff504a05740(0000) GS:ff2d8c4b2b380000(0000) knlGS:0000000000000000 [ 34.175513] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 34.190675] CR2: 000000000000001c CR3: 0000000157f2a004 CR4: 0000000000771ef0 [ 34.207373] PKRU: 55555554 [ 34.218980] Call Trace: [ 34.230226] <TASK> [ 34.241043] ? show_regs+0x6d/0x80 [ 34.253389] ? __die+0x29/0x70 [ 34.265311] ? page_fault_oops+0x15c/0x550 [ 34.278341] ? do_user_addr_fault+0x45e/0x7b0 [ 34.291477] ? ZSTD_compressEnd_public+0x2c/0x170 [ 34.304780] ? exc_page_fault+0x7c/0x170 [ 34.316962] ? asm_exc_page_fault+0x2b/0x30 [ 34.329194] ? krealloc_noprof+0x8f/0x300 [ 34.341001] ? zstd_compress_cctx+0x87/0xa0 [ 34.353005] aa_unpack+0x688/0x700 [ 34.364035] aa_replace_profiles+0x9e/0x1170 [ 34.375977] policy_update+0xd9/0x170 [ 34.387225] profile_replace+0xb0/0x130 [ 34.398644] vfs_write+0xf8/0x3e0 [ 34.409463] ? __x64_sys_openat+0x59/0xa0 [ 34.420909] ksys_write+0x6b/0xf0 [ 34.431356] __x64_sys_write+0x1d/0x30 [ 34.442244] x64_sys_call+0x1685/0x20d0 [ 34.453055] do_syscall_64+0x6f/0x110 [ 34.463491] ? ksys_read+0x6b/0xf0 [ 34.473492] ? syscall_exit_to_user_mode+0x57/0x1b0 [ 34.485139] ? do_syscall_64+0x7b/0x110 [ 34.495611] ? generic_file_read_iter+0xbf/0x110 [ 34.506980] ? apparmor_file_permission+0x6f/0x170 [ 34.518530] ? ext4_file_read_iter+0x5f/0x1e0 [ 34.529610] ? vfs_read+0x25c/0x340 [ 34.539607] ? ksys_read+0x6b/0xf0 [ 34.549394] ? syscall_exit_to_user_mode+0x57/0x1b0 [ 34.560829] ? do_syscall_64+0x7b/0x110 [ 34.571009] ? irqentry_exit_to_user_mode+0x33/0x170 [ 34.582461] ? irqentry_exit+0x21/0x40 [ 34.592443] ? exc_page_fault+0x8d/0x170 [ 34.602507] entry_SYSCALL_64_after_hwframe+0x76/0x7e [ 34.613786] RIP: 0033:0x7ff504714887 [ 34.623229] Code: 10 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 00 f3 0f 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 10 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 51 c3 48 83 ec 28 48 89 54 24 18 48 89 74 24 [ 34.655533] RSP: 002b:00007ffcb6fbc758 EFLAGS: 00000246 ORIG_RAX: 0000000000000001 [ 34.669681] RAX: ffffffffffffffda RBX: 000055f36c77bdc0 RCX: 00007ff504714887 [ 34.683405] RDX: 000000000000dac1 RSI: 000055f36c7a1680 RDI: 0000000000000007 [ 34.697133] RBP: 000000000000dac1 R08: 0000000000000000 R09: 000055f36c7a1680 [ 34.710815] R10: 0000000000000000 R11: 0000000000000246 R12: 000055f36c7a1680 [ 34.724467] R13: 000000000000dac1 R14: 000055f3654bcc5b R15: 0000000000000007 [ 34.738032] </TASK> [ 34.745917] Modules linked in: nls_iso8859_1 wmi_bmof rapl input_leds joydev ccp(+) k10temp wmi acpi_ipmi ipmi_si ipmi_devintf ipmi_msghandler mac_hid sch_fq_codel dm_multipath scsi_dh_rdac scsi_dh_emc scsi_dh_alua msr efi_pstore drm autofs4 btrfs blake2b_generic raid10 raid456 async_raid6_recov async_memcpy async_pq async_xor async_tx xor raid6_pq raid1 raid0 crct10dif_pclmul ahci crc32_pclmul tg3 ghash_clmulni_intel libahci i2c_piix4 i2c_smbus hid_generic usbhid hid aesni_intel crypto_simd cryptd [ 34.819993] CR2: 000000000000001c [ 34.830269] ---[ end trace 0000000000000000 ]---
On Wed, Oct 02, 2024, Ashish Kalra wrote: > Hello Peter, > > On 10/2/2024 9:58 AM, Peter Gonda wrote: > > On Tue, Sep 17, 2024 at 2:17 PM Ashish Kalra <Ashish.Kalra@amd.com> wrote: > >> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c > >> index 564daf748293..77900abb1b46 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 cipher_text_hiding = true; > >> +module_param(cipher_text_hiding, bool, 0444); > >> +MODULE_PARM_DESC(cipher_text_hiding, " if true, the PSP will enable Cipher Text Hiding"); > >> + > >> +static int max_snp_asid; > >> +module_param(max_snp_asid, int, 0444); > >> +MODULE_PARM_DESC(max_snp_asid, " override MAX_SNP_ASID for Cipher Text Hiding"); > > My read of the spec is if Ciphertext hiding is not enabled there is no > > additional split in the ASID space. Am I understanding that correctly? > Yes that is correct. > > If so, I don't think we want to enable ciphertext hiding by default > > because it might break whatever management of ASIDs systems already > > have. For instance right now we have to split SEV-ES and SEV ASIDS, > > and SNP guests need SEV-ES ASIDS. This change would half the # of SNP > > enable ASIDs on a system. > > My thought here is that we probably want to enable Ciphertext hiding by > default as that should fix any security issues and concerns around SNP > encryption as .Ciphertext hiding prevents host accesses from reading the > ciphertext of SNP guest private memory. > > This patch does add a new CCP module parameter, max_snp_asid, which can be > used to dedicate all SEV-ES ASIDs to SNP guests. > > > > > Also should we move the ASID splitting code to be all in one place? > > Right now KVM handles it in sev_hardware_setup(). > > Yes, but there is going to be a separate set of patches to move all ASID > handling code to CCP module. > > This refactoring won't be part of the SNP ciphertext hiding support patches. It should, because that's not a "refactoring", that's a change of roles and responsibilities. And this series does the same; even worse, this series leaves things in a half-baked state, where the CCP and KVM have a weird shared ownership of ASID management. I'm ok with essentially treating CipherText Hiding enablement as an extension of firmware, e.g. it's better than having to go into UEFI settings to toggle the feature on/off. But we need to have a clear, well-defined vision for how we want this all to look in the end.
On Tue, Sep 17, 2024, Ashish Kalra wrote: > diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c > index 564daf748293..77900abb1b46 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 cipher_text_hiding = true; > +module_param(cipher_text_hiding, bool, 0444); > +MODULE_PARM_DESC(cipher_text_hiding, " if true, the PSP will enable Cipher Text Hiding"); > + > +static int max_snp_asid; Why is this a signed int? '0' is used as the magic "no override" value, so there's no reason to allow a negative value. > +module_param(max_snp_asid, int, 0444); > +MODULE_PARM_DESC(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; > +EXPORT_SYMBOL(snp_cipher_text_hiding); > + > +/* MAX_SNP_ASID */ > +unsigned int snp_max_snp_asid; > +EXPORT_SYMBOL(snp_max_snp_asid); There is zero reason to have multiple variables. The module param varaibles should be the single source of true. I'm also not entirely sure exporting individual variables is the right interface, which is another reason why I want to see the entire "refactoring" in one series. > static bool psp_dead; > static int psp_timeout; > > @@ -1064,6 +1080,38 @@ 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 & SNP_CIPHER_TEXT_HIDING_SUPPORTED) && > + sev->snp_plat_status.ciphertext_hiding_cap) { snp_cipher_text_hiding should be set to %false if CipherTextHiding is unsupported. I.e. the module params need to reflect reality. > + /* Retrieve SEV CPUID information */ > + edx = cpuid_edx(0x8000001f); > + /* Do sanity checks on user-defined MAX_SNP_ASID */ > + if (max_snp_asid >= edx) { > + dev_info(sev->dev, "max_snp_asid module parameter is not valid, limiting to %d\n", > + edx - 1); > + max_snp_asid = edx - 1; > + } > + snp_max_snp_asid = max_snp_asid ? : (edx - 1) / 2; > + > + snp_cipher_text_hiding = 1; s/1/true > + data->ciphertext_hiding_en = 1; > + data->max_snp_asid = snp_max_snp_asid; > + > + dev_dbg(sev->dev, "SEV-SNP CipherTextHiding feature support enabled\n"); > + } > +}
On 11/20/2024 3:53 PM, Sean Christopherson wrote: > On Tue, Nov 19, 2024, Ashish Kalra wrote: >> On 10/11/2024 11:04 AM, Sean Christopherson wrote: >>> On Wed, Oct 02, 2024, Ashish Kalra wrote: >>>> Yes, but there is going to be a separate set of patches to move all ASID >>>> handling code to CCP module. >>>> >>>> This refactoring won't be part of the SNP ciphertext hiding support patches. >>> >>> It should, because that's not a "refactoring", that's a change of roles and >>> responsibilities. And this series does the same; even worse, this series leaves >>> things in a half-baked state, where the CCP and KVM have a weird shared ownership >>> of ASID management. >> >> Sorry for the delayed reply to your response, the SNP DOWNLOAD_FIRMWARE_EX >> patches got posted in the meanwhile and that had additional considerations of >> moving SNP GCTX pages stuff into the PSP driver from KVM and that again got >> into this discussion about splitting ASID management across KVM and PSP >> driver and as you pointed out on those patches that there is zero reason that >> the PSP driver needs to care about ASIDs. >> >> Well, CipherText Hiding (CTH) support is one reason where the PSP driver gets >> involved with ASIDs as CTH feature has to be enabled as part of SNP_INIT_EX >> and once CTH feature is enabled, the SEV-ES ASID space is split across >> SEV-SNP and SEV-ES VMs. > > Right, but that's just a case where KVM needs to react to the setup done by the > PSP, correct? E.g. it's similar to SEV-ES being enabled/disabled in firmware, > only that "firmware" happens to be a kernel driver. Yes that is true. > >> With reference to SNP GCTX pages, we are looking at some possibilities to >> push the requirement to update SNP GCTX pages to SNP firmware and remove that >> requirement from the kernel/KVM side. > > Heh, that'd work too. > >> Considering that, I will still like to keep ASID management in KVM, there are >> issues with locking, for example, sev_deactivate_lock is used to protect SNP >> ASID allocations (or actually for protecting ASID reuse/lazy-allocation >> requiring WBINVD/DF_FLUSH) and guarding this DF_FLUSH from VM destruction >> (DEACTIVATE). Moving ASID management stuff into PSP driver will then add >> complexity of adding this synchronization between different kernel modules or >> handling locking in two different kernel modules, to guard ASID allocation in >> PSP driver with VM destruction in KVM module. >> >> There is also this sev_vmcbs[] array indexed by ASID (part of svm_cpu_data) >> which gets referenced during the ASID free code path in KVM. It just makes it >> simpler to keep ASID management stuff in KVM. >> >> So probably we can add an API interface exported by the PSP driver something >> like is_sev_ciphertext_hiding_enabled() or sev_override_max_snp_asid() > > What about adding a cc_attr_flags entry? Yes, that is a possibility i will look into. But, along with an additional cc_attr_flags entry, max_snp_asid (which is a PSP driver module parameter) also needs to be propagated to KVM, that's what i was considering passing as parameter to the above API interface. Thanks, Ashish
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c index 0b851ef937f2..a345b4111ad6 100644 --- a/arch/x86/kvm/svm/sev.c +++ b/arch/x86/kvm/svm/sev.c @@ -171,7 +171,7 @@ static void sev_misc_cg_uncharge(struct kvm_sev_info *sev) misc_cg_uncharge(type, sev->misc_cg, 1); } -static int sev_asid_new(struct kvm_sev_info *sev) +static int sev_asid_new(struct kvm_sev_info *sev, unsigned long vm_type) { /* * SEV-enabled guests must use asid from min_sev_asid to max_sev_asid. @@ -199,6 +199,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 && sev->es_active) { + if (vm_type == KVM_X86_SNP_VM) + max_asid = snp_max_snp_asid; + else + min_asid = snp_max_snp_asid + 1; + } again: asid = find_next_zero_bit(sev_asid_bitmap, max_asid + 1, min_asid); if (asid > max_asid) { @@ -440,7 +452,7 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp, if (vm_type == KVM_X86_SNP_VM) sev->vmsa_features |= SVM_SEV_FEAT_SNP_ACTIVE; - ret = sev_asid_new(sev); + ret = sev_asid_new(sev, vm_type); if (ret) goto e_no_asid; @@ -3059,14 +3071,18 @@ 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 (snp_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 ? snp_max_snp_asid ? snp_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, snp_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 564daf748293..77900abb1b46 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 cipher_text_hiding = true; +module_param(cipher_text_hiding, bool, 0444); +MODULE_PARM_DESC(cipher_text_hiding, " if true, the PSP will enable Cipher Text Hiding"); + +static int max_snp_asid; +module_param(max_snp_asid, int, 0444); +MODULE_PARM_DESC(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; +EXPORT_SYMBOL(snp_cipher_text_hiding); + +/* MAX_SNP_ASID */ +unsigned int snp_max_snp_asid; +EXPORT_SYMBOL(snp_max_snp_asid); + static bool psp_dead; static int psp_timeout; @@ -1064,6 +1080,38 @@ 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 & SNP_CIPHER_TEXT_HIDING_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 (max_snp_asid >= edx) { + dev_info(sev->dev, "max_snp_asid module parameter is not valid, limiting to %d\n", + edx - 1); + max_snp_asid = edx - 1; + } + snp_max_snp_asid = max_snp_asid ? : (edx - 1) / 2; + + snp_cipher_text_hiding = 1; + data->ciphertext_hiding_en = 1; + data->max_snp_asid = snp_max_snp_asid; + + dev_dbg(sev->dev, "SEV-SNP CipherTextHiding feature support enabled\n"); + } +} + static void snp_get_platform_data(void) { struct sev_device *sev = psp_master->sev_data; @@ -1199,6 +1247,10 @@ static int __sev_snp_init_locked(int *error) } memset(&data, 0, sizeof(data)); + + if (cipher_text_hiding) + 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 6068a89839e1..2102248bd436 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; +extern unsigned int snp_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; /** @@ -841,6 +847,8 @@ struct snp_feature_info { u32 edx; } __packed; +#define SNP_CIPHER_TEXT_HIDING_SUPPORTED BIT(3) + #ifdef CONFIG_CRYPTO_DEV_SP_PSP /**