Re: [RFC PATCH 4/6] i386/sev: add the SNP launch start context

[Dov Murik]

Hi Brijesh,

On 10/07/2021 0:55, Brijesh Singh wrote:
> The SNP_LAUNCH_START is called first to create a cryptographic launch
> context within the firmware.
> 
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  target/i386/sev.c        | 30 +++++++++++++++++++++++++++++-
>  target/i386/trace-events |  1 +
>  2 files changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 84ae244af0..259408a8f1 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -812,6 +812,29 @@ sev_read_file_base64(const char *filename, guchar 
> **data, gsize *len)
>      return 0;
>  }
>  
> +static int
> +sev_snp_launch_start(SevGuestState *sev)
> +{
> +    int ret = 1;
> +    int fw_error, rc;
> +    struct kvm_sev_snp_launch_start *start = &sev->snp_config.start;
> +
> +    trace_kvm_sev_snp_launch_start(start->policy);
> +
> +    rc = sev_ioctl(sev->sev_fd, KVM_SEV_SNP_LAUNCH_START, start, &fw_error);
> +    if (rc < 0) {
> +        error_report("%s: SNP_LAUNCH_START ret=%d fw_error=%d '%s'",
> +                __func__, ret, fw_error, fw_error_to_str(fw_error));

Did you mean to report the value of ret or rc?


> +        goto out;

Suggestion:

Remove the `ret` variable.
Here: simply `return 1`.
At the end: remove the `out:` label; simply `return 0`.


> +    }
> +
> +    sev_set_guest_state(sev, SEV_STATE_LAUNCH_UPDATE);
> +    ret = 0;
> +
> +out:
> +    return ret;
> +}
> +
>  static int
>  sev_launch_start(SevGuestState *sev)
>  {
> @@ -1105,7 +1128,12 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error 
> **errp)
>          goto err;
>      }
>  
> -    ret = sev_launch_start(sev);
> +    if (sev_snp_enabled()) {
> +        ret = sev_snp_launch_start(sev);
> +    } else {
> +        ret = sev_launch_start(sev);
> +    }
> +
>      if (ret) {
>          error_setg(errp, "%s: failed to create encryption context", 
> __func__);
>          goto err;
> diff --git a/target/i386/trace-events b/target/i386/trace-events
> index 2cd8726eeb..18cc14b956 100644
> --- a/target/i386/trace-events
> +++ b/target/i386/trace-events
> @@ -11,3 +11,4 @@ kvm_sev_launch_measurement(const char *value) "data %s"
>  kvm_sev_launch_finish(void) ""
>  kvm_sev_launch_secret(uint64_t hpa, uint64_t hva, uint64_t secret, int len) 
> "hpa 0x%" PRIx64 " hva 0x%" PRIx64 " data 0x%" PRIx64 " len %d"
>  kvm_sev_attestation_report(const char *mnonce, const char *data) "mnonce %s 
> data %s"
> +kvm_sev_snp_launch_start(uint64_t policy) "policy 0x%" PRIx64
>