[v2,3/3] x86/sev: Add SEV-SNP CipherTextHiding support

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(-)

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");
> +	}
> +}

Hello Sean,

On 10/11/2024 11:04 AM, Sean Christopherson wrote:
> 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.
> 

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. 

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.

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() instead of using
external variables in PSP driver, which KVM can call in sev_hardware_setup() to
retrieve MAX_SNP_ASID and also overriding max_asid (when CTH feature is enabled) in sev_asid_new(). 

Thanks,
Ashish

> 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, 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.

> 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?

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

On 11/20/2024 5:43 PM, Kalra, Ashish wrote:
> 
> 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.
> 

Adding a new cc_attr_flags entry indicating CTH support is enabled.

And as discussed with Boris, using the cc_platform_set() to add a new attr like max_asid and adding a getter interface on top to return the
max_snp_asid.

Thanks,
Ashish

On Thu, Nov 21, 2024, Ashish Kalra wrote:
> On 11/20/2024 5:43 PM, Kalra, Ashish wrote:
> > 
> > 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.

Doh, right, I managed to forget about those module params.

> Adding a new cc_attr_flags entry indicating CTH support is enabled.
> 
> And as discussed with Boris, using the cc_platform_set() to add a new attr
> like max_asid and adding a getter interface on top to return the
> max_snp_asid.

Actually, IMO, the behavior of _sev_platform_init_locked() and pretty much all of
the APIs that invoke it are flawed, and make all of this way more confusing and
convoluted than it needs to be.

IIUC, SNP initialization is forced during probe purely because SNP can't be
initialized if VMs are running.  But the only in-tree user of SEV-XXX functionality
is KVM, and KVM depends on whatever this driver is called.  So forcing SNP
initialization because a hypervisor could be running legacy VMs make no sense.
Just require KVM to initialize SEV functionality if KVM wants to use SEV+.

	/*
	 * Legacy guests cannot be running while SNP_INIT(_EX) is executing,
	 * so perform SEV-SNP initialization at probe time.
	 */
	rc = __sev_snp_init_locked(&args->error); 

Rather than automatically init SEV+ functionality, can we instead do something
like the (half-baked pseudo-patch) below?  I.e. delete all paths that implicitly
init the PSP, and force KVM to explicitly initialize the PSP if KVM wants to use
SEV+.  Then we can put the CipherText and SNP ASID params in KVM.

That would also allow (a) registering the SNP panic notifier if and only if SNP
is actually initailized and (b) shutting down SEV+ in the PSP when KVM is unloaded.
Arguably, the PSP should be shutdown when KVM is unloaded, irrespective of the
CipherText and SNP ASID knobs.  But with those knobs, it becomes even more desirable,
because it would allow userspace to reload *KVM* in order to change the CipherText
and SNP ASID module params.  I.e. doesn't require unloading the entire CCP driver.

If dropping the implicit initialization in some of the ioctls would break existing
userspace, then maybe we could add a module param (or Kconfig?) to preserve that
behavior?  I'm not familiar with what actually uses /dev/sev.

Side topic #1, sev_pci_init() is buggy.  It should destroy SEV if getting the
API version fails after a firmware update.

Side topic #2, the version check is broken, as it returns "success" when
initialization quite obviously failed.

	if (!sev_version_greater_or_equal(SNP_MIN_API_MAJOR, SNP_MIN_API_MINOR)) {
		dev_dbg(sev->dev, "SEV-SNP support requires firmware version >= %d:%d\n",
			SNP_MIN_API_MAJOR, SNP_MIN_API_MINOR);
		return 0;
	}

---
 drivers/crypto/ccp/sev-dev.c | 102 +++++++++--------------------------
 include/linux/psp-sev.h      |  19 ++-----
 2 files changed, 28 insertions(+), 93 deletions(-)

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index af018afd9cd7..563cc235b095 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -69,10 +69,6 @@ static char *init_ex_path;
 module_param(init_ex_path, charp, 0444);
 MODULE_PARM_DESC(init_ex_path, " Path for INIT_EX data; if set try INIT_EX");
 
-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");
-
 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 */
@@ -1306,11 +1302,13 @@ static int __sev_platform_init_locked(int *error)
 	return 0;
 }
 
-static int _sev_platform_init_locked(struct sev_platform_init_args *args)
+int sev_platform_init(int *error)
 {
 	struct sev_device *sev;
 	int rc;
 
+	guard(mutex)(&sev_cmd_mutex)
+
 	if (!psp_master || !psp_master->sev_data)
 		return -ENODEV;
 
@@ -1319,36 +1317,17 @@ static int _sev_platform_init_locked(struct sev_platform_init_args *args)
 	if (sev->state == SEV_STATE_INIT)
 		return 0;
 
-	/*
-	 * Legacy guests cannot be running while SNP_INIT(_EX) is executing,
-	 * so perform SEV-SNP initialization at probe time.
-	 */
-	rc = __sev_snp_init_locked(&args->error);
+	rc = __sev_snp_init_locked(error);
 	if (rc && rc != -ENODEV) {
 		/*
 		 * Don't abort the probe if SNP INIT failed,
 		 * continue to initialize the legacy SEV firmware.
 		 */
 		dev_err(sev->dev, "SEV-SNP: failed to INIT rc %d, error %#x\n",
-			rc, args->error);
+			rc, *error);
 	}
 
-	/* Defer legacy SEV/SEV-ES support if allowed by caller/module. */
-	if (args->probe && !psp_init_on_probe)
-		return 0;
-
-	return __sev_platform_init_locked(&args->error);
-}
-
-int sev_platform_init(struct sev_platform_init_args *args)
-{
-	int rc;
-
-	mutex_lock(&sev_cmd_mutex);
-	rc = _sev_platform_init_locked(args);
-	mutex_unlock(&sev_cmd_mutex);
-
-	return rc;
+	return __sev_platform_init_locked(error);
 }
 EXPORT_SYMBOL_GPL(sev_platform_init);
 
@@ -1441,16 +1420,12 @@ static int sev_ioctl_do_platform_status(struct sev_issue_cmd *argp)
 static int sev_ioctl_do_pek_pdh_gen(int cmd, struct sev_issue_cmd *argp, bool writable)
 {
 	struct sev_device *sev = psp_master->sev_data;
-	int rc;
 
 	if (!writable)
 		return -EPERM;
 
-	if (sev->state == SEV_STATE_UNINIT) {
-		rc = __sev_platform_init_locked(&argp->error);
-		if (rc)
-			return rc;
-	}
+	if (sev->state == SEV_STATE_UNINIT)
+		return -ENOTTY;
 
 	return __sev_do_cmd_locked(cmd, NULL, &argp->error);
 }
@@ -1467,6 +1442,9 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
 	if (!writable)
 		return -EPERM;
 
+	if (sev->state == SEV_STATE_UNINIT)
+		return -ENOTTY;
+
 	if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
 		return -EFAULT;
 
@@ -1489,12 +1467,6 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
 	data.len = input.length;
 
 cmd:
-	if (sev->state == SEV_STATE_UNINIT) {
-		ret = __sev_platform_init_locked(&argp->error);
-		if (ret)
-			goto e_free_blob;
-	}
-
 	ret = __sev_do_cmd_locked(SEV_CMD_PEK_CSR, &data, &argp->error);
 
 	 /* If we query the CSR length, FW responded with expected data. */
@@ -1584,7 +1556,6 @@ static int sev_get_firmware(struct device *dev,
 	return -ENOENT;
 }
 
-/* Don't fail if SEV FW couldn't be updated. Continue with existing SEV FW */
 static int sev_update_firmware(struct device *dev)
 {
 	struct sev_data_download_firmware *data;
@@ -1732,6 +1703,9 @@ static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp, bool writable)
 	if (!writable)
 		return -EPERM;
 
+	if (sev->state == SEV_STATE_UNINIT)
+		return -ENOTTY;
+
 	if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
 		return -EFAULT;
 
@@ -1754,16 +1728,8 @@ static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp, bool writable)
 	data.oca_cert_address = __psp_pa(oca_blob);
 	data.oca_cert_len = input.oca_cert_len;
 
-	/* If platform is not in INIT state then transition it to INIT */
-	if (sev->state != SEV_STATE_INIT) {
-		ret = __sev_platform_init_locked(&argp->error);
-		if (ret)
-			goto e_free_oca;
-	}
-
 	ret = __sev_do_cmd_locked(SEV_CMD_PEK_CERT_IMPORT, &data, &argp->error);
 
-e_free_oca:
 	kfree(oca_blob);
 e_free_pek:
 	kfree(pek_blob);
@@ -1882,15 +1848,8 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
 	void __user *input_pdh_cert_address;
 	int ret;
 
-	/* If platform is not in INIT state then transition it to INIT. */
-	if (sev->state != SEV_STATE_INIT) {
-		if (!writable)
-			return -EPERM;
-
-		ret = __sev_platform_init_locked(&argp->error);
-		if (ret)
-			return ret;
-	}
+	if (sev->state != SEV_STATE_INIT)
+		return -ENOTTY;
 
 	if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
 		return -EFAULT;
@@ -2296,6 +2255,9 @@ static void __sev_firmware_shutdown(struct sev_device *sev, bool panic)
 {
 	int error;
 
+	atomic_notifier_chain_unregister(&panic_notifier_list,
+					 &snp_panic_notifier);
+
 	__sev_platform_shutdown_locked(NULL);
 
 	if (sev_es_tmr) {
@@ -2390,9 +2352,7 @@ EXPORT_SYMBOL_GPL(sev_issue_cmd_external_user);
 void sev_pci_init(void)
 {
 	struct sev_device *sev = psp_master->sev_data;
-	struct sev_platform_init_args args = {0};
 	u8 api_major, api_minor, build;
-	int rc;
 
 	if (!sev)
 		return;
@@ -2406,27 +2366,18 @@ void sev_pci_init(void)
 	api_minor = sev->api_minor;
 	build     = sev->build;
 
-	if (sev_update_firmware(sev->dev) == 0)
-		sev_get_api_version();
+	/* Don't fail if SEV FW couldn't be updated. Continue with existing SEV FW. */
+	if (sev_update_firmware(sev->dev))
+		return;
+
+	if (sev_get_api_version())
+		goto err;
 
 	if (api_major != sev->api_major || api_minor != sev->api_minor ||
 	    build != sev->build)
 		dev_info(sev->dev, "SEV firmware updated from %d.%d.%d to %d.%d.%d\n",
 			 api_major, api_minor, build,
 			 sev->api_major, sev->api_minor, sev->build);
-
-	/* Initialize the platform */
-	args.probe = true;
-	rc = sev_platform_init(&args);
-	if (rc)
-		dev_err(sev->dev, "SEV: failed to INIT error %#x, rc %d\n",
-			args.error, rc);
-
-	dev_info(sev->dev, "SEV%s API:%d.%d build:%d\n", sev->snp_initialized ?
-		"-SNP" : "", sev->api_major, sev->api_minor, sev->build);
-
-	atomic_notifier_chain_register(&panic_notifier_list,
-				       &snp_panic_notifier);
 	return;
 
 err:
@@ -2443,7 +2394,4 @@ void sev_pci_exit(void)
 		return;
 
 	sev_firmware_shutdown(sev);
-
-	atomic_notifier_chain_unregister(&panic_notifier_list,
-					 &snp_panic_notifier);
 }
diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
index 903ddfea8585..def40f7ea01d 100644
--- a/include/linux/psp-sev.h
+++ b/include/linux/psp-sev.h
@@ -790,19 +790,6 @@ struct sev_data_snp_shutdown_ex {
 	u32 rsvd1:31;
 } __packed;
 
-/**
- * struct sev_platform_init_args
- *
- * @error: SEV firmware error code
- * @probe: True if this is being called as part of CCP module probe, which
- *  will defer SEV_INIT/SEV_INIT_EX firmware initialization until needed
- *  unless psp_init_on_probe module param is set
- */
-struct sev_platform_init_args {
-	int error;
-	bool probe;
-};
-
 /**
  * struct sev_data_snp_commit - SNP_COMMIT structure
  *
@@ -817,7 +804,7 @@ struct sev_data_snp_commit {
 /**
  * sev_platform_init - perform SEV INIT command
  *
- * @args: struct sev_platform_init_args to pass in arguments
+ * @error: SEV firmware error code
  *
  * Returns:
  * 0 if the SEV successfully processed the command
@@ -826,7 +813,7 @@ struct sev_data_snp_commit {
  * -%ETIMEDOUT if the SEV command timed out
  * -%EIO       if the SEV returned a non-zero return code
  */
-int sev_platform_init(struct sev_platform_init_args *args);
+int sev_platform_init(int *error);
 
 /**
  * sev_platform_status - perform SEV PLATFORM_STATUS command
@@ -951,7 +938,7 @@ void snp_free_firmware_page(void *addr);
 static inline int
 sev_platform_status(struct sev_user_data_status *status, int *error) { return -ENODEV; }
 
-static inline int sev_platform_init(struct sev_platform_init_args *args) { return -ENODEV; }
+static inline int sev_platform_init(int *error) { return -ENODEV; }
 
 static inline int
 sev_guest_deactivate(struct sev_data_deactivate *data, int *error) { return -ENODEV; }

base-commit: 43fb83c17ba2d63dfb798f0be7453ed55ca3f9c2
--

On 11/21/24 10:56, Sean Christopherson wrote:
> On Thu, Nov 21, 2024, Ashish Kalra wrote:
>> On 11/20/2024 5:43 PM, Kalra, Ashish wrote:
>>>
>>> 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.
> 
> Doh, right, I managed to forget about those module params.
> 
>> Adding a new cc_attr_flags entry indicating CTH support is enabled.
>>
>> And as discussed with Boris, using the cc_platform_set() to add a new attr
>> like max_asid and adding a getter interface on top to return the
>> max_snp_asid.
> 
> Actually, IMO, the behavior of _sev_platform_init_locked() and pretty much all of
> the APIs that invoke it are flawed, and make all of this way more confusing and
> convoluted than it needs to be.
> 
> IIUC, SNP initialization is forced during probe purely because SNP can't be
> initialized if VMs are running.  But the only in-tree user of SEV-XXX functionality
> is KVM, and KVM depends on whatever this driver is called.  So forcing SNP
> initialization because a hypervisor could be running legacy VMs make no sense.
> Just require KVM to initialize SEV functionality if KVM wants to use SEV+.

When we say legacy VMs, that also means non-SEV VMs. So you can't have any
VM running within a VMRUN instruction.

Or...

> 
> 	/*
> 	 * Legacy guests cannot be running while SNP_INIT(_EX) is executing,
> 	 * so perform SEV-SNP initialization at probe time.
> 	 */
> 	rc = __sev_snp_init_locked(&args->error); 
> 
> Rather than automatically init SEV+ functionality, can we instead do something
> like the (half-baked pseudo-patch) below?  I.e. delete all paths that implicitly
> init the PSP, and force KVM to explicitly initialize the PSP if KVM wants to use
> SEV+.  Then we can put the CipherText and SNP ASID params in KVM.

... do you mean at module load time (based on the module parameters)? Or
when the first SEV VM is run? I would think the latter, as the parameters
are all true by default. If the latter, that would present a problem of
having to ensure no VMs are active while performing the SNP_INIT.

> 
> That would also allow (a) registering the SNP panic notifier if and only if SNP
> is actually initailized and (b) shutting down SEV+ in the PSP when KVM is unloaded.
> Arguably, the PSP should be shutdown when KVM is unloaded, irrespective of the
> CipherText and SNP ASID knobs.  But with those knobs, it becomes even more desirable,
> because it would allow userspace to reload *KVM* in order to change the CipherText
> and SNP ASID module params.  I.e. doesn't require unloading the entire CCP driver.
> 
> If dropping the implicit initialization in some of the ioctls would break existing
> userspace, then maybe we could add a module param (or Kconfig?) to preserve that
> behavior?  I'm not familiar with what actually uses /dev/sev.
> 
> Side topic #1, sev_pci_init() is buggy.  It should destroy SEV if getting the
> API version fails after a firmware update.

True, we'll look at doing a fix for that.

> 
> Side topic #2, the version check is broken, as it returns "success" when
> initialization quite obviously failed.

That is ok because you can still initialize SEV / SEV-ES support.

Thanks,
Tom

> 
> 	if (!sev_version_greater_or_equal(SNP_MIN_API_MAJOR, SNP_MIN_API_MINOR)) {
> 		dev_dbg(sev->dev, "SEV-SNP support requires firmware version >= %d:%d\n",
> 			SNP_MIN_API_MAJOR, SNP_MIN_API_MINOR);
> 		return 0;
> 	}
> 
> ---
>  drivers/crypto/ccp/sev-dev.c | 102 +++++++++--------------------------
>  include/linux/psp-sev.h      |  19 ++-----
>  2 files changed, 28 insertions(+), 93 deletions(-)
> 
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index af018afd9cd7..563cc235b095 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -69,10 +69,6 @@ static char *init_ex_path;
>  module_param(init_ex_path, charp, 0444);
>  MODULE_PARM_DESC(init_ex_path, " Path for INIT_EX data; if set try INIT_EX");
>  
> -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");
> -
>  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 */
> @@ -1306,11 +1302,13 @@ static int __sev_platform_init_locked(int *error)
>  	return 0;
>  }
>  
> -static int _sev_platform_init_locked(struct sev_platform_init_args *args)
> +int sev_platform_init(int *error)
>  {
>  	struct sev_device *sev;
>  	int rc;
>  
> +	guard(mutex)(&sev_cmd_mutex)
> +
>  	if (!psp_master || !psp_master->sev_data)
>  		return -ENODEV;
>  
> @@ -1319,36 +1317,17 @@ static int _sev_platform_init_locked(struct sev_platform_init_args *args)
>  	if (sev->state == SEV_STATE_INIT)
>  		return 0;
>  
> -	/*
> -	 * Legacy guests cannot be running while SNP_INIT(_EX) is executing,
> -	 * so perform SEV-SNP initialization at probe time.
> -	 */
> -	rc = __sev_snp_init_locked(&args->error);
> +	rc = __sev_snp_init_locked(error);
>  	if (rc && rc != -ENODEV) {
>  		/*
>  		 * Don't abort the probe if SNP INIT failed,
>  		 * continue to initialize the legacy SEV firmware.
>  		 */
>  		dev_err(sev->dev, "SEV-SNP: failed to INIT rc %d, error %#x\n",
> -			rc, args->error);
> +			rc, *error);
>  	}
>  
> -	/* Defer legacy SEV/SEV-ES support if allowed by caller/module. */
> -	if (args->probe && !psp_init_on_probe)
> -		return 0;
> -
> -	return __sev_platform_init_locked(&args->error);
> -}
> -
> -int sev_platform_init(struct sev_platform_init_args *args)
> -{
> -	int rc;
> -
> -	mutex_lock(&sev_cmd_mutex);
> -	rc = _sev_platform_init_locked(args);
> -	mutex_unlock(&sev_cmd_mutex);
> -
> -	return rc;
> +	return __sev_platform_init_locked(error);
>  }
>  EXPORT_SYMBOL_GPL(sev_platform_init);
>  
> @@ -1441,16 +1420,12 @@ static int sev_ioctl_do_platform_status(struct sev_issue_cmd *argp)
>  static int sev_ioctl_do_pek_pdh_gen(int cmd, struct sev_issue_cmd *argp, bool writable)
>  {
>  	struct sev_device *sev = psp_master->sev_data;
> -	int rc;
>  
>  	if (!writable)
>  		return -EPERM;
>  
> -	if (sev->state == SEV_STATE_UNINIT) {
> -		rc = __sev_platform_init_locked(&argp->error);
> -		if (rc)
> -			return rc;
> -	}
> +	if (sev->state == SEV_STATE_UNINIT)
> +		return -ENOTTY;
>  
>  	return __sev_do_cmd_locked(cmd, NULL, &argp->error);
>  }
> @@ -1467,6 +1442,9 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
>  	if (!writable)
>  		return -EPERM;
>  
> +	if (sev->state == SEV_STATE_UNINIT)
> +		return -ENOTTY;
> +
>  	if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
>  		return -EFAULT;
>  
> @@ -1489,12 +1467,6 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
>  	data.len = input.length;
>  
>  cmd:
> -	if (sev->state == SEV_STATE_UNINIT) {
> -		ret = __sev_platform_init_locked(&argp->error);
> -		if (ret)
> -			goto e_free_blob;
> -	}
> -
>  	ret = __sev_do_cmd_locked(SEV_CMD_PEK_CSR, &data, &argp->error);
>  
>  	 /* If we query the CSR length, FW responded with expected data. */
> @@ -1584,7 +1556,6 @@ static int sev_get_firmware(struct device *dev,
>  	return -ENOENT;
>  }
>  
> -/* Don't fail if SEV FW couldn't be updated. Continue with existing SEV FW */
>  static int sev_update_firmware(struct device *dev)
>  {
>  	struct sev_data_download_firmware *data;
> @@ -1732,6 +1703,9 @@ static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp, bool writable)
>  	if (!writable)
>  		return -EPERM;
>  
> +	if (sev->state == SEV_STATE_UNINIT)
> +		return -ENOTTY;
> +
>  	if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
>  		return -EFAULT;
>  
> @@ -1754,16 +1728,8 @@ static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp, bool writable)
>  	data.oca_cert_address = __psp_pa(oca_blob);
>  	data.oca_cert_len = input.oca_cert_len;
>  
> -	/* If platform is not in INIT state then transition it to INIT */
> -	if (sev->state != SEV_STATE_INIT) {
> -		ret = __sev_platform_init_locked(&argp->error);
> -		if (ret)
> -			goto e_free_oca;
> -	}
> -
>  	ret = __sev_do_cmd_locked(SEV_CMD_PEK_CERT_IMPORT, &data, &argp->error);
>  
> -e_free_oca:
>  	kfree(oca_blob);
>  e_free_pek:
>  	kfree(pek_blob);
> @@ -1882,15 +1848,8 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
>  	void __user *input_pdh_cert_address;
>  	int ret;
>  
> -	/* If platform is not in INIT state then transition it to INIT. */
> -	if (sev->state != SEV_STATE_INIT) {
> -		if (!writable)
> -			return -EPERM;
> -
> -		ret = __sev_platform_init_locked(&argp->error);
> -		if (ret)
> -			return ret;
> -	}
> +	if (sev->state != SEV_STATE_INIT)
> +		return -ENOTTY;
>  
>  	if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
>  		return -EFAULT;
> @@ -2296,6 +2255,9 @@ static void __sev_firmware_shutdown(struct sev_device *sev, bool panic)
>  {
>  	int error;
>  
> +	atomic_notifier_chain_unregister(&panic_notifier_list,
> +					 &snp_panic_notifier);
> +
>  	__sev_platform_shutdown_locked(NULL);
>  
>  	if (sev_es_tmr) {
> @@ -2390,9 +2352,7 @@ EXPORT_SYMBOL_GPL(sev_issue_cmd_external_user);
>  void sev_pci_init(void)
>  {
>  	struct sev_device *sev = psp_master->sev_data;
> -	struct sev_platform_init_args args = {0};
>  	u8 api_major, api_minor, build;
> -	int rc;
>  
>  	if (!sev)
>  		return;
> @@ -2406,27 +2366,18 @@ void sev_pci_init(void)
>  	api_minor = sev->api_minor;
>  	build     = sev->build;
>  
> -	if (sev_update_firmware(sev->dev) == 0)
> -		sev_get_api_version();
> +	/* Don't fail if SEV FW couldn't be updated. Continue with existing SEV FW. */
> +	if (sev_update_firmware(sev->dev))
> +		return;
> +
> +	if (sev_get_api_version())
> +		goto err;
>  
>  	if (api_major != sev->api_major || api_minor != sev->api_minor ||
>  	    build != sev->build)
>  		dev_info(sev->dev, "SEV firmware updated from %d.%d.%d to %d.%d.%d\n",
>  			 api_major, api_minor, build,
>  			 sev->api_major, sev->api_minor, sev->build);
> -
> -	/* Initialize the platform */
> -	args.probe = true;
> -	rc = sev_platform_init(&args);
> -	if (rc)
> -		dev_err(sev->dev, "SEV: failed to INIT error %#x, rc %d\n",
> -			args.error, rc);
> -
> -	dev_info(sev->dev, "SEV%s API:%d.%d build:%d\n", sev->snp_initialized ?
> -		"-SNP" : "", sev->api_major, sev->api_minor, sev->build);
> -
> -	atomic_notifier_chain_register(&panic_notifier_list,
> -				       &snp_panic_notifier);
>  	return;
>  
>  err:
> @@ -2443,7 +2394,4 @@ void sev_pci_exit(void)
>  		return;
>  
>  	sev_firmware_shutdown(sev);
> -
> -	atomic_notifier_chain_unregister(&panic_notifier_list,
> -					 &snp_panic_notifier);
>  }
> diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
> index 903ddfea8585..def40f7ea01d 100644
> --- a/include/linux/psp-sev.h
> +++ b/include/linux/psp-sev.h
> @@ -790,19 +790,6 @@ struct sev_data_snp_shutdown_ex {
>  	u32 rsvd1:31;
>  } __packed;
>  
> -/**
> - * struct sev_platform_init_args
> - *
> - * @error: SEV firmware error code
> - * @probe: True if this is being called as part of CCP module probe, which
> - *  will defer SEV_INIT/SEV_INIT_EX firmware initialization until needed
> - *  unless psp_init_on_probe module param is set
> - */
> -struct sev_platform_init_args {
> -	int error;
> -	bool probe;
> -};
> -
>  /**
>   * struct sev_data_snp_commit - SNP_COMMIT structure
>   *
> @@ -817,7 +804,7 @@ struct sev_data_snp_commit {
>  /**
>   * sev_platform_init - perform SEV INIT command
>   *
> - * @args: struct sev_platform_init_args to pass in arguments
> + * @error: SEV firmware error code
>   *
>   * Returns:
>   * 0 if the SEV successfully processed the command
> @@ -826,7 +813,7 @@ struct sev_data_snp_commit {
>   * -%ETIMEDOUT if the SEV command timed out
>   * -%EIO       if the SEV returned a non-zero return code
>   */
> -int sev_platform_init(struct sev_platform_init_args *args);
> +int sev_platform_init(int *error);
>  
>  /**
>   * sev_platform_status - perform SEV PLATFORM_STATUS command
> @@ -951,7 +938,7 @@ void snp_free_firmware_page(void *addr);
>  static inline int
>  sev_platform_status(struct sev_user_data_status *status, int *error) { return -ENODEV; }
>  
> -static inline int sev_platform_init(struct sev_platform_init_args *args) { return -ENODEV; }
> +static inline int sev_platform_init(int *error) { return -ENODEV; }
>  
>  static inline int
>  sev_guest_deactivate(struct sev_data_deactivate *data, int *error) { return -ENODEV; }
> 
> base-commit: 43fb83c17ba2d63dfb798f0be7453ed55ca3f9c2

On Thu, Nov 21, 2024, Tom Lendacky wrote:
> On 11/21/24 10:56, Sean Christopherson wrote:
> > On Thu, Nov 21, 2024, Ashish Kalra wrote:
> > Actually, IMO, the behavior of _sev_platform_init_locked() and pretty much all of
> > the APIs that invoke it are flawed, and make all of this way more confusing and
> > convoluted than it needs to be.
> > 
> > IIUC, SNP initialization is forced during probe purely because SNP can't be
> > initialized if VMs are running.  But the only in-tree user of SEV-XXX functionality
> > is KVM, and KVM depends on whatever this driver is called.  So forcing SNP
> > initialization because a hypervisor could be running legacy VMs make no sense.
> > Just require KVM to initialize SEV functionality if KVM wants to use SEV+.
> 
> When we say legacy VMs, that also means non-SEV VMs. So you can't have any
> VM running within a VMRUN instruction.

Yeah, I know.  But if KVM initializes the PSP SEV stuff when KVM is loaded, then
KVM can't possibly be running VMs of any kind.

> Or...
> 
> > 
> > 	/*
> > 	 * Legacy guests cannot be running while SNP_INIT(_EX) is executing,
> > 	 * so perform SEV-SNP initialization at probe time.
> > 	 */
> > 	rc = __sev_snp_init_locked(&args->error); 
> > 
> > Rather than automatically init SEV+ functionality, can we instead do something
> > like the (half-baked pseudo-patch) below?  I.e. delete all paths that implicitly
> > init the PSP, and force KVM to explicitly initialize the PSP if KVM wants to use
> > SEV+.  Then we can put the CipherText and SNP ASID params in KVM.
> 
> ... do you mean at module load time (based on the module parameters)? Or
> when the first SEV VM is run? I would think the latter, as the parameters
> are all true by default. If the latter, that would present a problem of
> having to ensure no VMs are active while performing the SNP_INIT.

kvm-amd.ko load time.

> > That would also allow (a) registering the SNP panic notifier if and only if SNP
> > is actually initailized and (b) shutting down SEV+ in the PSP when KVM is unloaded.
> > Arguably, the PSP should be shutdown when KVM is unloaded, irrespective of the
> > CipherText and SNP ASID knobs.  But with those knobs, it becomes even more desirable,
> > because it would allow userspace to reload *KVM* in order to change the CipherText
> > and SNP ASID module params.  I.e. doesn't require unloading the entire CCP driver.
> > 
> > If dropping the implicit initialization in some of the ioctls would break existing
> > userspace, then maybe we could add a module param (or Kconfig?) to preserve that
> > behavior?  I'm not familiar with what actually uses /dev/sev.
> > 
> > Side topic #1, sev_pci_init() is buggy.  It should destroy SEV if getting the
> > API version fails after a firmware update.
> 
> True, we'll look at doing a fix for that.
> 
> > 
> > Side topic #2, the version check is broken, as it returns "success" when
> > initialization quite obviously failed.
> 
> That is ok because you can still initialize SEV / SEV-ES support.

Right, but as I've complained elsewhere, KVM shouldn't think SNP is supported
when in reality firmware is effectively too old.

On 11/21/2024 11:42 AM, Sean Christopherson wrote:
> On Thu, Nov 21, 2024, Tom Lendacky wrote:
>> On 11/21/24 10:56, Sean Christopherson wrote:
>>> On Thu, Nov 21, 2024, Ashish Kalra wrote:
>>> Actually, IMO, the behavior of _sev_platform_init_locked() and pretty much all of
>>> the APIs that invoke it are flawed, and make all of this way more confusing and
>>> convoluted than it needs to be.
>>>
>>> IIUC, SNP initialization is forced during probe purely because SNP can't be
>>> initialized if VMs are running.  But the only in-tree user of SEV-XXX functionality
>>> is KVM, and KVM depends on whatever this driver is called.  So forcing SNP
>>> initialization because a hypervisor could be running legacy VMs make no sense.
>>> Just require KVM to initialize SEV functionality if KVM wants to use SEV+.
>>
>> When we say legacy VMs, that also means non-SEV VMs. So you can't have any
>> VM running within a VMRUN instruction.
> 
> Yeah, I know.  But if KVM initializes the PSP SEV stuff when KVM is loaded, then
> KVM can't possibly be running VMs of any kind.
> 
>> Or...
>>
>>>
>>> 	/*
>>> 	 * Legacy guests cannot be running while SNP_INIT(_EX) is executing,
>>> 	 * so perform SEV-SNP initialization at probe time.
>>> 	 */
>>> 	rc = __sev_snp_init_locked(&args->error); 
>>>
>>> Rather than automatically init SEV+ functionality, can we instead do something
>>> like the (half-baked pseudo-patch) below?  I.e. delete all paths that implicitly
>>> init the PSP, and force KVM to explicitly initialize the PSP if KVM wants to use
>>> SEV+.  Then we can put the CipherText and SNP ASID params in KVM.
>>
>> ... do you mean at module load time (based on the module parameters)? Or
>> when the first SEV VM is run? I would think the latter, as the parameters
>> are all true by default. If the latter, that would present a problem of
>> having to ensure no VMs are active while performing the SNP_INIT.
> 
> kvm-amd.ko load time.

Ok, so kvm module load will init SEV+ if indicated by it's module parameters.

But, there are additional concerns here. 

SNP will still have to be initialized first, because SNP_INIT will fail if SEV INIT has been done.

Additionally, to support SEV firmware hotloading (DLFW_EX), SEV can't be initialized. 

So probably, we will have to retain some PSP style SEV+ initialization here, SNP_INIT is always
done first and then SEV INIT is skipped if explicitly specified by a module param. This allows
SEV firmware hotloading to be supported.

But, then with SEV firmware hotload support how do we do SEV INIT without unloading and reloading KVM module ?

This can reuse the current support (in KVM) to do SEV INIT implicitly when the first SEV VM is run:
sev_guest_init() -> sev_platform_init() 

> 
>>> That would also allow (a) registering the SNP panic notifier if and only if SNP
>>> is actually initailized and (b) shutting down SEV+ in the PSP when KVM is unloaded.
>>> Arguably, the PSP should be shutdown when KVM is unloaded, irrespective of the
>>> CipherText and SNP ASID knobs.  But with those knobs, it becomes even more desirable,
>>> because it would allow userspace to reload *KVM* in order to change the CipherText
>>> and SNP ASID module params.  I.e. doesn't require unloading the entire CCP driver.
>>>
>>> If dropping the implicit initialization in some of the ioctls would break existing
>>> userspace, then maybe we could add a module param (or Kconfig?) to preserve that
>>> behavior?  I'm not familiar with what actually uses /dev/sev.
>>>

Yes, i do think this can be an issue as those ioctl's may be in use by userspace tools like
sevtool, etc., and require SEV implicit initialization and we will have to preserve this 
behavior if indicated by a PSP module param.

Thanks,
Ashish

>>> Side topic #1, sev_pci_init() is buggy.  It should destroy SEV if getting the
>>> API version fails after a firmware update.
>>
>> True, we'll look at doing a fix for that.
>>
>>>
>>> Side topic #2, the version check is broken, as it returns "success" when
>>> initialization quite obviously failed.
>>
>> That is ok because you can still initialize SEV / SEV-ES support.
> 
> Right, but as I've complained elsewhere, KVM shouldn't think SNP is supported
> when in reality firmware is effectively too old.

On Thu, Nov 21, 2024, Ashish Kalra wrote:
> On 11/21/2024 11:42 AM, Sean Christopherson wrote:
> > On Thu, Nov 21, 2024, Tom Lendacky wrote:
> >> On 11/21/24 10:56, Sean Christopherson wrote:
> >>> On Thu, Nov 21, 2024, Ashish Kalra wrote:
> >>> Actually, IMO, the behavior of _sev_platform_init_locked() and pretty much all of
> >>> the APIs that invoke it are flawed, and make all of this way more confusing and
> >>> convoluted than it needs to be.
> >>>
> >>> IIUC, SNP initialization is forced during probe purely because SNP can't be
> >>> initialized if VMs are running.  But the only in-tree user of SEV-XXX functionality
> >>> is KVM, and KVM depends on whatever this driver is called.  So forcing SNP
> >>> initialization because a hypervisor could be running legacy VMs make no sense.
> >>> Just require KVM to initialize SEV functionality if KVM wants to use SEV+.
> >>
> >> When we say legacy VMs, that also means non-SEV VMs. So you can't have any
> >> VM running within a VMRUN instruction.
> > 
> > Yeah, I know.  But if KVM initializes the PSP SEV stuff when KVM is loaded, then
> > KVM can't possibly be running VMs of any kind.
> > 
> >> Or...
> >>
> >>>
> >>> 	/*
> >>> 	 * Legacy guests cannot be running while SNP_INIT(_EX) is executing,
> >>> 	 * so perform SEV-SNP initialization at probe time.
> >>> 	 */
> >>> 	rc = __sev_snp_init_locked(&args->error); 
> >>>
> >>> Rather than automatically init SEV+ functionality, can we instead do something
> >>> like the (half-baked pseudo-patch) below?  I.e. delete all paths that implicitly
> >>> init the PSP, and force KVM to explicitly initialize the PSP if KVM wants to use
> >>> SEV+.  Then we can put the CipherText and SNP ASID params in KVM.
> >>
> >> ... do you mean at module load time (based on the module parameters)? Or
> >> when the first SEV VM is run? I would think the latter, as the parameters
> >> are all true by default. If the latter, that would present a problem of
> >> having to ensure no VMs are active while performing the SNP_INIT.
> > 
> > kvm-amd.ko load time.
> 
> Ok, so kvm module load will init SEV+ if indicated by it's module parameters.
> 
> But, there are additional concerns here. 
> 
> SNP will still have to be initialized first, because SNP_INIT will fail if
> SEV INIT has been done.
> 
> Additionally, to support SEV firmware hotloading (DLFW_EX), SEV can't be
> initialized. 
> 
> So probably, we will have to retain some PSP style SEV+ initialization here,
> SNP_INIT is always done first and then SEV INIT is skipped if explicitly
> specified by a module param. This allows SEV firmware hotloading to be
> supported.
> 
> But, then with SEV firmware hotload support how do we do SEV INIT without
> unloading and reloading KVM module ?

So the above says:

 SEV_CMD_SNP_INIT{_ES} cannot be executed if SEV_CMD_INIT{_EX} has been executed.

but the existing comment in _sev_platform_init_locked() says:

	/*
	 * Legacy guests cannot be running while SNP_INIT(_EX) is executing,
	 * so perform SEV-SNP initialization at probe time.
	 */

Which one is correct?  I don't think it matters in the end, just trying to wrap my
head around everything.

And IIUC, SEV_CMD_SNP_INIT{_EX} can be executed before firmware hotload, but
SEV_CMD_INIT{_EX} cannot.  Is that correct?  Because if firmware hotload can't
be done while SEV VMs are _active_, then that's a very different situation.

> This can reuse the current support (in KVM) to do SEV INIT implicitly when
> the first SEV VM is run: sev_guest_init() -> sev_platform_init() 

I don't love the implicit behavior, but assuming hotloading firmware can't be done
after SEV_CMD_INIT{_EX}, that does seem like the least awful solution.

To summarize, if the above assumptions hold:

 1. Initialize SNP when kvm-amd.ko is loaded.
 2. Define CipherTextHiding and ASID params kvm-amd.ko.
 3. Initialize SEV+ at first use.

Just to triple check: that will allow firmware hotloading even if kvm-amd.ko is
built-in, correct?  I.e. doesn't requires deferring kvm-amd.ko load until after
firmware hotloading.

Hello Sean, 

Please see my responses below:

On 12/6/2024 4:30 PM, Sean Christopherson wrote:
> On Thu, Nov 21, 2024, Ashish Kalra wrote:
>> On 11/21/2024 11:42 AM, Sean Christopherson wrote:
>>> On Thu, Nov 21, 2024, Tom Lendacky wrote:
>>>> On 11/21/24 10:56, Sean Christopherson wrote:
>>>>> On Thu, Nov 21, 2024, Ashish Kalra wrote:
>>>>> Actually, IMO, the behavior of _sev_platform_init_locked() and pretty much all of
>>>>> the APIs that invoke it are flawed, and make all of this way more confusing and
>>>>> convoluted than it needs to be.
>>>>>
>>>>> IIUC, SNP initialization is forced during probe purely because SNP can't be
>>>>> initialized if VMs are running.  But the only in-tree user of SEV-XXX functionality
>>>>> is KVM, and KVM depends on whatever this driver is called.  So forcing SNP
>>>>> initialization because a hypervisor could be running legacy VMs make no sense.
>>>>> Just require KVM to initialize SEV functionality if KVM wants to use SEV+.
>>>>
>>>> When we say legacy VMs, that also means non-SEV VMs. So you can't have any
>>>> VM running within a VMRUN instruction.
>>>
>>> Yeah, I know.  But if KVM initializes the PSP SEV stuff when KVM is loaded, then
>>> KVM can't possibly be running VMs of any kind.
>>>
>>>> Or...
>>>>
>>>>>
>>>>> 	/*
>>>>> 	 * Legacy guests cannot be running while SNP_INIT(_EX) is executing,
>>>>> 	 * so perform SEV-SNP initialization at probe time.
>>>>> 	 */
>>>>> 	rc = __sev_snp_init_locked(&args->error); 
>>>>>
>>>>> Rather than automatically init SEV+ functionality, can we instead do something
>>>>> like the (half-baked pseudo-patch) below?  I.e. delete all paths that implicitly
>>>>> init the PSP, and force KVM to explicitly initialize the PSP if KVM wants to use
>>>>> SEV+.  Then we can put the CipherText and SNP ASID params in KVM.
>>>>
>>>> ... do you mean at module load time (based on the module parameters)? Or
>>>> when the first SEV VM is run? I would think the latter, as the parameters
>>>> are all true by default. If the latter, that would present a problem of
>>>> having to ensure no VMs are active while performing the SNP_INIT.
>>>
>>> kvm-amd.ko load time.
>>
>> Ok, so kvm module load will init SEV+ if indicated by it's module parameters.
>>
>> But, there are additional concerns here. 
>>
>> SNP will still have to be initialized first, because SNP_INIT will fail if
>> SEV INIT has been done.
>>
>> Additionally, to support SEV firmware hotloading (DLFW_EX), SEV can't be
>> initialized. 
>>
>> So probably, we will have to retain some PSP style SEV+ initialization here,
>> SNP_INIT is always done first and then SEV INIT is skipped if explicitly
>> specified by a module param. This allows SEV firmware hotloading to be
>> supported.
>>
>> But, then with SEV firmware hotload support how do we do SEV INIT without
>> unloading and reloading KVM module ?
> 
> So the above says:
> 
>  SEV_CMD_SNP_INIT{_ES} cannot be executed if SEV_CMD_INIT{_EX} has been executed.
> 
> but the existing comment in _sev_platform_init_locked() says:
> 
> 	/*
> 	 * Legacy guests cannot be running while SNP_INIT(_EX) is executing,
> 	 * so perform SEV-SNP initialization at probe time.
> 	 */
> 
> Which one is correct?  I don't think it matters in the end, just trying to wrap my
> head around everything.

Here is a summary:

1). SNP_INIT(_EX) cannot be done if SEV INIT has been done, the above comment mentions the same,  
in other words, if SEV INIT has completed successfully then any subsequent SNP_INIT(_EX) will fail.

2). Also if SNP is enabled (SNPEn) system-wide, which is done during early kernel boot, then 
SNP_INIT_EX has to be done before SEV INIT, otherwise SEV INIT will fail. 

SNP is enabled system-wide if SNP support and RMP table support is enabled in BIOS and 
additionally RMP table sanity checks are successful, RMP table is mapped and SNP support is
enabled on the IOMMU.

This also means that if we only want to launch legacy VMs (SEV/SEV-ES), but SNP has been enabled
at early kernel boot then we still need to do SNP_INIT(_EX) before SEV INIT. 

Considering all these, SNP_INIT(_EX) is always attempted before SEV INIT as above.

If SNP is not enabled system-wide (CC_ATTR_HOST_SEV_SNP is not set), then SNP_INIT(_EX) will
be skipped.

If we specify KVM module parameter sev_snp=false, but if SNPEn is set system-wide, we
will still do SNP_INIT(_EX) before SEV INIT to ensure that SEV INIT is successful. 
But do note, in this configuration launching any SNP VMs will fail.

> 
> And IIUC, SEV_CMD_SNP_INIT{_EX} can be executed before firmware hotload, but
> SEV_CMD_INIT{_EX} cannot.  Is that correct?  Because if firmware hotload can't
> be done while SEV VMs are _active_, then that's a very different situation.
> 

Yes, that is true. As per SNP Firmware API specs, SNP DOWNLOAD_FIRMWARE_EX adds support 
for provisional updates and for updates while SNP firmware is in the INIT state.

The SNP firmware may be in any state. SEV must be in the UNINIT state.

>> This can reuse the current support (in KVM) to do SEV INIT implicitly when
>> the first SEV VM is run: sev_guest_init() -> sev_platform_init() 
> 
> I don't love the implicit behavior, but assuming hotloading firmware can't be done
> after SEV_CMD_INIT{_EX}, that does seem like the least awful solution.
> 
> To summarize, if the above assumptions hold:
> 
>  1. Initialize SNP when kvm-amd.ko is loaded.
>  2. Define CipherTextHiding and ASID params kvm-amd.ko.
>  3. Initialize SEV+ at first use.

Yes, the above summary is correct except for (3).

The initial set of patches will initialize SNP and SEV both at kvm-amd.ko module load,
similar to PSP module load/probe time.

For backward compatibility, the PSP module parameter psp_init_on_probe will still be
supported, i believe it is used for INIT_EX support.

I have the base set of patches ready which remove SEV/SNP platform initialization from
PSP module probe time and instead move it to KVM module load time (sev_hardware_setup)
and similarly for platform shutdown.

I will post this patch-set early next week.

On top of this base patch-set we will push the SNP CipherTextHiding support and SEV
firmware hotloading support. The SEV firmware hotloading support will add support
for (3) above.

> 
> Just to triple check: that will allow firmware hotloading even if kvm-amd.ko is
> built-in, correct?  I.e. doesn't requires deferring kvm-amd.ko load until after
> firmware hotloading.

Yes, this should work, for supporting firmware hotloading, the PSP driver's
psp_init_on_probe parameter will need to be set to false, which will ensure
that SEV INIT is not done during SEV/SNP platform initialization at KVM module
probe time and instead it will be done implicitly at first SEV/SEV-ES VM launch.

Hopefully, this summarizes all the possible configurations for SEV/SNP platform
initialization.

Thanks,
Ashish

On Fri, Dec 06, 2024, Ashish Kalra wrote:
> On 12/6/2024 4:30 PM, Sean Christopherson wrote:
> >> This can reuse the current support (in KVM) to do SEV INIT implicitly when
> >> the first SEV VM is run: sev_guest_init() -> sev_platform_init() 
> > 
> > I don't love the implicit behavior, but assuming hotloading firmware can't be done
> > after SEV_CMD_INIT{_EX}, that does seem like the least awful solution.
> > 
> > To summarize, if the above assumptions hold:
> > 
> >  1. Initialize SNP when kvm-amd.ko is loaded.
> >  2. Define CipherTextHiding and ASID params kvm-amd.ko.
> >  3. Initialize SEV+ at first use.
> 
> Yes, the above summary is correct except for (3).

Heh, that wasn't a statement of fast, it was a suggestion for a possible
implementation.

> The initial set of patches will initialize SNP and SEV both at kvm-amd.ko module load,
> similar to PSP module load/probe time.

Why?  If SEV+ is initialized at kvm-amd.ko load, doesn't that prevent firmware
hotloading?

> For backward compatibility, the PSP module parameter psp_init_on_probe will still be
> supported, i believe it is used for INIT_EX support.

Again, why?  If the only use of psp_init_on_probe is to _disable_ that behavior,
and we make the code never init-on-probe, then the param is unnecessary, no?

> > Just to triple check: that will allow firmware hotloading even if kvm-amd.ko is
> > built-in, correct?  I.e. doesn't requires deferring kvm-amd.ko load until after
> > firmware hotloading.
> 
> Yes, this should work, for supporting firmware hotloading, the PSP driver's
> psp_init_on_probe parameter will need to be set to false, which will ensure
> that SEV INIT is not done during SEV/SNP platform initialization at KVM module
> probe time and instead it will be done implicitly at first SEV/SEV-ES VM launch.

Please no.  I really, really don't want gunk like this in KVM:

	init_args.probe = false;
	ret = sev_platform_init(&init_args);

That's inscrutable without a verbose comment, and all kinds of ugly.  Why can't
we simply separate SNP initialization from SEV+ initialization?

Hello Sean,

On 12/9/2024 7:30 PM, Sean Christopherson wrote:
> On Fri, Dec 06, 2024, Ashish Kalra wrote:
>> On 12/6/2024 4:30 PM, Sean Christopherson wrote:
>>>> This can reuse the current support (in KVM) to do SEV INIT implicitly when
>>>> the first SEV VM is run: sev_guest_init() -> sev_platform_init() 
>>>
>>> I don't love the implicit behavior, but assuming hotloading firmware can't be done
>>> after SEV_CMD_INIT{_EX}, that does seem like the least awful solution.
>>>
>>> To summarize, if the above assumptions hold:
>>>
>>>  1. Initialize SNP when kvm-amd.ko is loaded.
>>>  2. Define CipherTextHiding and ASID params kvm-amd.ko.
>>>  3. Initialize SEV+ at first use.
>>
>> Yes, the above summary is correct except for (3).
> 
> Heh, that wasn't a statement of fast, it was a suggestion for a possible
> implementation.
> 
>> The initial set of patches will initialize SNP and SEV both at kvm-amd.ko module load,
>> similar to PSP module load/probe time.
> 
> Why?  If SEV+ is initialized at kvm-amd.ko load, doesn't that prevent firmware
> hotloading?

Yes it does, i was thinking of fixing it as part of a series on top of these patches
to support SEV firmware hotloading.

> 
>> For backward compatibility, the PSP module parameter psp_init_on_probe will still be
>> supported, i believe it is used for INIT_EX support.
> 
> Again, why?  If the only use of psp_init_on_probe is to _disable_ that behavior,
> and we make the code never init-on-probe, then the param is unnecessary, no?

Yes, it makes sense to remove this param.

> 
>>> Just to triple check: that will allow firmware hotloading even if kvm-amd.ko is
>>> built-in, correct?  I.e. doesn't requires deferring kvm-amd.ko load until after
>>> firmware hotloading.
>>
>> Yes, this should work, for supporting firmware hotloading, the PSP driver's
>> psp_init_on_probe parameter will need to be set to false, which will ensure
>> that SEV INIT is not done during SEV/SNP platform initialization at KVM module
>> probe time and instead it will be done implicitly at first SEV/SEV-ES VM launch.
> 
> Please no.  I really, really don't want gunk like this in KVM:
> 
> 	init_args.probe = false;
> 	ret = sev_platform_init(&init_args);
> 
> That's inscrutable without a verbose comment, and all kinds of ugly.  Why can't
> we simply separate SNP initialization from SEV+ initialization?

Yes we can do that, by default KVM module load time will only do SNP initialization,
and then we will do SEV initialization if a SEV VM is being launched.

This will remove the probe parameter from init_args above, but will need to add another
parameter like VM type to specify if SNP or SEV initialization is to be performed with
the sev_platform_init() call.

Will work on re-posting my series with SNP initialization separated from SEV
initialization.

Thanks,
Ashish

On Tue, Dec 10, 2024, Ashish Kalra wrote:
> On 12/9/2024 7:30 PM, Sean Christopherson wrote:
> > Why can't we simply separate SNP initialization from SEV+ initialization?
> 
> Yes we can do that, by default KVM module load time will only do SNP initialization,
> and then we will do SEV initialization if a SEV VM is being launched.
> 
> This will remove the probe parameter from init_args above, but will need to add another
> parameter like VM type to specify if SNP or SEV initialization is to be performed with
> the sev_platform_init() call.

Any reason not to simply use separate APIs?  E.g. sev_snp_platform_init() and
sev_platform_init()?

And if the cc_platform_has(CC_ATTR_HOST_SEV_SNP) check is moved inside of
sev_snp_platform_init() (probably needs to be there anyways), then the KVM code
is quite simple and will undergo minimal churn.

E.g.

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 5e4581ed0ef1..7e75bc55d017 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -404,7 +404,6 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
                            unsigned long vm_type)
 {
        struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
-       struct sev_platform_init_args init_args = {0};
        bool es_active = vm_type != KVM_X86_SEV_VM;
        u64 valid_vmsa_features = es_active ? sev_supported_vmsa_features : 0;
        int ret;
@@ -444,8 +443,7 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
        if (ret)
                goto e_no_asid;
 
-       init_args.probe = false;
-       ret = sev_platform_init(&init_args);
+       ret = sev_platform_init();
        if (ret)
                goto e_free;
 
@@ -3053,7 +3051,7 @@ void __init sev_hardware_setup(void)
        sev_es_asid_count = min_sev_asid - 1;
        WARN_ON_ONCE(misc_cg_set_capacity(MISC_CG_RES_SEV_ES, sev_es_asid_count));
        sev_es_supported = true;
-       sev_snp_supported = sev_snp_enabled && cc_platform_has(CC_ATTR_HOST_SEV_SNP);
+       sev_snp_supported = sev_snp_enabled && !sev_snp_platform_init();
 
 out:
        if (boot_cpu_has(X86_FEATURE_SEV))

On 12/10/2024 4:57 PM, Sean Christopherson wrote:
> On Tue, Dec 10, 2024, Ashish Kalra wrote:
>> On 12/9/2024 7:30 PM, Sean Christopherson wrote:
>>> Why can't we simply separate SNP initialization from SEV+ initialization?
>>
>> Yes we can do that, by default KVM module load time will only do SNP initialization,
>> and then we will do SEV initialization if a SEV VM is being launched.
>>
>> This will remove the probe parameter from init_args above, but will need to add another
>> parameter like VM type to specify if SNP or SEV initialization is to be performed with
>> the sev_platform_init() call.
> 
> Any reason not to simply use separate APIs?  E.g. sev_snp_platform_init() and
> sev_platform_init()?

One reason is the need to do SEV SHUTDOWN before SNP_SHUTDOWN if any SEV VMs are active
and this is taken care with the single API interface sev_platform_shutdown(), so that's 
why considering using a consistent API interface for both INIT and SHUTDOWN ...
- sev_platform_init()
- sev_platform_shutdown()

We can use separate APIs, but then we probably need the same for shutdown too and KVM
will need to keep track of any active SEV VMs and ensure to call sev_platform_shutdown()
before sev_snp_platform_shutdown() (as part of sev_hardware_unsetup()).

Thanks,
Ashish

> 
> And if the cc_platform_has(CC_ATTR_HOST_SEV_SNP) check is moved inside of
> sev_snp_platform_init() (probably needs to be there anyways), then the KVM code
> is quite simple and will undergo minimal churn.
> 
> E.g.
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 5e4581ed0ef1..7e75bc55d017 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -404,7 +404,6 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
>                             unsigned long vm_type)
>  {
>         struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> -       struct sev_platform_init_args init_args = {0};
>         bool es_active = vm_type != KVM_X86_SEV_VM;
>         u64 valid_vmsa_features = es_active ? sev_supported_vmsa_features : 0;
>         int ret;
> @@ -444,8 +443,7 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
>         if (ret)
>                 goto e_no_asid;
>  
> -       init_args.probe = false;
> -       ret = sev_platform_init(&init_args);
> +       ret = sev_platform_init();
>         if (ret)
>                 goto e_free;
>  
> @@ -3053,7 +3051,7 @@ void __init sev_hardware_setup(void)
>         sev_es_asid_count = min_sev_asid - 1;
>         WARN_ON_ONCE(misc_cg_set_capacity(MISC_CG_RES_SEV_ES, sev_es_asid_count));
>         sev_es_supported = true;
> -       sev_snp_supported = sev_snp_enabled && cc_platform_has(CC_ATTR_HOST_SEV_SNP);
> +       sev_snp_supported = sev_snp_enabled && !sev_snp_platform_init();
>  
>  out:
>         if (boot_cpu_has(X86_FEATURE_SEV))

On 12/10/2024 6:48 PM, Kalra, Ashish wrote:
> 
> 
> On 12/10/2024 4:57 PM, Sean Christopherson wrote:
>> On Tue, Dec 10, 2024, Ashish Kalra wrote:
>>> On 12/9/2024 7:30 PM, Sean Christopherson wrote:
>>>> Why can't we simply separate SNP initialization from SEV+ initialization?
>>>
>>> Yes we can do that, by default KVM module load time will only do SNP initialization,
>>> and then we will do SEV initialization if a SEV VM is being launched.
>>>
>>> This will remove the probe parameter from init_args above, but will need to add another
>>> parameter like VM type to specify if SNP or SEV initialization is to be performed with
>>> the sev_platform_init() call.
>>
>> Any reason not to simply use separate APIs?  E.g. sev_snp_platform_init() and
>> sev_platform_init()?
> 
> One reason is the need to do SEV SHUTDOWN before SNP_SHUTDOWN if any SEV VMs are active
> and this is taken care with the single API interface sev_platform_shutdown(), so that's 
> why considering using a consistent API interface for both INIT and SHUTDOWN ...
> - sev_platform_init()
> - sev_platform_shutdown()

Which also assists in using the same internal interface __sev_firmware_shutdown()
to be called both with sev_platform_shutdown() and the SNP panic notifier to shutdown
both SEV and SNP (in that order). 

Thanks,
Ashish

> 
> We can use separate APIs, but then we probably need the same for shutdown too and KVM
> will need to keep track of any active SEV VMs and ensure to call sev_platform_shutdown()
> before sev_snp_platform_shutdown() (as part of sev_hardware_unsetup()).
> 
> Thanks,
> Ashish
> 
>>
>> And if the cc_platform_has(CC_ATTR_HOST_SEV_SNP) check is moved inside of
>> sev_snp_platform_init() (probably needs to be there anyways), then the KVM code
>> is quite simple and will undergo minimal churn.
>>
>> E.g.
>>
>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>> index 5e4581ed0ef1..7e75bc55d017 100644
>> --- a/arch/x86/kvm/svm/sev.c
>> +++ b/arch/x86/kvm/svm/sev.c
>> @@ -404,7 +404,6 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
>>                             unsigned long vm_type)
>>  {
>>         struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
>> -       struct sev_platform_init_args init_args = {0};
>>         bool es_active = vm_type != KVM_X86_SEV_VM;
>>         u64 valid_vmsa_features = es_active ? sev_supported_vmsa_features : 0;
>>         int ret;
>> @@ -444,8 +443,7 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
>>         if (ret)
>>                 goto e_no_asid;
>>  
>> -       init_args.probe = false;
>> -       ret = sev_platform_init(&init_args);
>> +       ret = sev_platform_init();
>>         if (ret)
>>                 goto e_free;
>>  
>> @@ -3053,7 +3051,7 @@ void __init sev_hardware_setup(void)
>>         sev_es_asid_count = min_sev_asid - 1;
>>         WARN_ON_ONCE(misc_cg_set_capacity(MISC_CG_RES_SEV_ES, sev_es_asid_count));
>>         sev_es_supported = true;
>> -       sev_snp_supported = sev_snp_enabled && cc_platform_has(CC_ATTR_HOST_SEV_SNP);
>> +       sev_snp_supported = sev_snp_enabled && !sev_snp_platform_init();
>>  
>>  out:
>>         if (boot_cpu_has(X86_FEATURE_SEV))
>

On 12/10/2024 7:01 PM, Kalra, Ashish wrote:
> 
> 
> On 12/10/2024 6:48 PM, Kalra, Ashish wrote:
>>
>>
>> On 12/10/2024 4:57 PM, Sean Christopherson wrote:
>>> On Tue, Dec 10, 2024, Ashish Kalra wrote:
>>>> On 12/9/2024 7:30 PM, Sean Christopherson wrote:
>>>>> Why can't we simply separate SNP initialization from SEV+ initialization?
>>>>
>>>> Yes we can do that, by default KVM module load time will only do SNP initialization,
>>>> and then we will do SEV initialization if a SEV VM is being launched.
>>>>
>>>> This will remove the probe parameter from init_args above, but will need to add another
>>>> parameter like VM type to specify if SNP or SEV initialization is to be performed with
>>>> the sev_platform_init() call.
>>>
>>> Any reason not to simply use separate APIs?  E.g. sev_snp_platform_init() and
>>> sev_platform_init()?
>>
>> One reason is the need to do SEV SHUTDOWN before SNP_SHUTDOWN if any SEV VMs are active
>> and this is taken care with the single API interface sev_platform_shutdown(), so that's 
>> why considering using a consistent API interface for both INIT and SHUTDOWN ...
>> - sev_platform_init()
>> - sev_platform_shutdown()
> 
> Which also assists in using the same internal interface __sev_firmware_shutdown()
> to be called both with sev_platform_shutdown() and the SNP panic notifier to shutdown
> both SEV and SNP (in that order). 
> 
> Thanks,
> Ashish
> 
>>
>> We can use separate APIs, but then we probably need the same for shutdown too and KVM
>> will need to keep track of any active SEV VMs and ensure to call sev_platform_shutdown()
>> before sev_snp_platform_shutdown() (as part of sev_hardware_unsetup()).
>>

Worked on separating SEV and SNP initialization and got it working, but it needs 
additional fix in qemu to remove the check for SEV-ES being already initialized (i.e, SEV
INIT being already done) as below to ensure that SEV INIT is done on demand when
SEV/SEV-ES guests are being launched: 

diff --git a/target/i386/sev.c b/target/i386/sev.c
index a0d271f898..bb541f9d41 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -1503,15 +1503,6 @@ static int sev_common_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
         }
     }

-    if (sev_es_enabled() && !sev_snp_enabled()) {
-        if (!(status.flags & SEV_STATUS_FLAGS_CONFIG_ES)) {
-            error_setg(errp, "%s: guest policy requires SEV-ES, but "
-                         "host SEV-ES support unavailable",
-                         __func__);
-            return -1;
-        }
-    }
-
     trace_kvm_sev_init();
     switch (x86_klass->kvm_type(X86_CONFIDENTIAL_GUEST(sev_common))) {
     case KVM_X86_DEFAULT_VM:
>>
>>>
>>> And if the cc_platform_has(CC_ATTR_HOST_SEV_SNP) check is moved inside of
>>> sev_snp_platform_init() (probably needs to be there anyways), then the KVM code
>>> is quite simple and will undergo minimal churn.
>>>
>>> E.g.
>>>
>>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>>> index 5e4581ed0ef1..7e75bc55d017 100644
>>> --- a/arch/x86/kvm/svm/sev.c
>>> +++ b/arch/x86/kvm/svm/sev.c
>>> @@ -404,7 +404,6 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
>>>                             unsigned long vm_type)
>>>  {
>>>         struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
>>> -       struct sev_platform_init_args init_args = {0};
>>>         bool es_active = vm_type != KVM_X86_SEV_VM;
>>>         u64 valid_vmsa_features = es_active ? sev_supported_vmsa_features : 0;
>>>         int ret;
>>> @@ -444,8 +443,7 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
>>>         if (ret)
>>>                 goto e_no_asid;
>>>  
>>> -       init_args.probe = false;
>>> -       ret = sev_platform_init(&init_args);
>>> +       ret = sev_platform_init();
>>>         if (ret)
>>>                 goto e_free;
>>>  
>>> @@ -3053,7 +3051,7 @@ void __init sev_hardware_setup(void)
>>>         sev_es_asid_count = min_sev_asid - 1;
>>>         WARN_ON_ONCE(misc_cg_set_capacity(MISC_CG_RES_SEV_ES, sev_es_asid_count));
>>>         sev_es_supported = true;
>>> -       sev_snp_supported = sev_snp_enabled && cc_platform_has(CC_ATTR_HOST_SEV_SNP);
>>> +       sev_snp_supported = sev_snp_enabled && !sev_snp_platform_init();
>>>  
>>>  out:
>>>         if (boot_cpu_has(X86_FEATURE_SEV))
>>
>