Re: [RFC PATCH 5/6] i386/sev: add support to encrypt BIOS when SEV-SNP is enabled

[Dov Murik <dovmurik@linux.ibm.com>]

On 10/07/2021 0:55, Brijesh Singh wrote:
> The KVM_SEV_SNP_LAUNCH_UPDATE command is used for encrypting the bios
> image used for booting the SEV-SNP guest.
> 
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  target/i386/sev.c        | 33 ++++++++++++++++++++++++++++++++-
>  target/i386/trace-events |  1 +
>  2 files changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 259408a8f1..41dcb084d1 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -883,6 +883,30 @@ out:
>      return ret;
>  }
>  
> +static int
> +sev_snp_launch_update(SevGuestState *sev, uint8_t *addr, uint64_t len, int type)

This seems similar to the SEV LAUNCH_UPDATE_DATA API (with the added
`type` argument).  In SEV API these are the limitations (from the SEV
API document):

* PADDR - System physical address of the data to be encrypted.
          Must be 16 B aligned.
* LENGTH - Length of the data to be encrypted.
           Must be a multiple of 16 B.

But in SNP_LAUNCH_UPDATE it is my understanding that addr must be page
aligned (4KB) and length must be in whole pages (because the underlying
types are PAGE_TYPE_NORMAL, PAGE_TYPE_ZERO, ...).

So what happens if we call sev_encrypt_flash with a non-page-aligned
addr / length?

Or maybe I misunderstood the SNP ABI document?

-Dov



> +{
> +    int ret, fw_error;
> +    struct kvm_sev_snp_launch_update update = {};
> +
> +    if (!addr || !len) {
> +        return 1;
> +    }
> +
> +    update.uaddr = (__u64)(unsigned long)addr;
> +    update.len = len;
> +    update.page_type = type;
> +    trace_kvm_sev_snp_launch_update(addr, len, type);
> +    ret = sev_ioctl(sev->sev_fd, KVM_SEV_SNP_LAUNCH_UPDATE,
> +                    &update, &fw_error);
> +    if (ret) {
> +        error_report("%s: SNP_LAUNCH_UPDATE ret=%d fw_error=%d '%s'",
> +                __func__, ret, fw_error, fw_error_to_str(fw_error));
> +    }
> +
> +    return ret;
> +}
> +
>  static int
>  sev_launch_update_data(SevGuestState *sev, uint8_t *addr, uint64_t len)
>  {
> @@ -1161,7 +1185,14 @@ sev_encrypt_flash(uint8_t *ptr, uint64_t len, Error **errp)
>  
>      /* if SEV is in update state then encrypt the data else do nothing */
>      if (sev_check_state(sev_guest, SEV_STATE_LAUNCH_UPDATE)) {
> -        int ret = sev_launch_update_data(sev_guest, ptr, len);
> +        int ret;
> +
> +        if (sev_snp_enabled()) {
> +            ret = sev_snp_launch_update(sev_guest, ptr, len,
> +                                        KVM_SEV_SNP_PAGE_TYPE_NORMAL);
> +        } else {
> +            ret = sev_launch_update_data(sev_guest, ptr, len);
> +        }
>          if (ret < 0) {
>              error_setg(errp, "failed to encrypt pflash rom");
>              return ret;
> diff --git a/target/i386/trace-events b/target/i386/trace-events
> index 18cc14b956..0c2d250206 100644
> --- a/target/i386/trace-events
> +++ b/target/i386/trace-events
> @@ -12,3 +12,4 @@ 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
> +kvm_sev_snp_launch_update(void *addr, uint64_t len, int type) "addr %p len 0x%" PRIx64 " type %d"
>