Re: [RFC PATCH v2 04/12] i386/sev: initialize SNP context

[Dov Murik]

Hi Michael,

On 27/08/2021 1:26, Michael Roth wrote:
> From: Brijesh Singh <brijesh.singh@amd.com>
> 
> When SEV-SNP is enabled, the KVM_SNP_INIT command is used to initialize
> the platform. The command checks whether SNP is enabled in the KVM, if
> enabled then it allocates a new ASID from the SNP pool and calls the
> firmware to initialize the all the resources.
> 
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> Signed-off-by: Michael Roth <michael.roth@amd.com>
> ---
>  target/i386/sev-stub.c |  6 ++++++
>  target/i386/sev.c      | 27 ++++++++++++++++++++++++---
>  target/i386/sev_i386.h |  1 +
>  3 files changed, 31 insertions(+), 3 deletions(-)
> 
> diff --git a/target/i386/sev-stub.c b/target/i386/sev-stub.c
> index 0227cb5177..e4fb8e882e 100644
> --- a/target/i386/sev-stub.c
> +++ b/target/i386/sev-stub.c
> @@ -81,3 +81,9 @@ sev_get_attestation_report(const char *mnonce, Error **errp)
>      error_setg(errp, "SEV is not available in this QEMU");
>      return NULL;
>  }
> +
> +bool
> +sev_snp_enabled(void)
> +{
> +    return false;
> +}
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index ba08b7d3ab..b8bd6ed9ea 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -614,12 +614,21 @@ sev_enabled(void)
>      return !!object_dynamic_cast(OBJECT(cgs), TYPE_SEV_COMMON);
>  }
>  
> +bool
> +sev_snp_enabled(void)
> +{
> +    ConfidentialGuestSupport *cgs = MACHINE(qdev_get_machine())->cgs;
> +
> +    return !!object_dynamic_cast(OBJECT(cgs), TYPE_SEV_SNP_GUEST);
> +}
> +
>  bool
>  sev_es_enabled(void)
>  {
>      ConfidentialGuestSupport *cgs = MACHINE(qdev_get_machine())->cgs;
>  
> -    return sev_enabled() && (SEV_GUEST(cgs)->policy & SEV_POLICY_ES);
> +    return sev_snp_enabled() ||
> +            (sev_enabled() && SEV_GUEST(cgs)->policy & SEV_POLICY_ES);
>  }
>  
>  uint64_t
> @@ -1074,6 +1083,7 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error 
> **errp)
>      uint32_t ebx;
>      uint32_t host_cbitpos;
>      struct sev_user_data_status status = {};
> +    void *init_args = NULL;
>  
>      if (!sev_common) {
>          return 0;
> @@ -1126,7 +1136,18 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error 
> **errp)
>      sev_common->api_major = status.api_major;
>      sev_common->api_minor = status.api_minor;

Not visible here in the context: the code here is using the
SEV_PLATFORM_STATUS command to get the build_id, api_major, and api_minor.

I see that SNP has a new command SNP_PLATFORM_STATUS, which fills a
struct sev_data_snp_platform_status (hmmm, I can't find the struct's
definition; I assume it should look like Table 38 in 8.3.2 in SNP FW ABI
document).

My questions are:

1. Is it OK to call the "legacy" SEV_PLATFORM_STATUS when about to init
an SNP guest?
2. Do we want to save some info like installed TCB version and reported
TCB version, and maybe other fields from SNP platform status?
3. Should we check the state field in the platform status?



>  
> -    if (sev_es_enabled()) {
> +    if (sev_snp_enabled()) {
> +        SevSnpGuestState *sev_snp_guest = SEV_SNP_GUEST(sev_common);
> +        if (!kvm_kernel_irqchip_allowed()) {
> +            error_report("%s: SEV-SNP guests require in-kernel irqchip 
> support",
> +                         __func__);

Most errors in this function use error_setg(errp, ...).  This should follow.


> +            goto err;
> +        }
> +
> +        cmd = KVM_SEV_SNP_INIT;
> +        init_args = (void *)&sev_snp_guest->kvm_init_conf;
> +
> +    } else if (sev_es_enabled()) {
>          if (!kvm_kernel_irqchip_allowed()) {
>              error_report("%s: SEV-ES guests require in-kernel irqchip 
> support",
>                           __func__);

Not part of this patch, but this error_report (and another one in the
SEV-ES case) should be converted to error_setg similarly.  Maybe add a
separate patch for fixing this for SEV-ES.



> @@ -1145,7 +1166,7 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error 
> **errp)
>      }
>  
>      trace_kvm_sev_init();

Suggestions:

1. log the guest type (SEV / SEV-ES / SEV-SNP)
2. log the SNP init flags value when initializing an SNP guest


-Dov

> -    ret = sev_ioctl(sev_common->sev_fd, cmd, NULL, &fw_error);
> +    ret = sev_ioctl(sev_common->sev_fd, cmd, init_args, &fw_error);
>      if (ret) {
>          error_setg(errp, "%s: failed to initialize ret=%d fw_error=%d '%s'",
>                     __func__, ret, fw_error, fw_error_to_str(fw_error));
> diff --git a/target/i386/sev_i386.h b/target/i386/sev_i386.h
> index ae6d840478..e0e1a599be 100644
> --- a/target/i386/sev_i386.h
> +++ b/target/i386/sev_i386.h
> @@ -29,6 +29,7 @@
>  #define SEV_POLICY_SEV          0x20
>  
>  extern bool sev_es_enabled(void);
> +extern bool sev_snp_enabled(void);
>  extern uint64_t sev_get_me_mask(void);
>  extern SevInfo *sev_get_info(void);
>  extern uint32_t sev_get_cbit_position(void);
>