Hi Michael,
On 27/08/2021 1:26, Michael Roth wrote:
> From: Brijesh Singh <brijesh.singh@xxxxxxx>
>
> During the SNP guest launch sequence, a special secrets and cpuid page
> needs to be populated by the SEV-SNP firmware.
Just to be clear: these are two distinct pages. I suggest rephrasing to
"... a special secrets page and a special cpuid page need to be
populated ..." (or something along these lines).
> The secrets page contains
> the VM Platform Communication Key (VMPCKs) used by the guest to send and
> receive secure messages to the PSP. And CPUID page will contain the CPUID
> value filtered through the PSP.
>
> The guest BIOS (OVMF) reserves these pages in MEMFD and location of it
> is available through the SNP boot block GUID. While finalizing the guest
> boot flow, lookup for the boot block and call the SNP_LAUNCH_UPDATE
> command to populate secrets and cpuid pages.
>
> In order to support early boot code, the OVMF may ask hypervisor to
> request the pre-validation of certain memory range. If such range is
> present the call SNP_LAUNCH_UPDATE command to validate those address
> range without affecting the measurement. See the SEV-SNP specification
> for further details.
>
> Finally, call the SNP_LAUNCH_FINISH to finalize the guest boot.
>
> Signed-off-by: Brijesh Singh <brijesh.singh@xxxxxxx>
> Signed-off-by: Michael Roth <michael.roth@xxxxxxx>
> ---
> target/i386/sev.c | 189 ++++++++++++++++++++++++++++++++++++++-
> target/i386/trace-events | 2 +
> 2 files changed, 189 insertions(+), 2 deletions(-)
>
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 867c0cb457..0009c93d28 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -33,6 +33,7 @@
> #include "monitor/monitor.h"
> #include "exec/confidential-guest-support.h"
> #include "hw/i386/pc.h"
> +#include "qemu/range.h"
>
> #define TYPE_SEV_COMMON "sev-common"
> OBJECT_DECLARE_SIMPLE_TYPE(SevCommonState, SEV_COMMON)
> @@ -107,6 +108,19 @@ typedef struct __attribute__((__packed__)) SevInfoBlock {
> uint32_t reset_addr;
> } SevInfoBlock;
>
> +#define SEV_SNP_BOOT_BLOCK_GUID "bd39c0c2-2f8e-4243-83e8-1b74cebcb7d9"
> +typedef struct __attribute__((__packed__)) SevSnpBootInfoBlock {
> + /* Prevalidate range address */
> + uint32_t pre_validated_start;
> + uint32_t pre_validated_end;
> + /* Secrets page address */
> + uint32_t secrets_addr;
> + uint32_t secrets_len;
Just curious: isn't secrets_len always 4096? (same for cpuid_len)
Though it might be a good future proofing to have a length field.
> + /* CPUID page address */
> + uint32_t cpuid_addr;
> + uint32_t cpuid_len;
> +} SevSnpBootInfoBlock;
> +
> static Error *sev_mig_blocker;
>
> static const char *const sev_fw_errlist[] = {
> @@ -1086,6 +1100,162 @@ static Notifier sev_machine_done_notify = {
> .notify = sev_launch_get_measure,
> };
>
> +static int
> +sev_snp_launch_update_gpa(uint32_t hwaddr, uint32_t size, uint8_t type)
> +{
> + void *hva;
> + MemoryRegion *mr = NULL;
> + SevSnpGuestState *sev_snp_guest =
> + SEV_SNP_GUEST(MACHINE(qdev_get_machine())->cgs);
> +
> + hva = gpa2hva(&mr, hwaddr, size, NULL);
> + if (!hva) {
> + error_report("SEV-SNP failed to get HVA for GPA 0x%x", hwaddr);
> + return 1;
> + }
> +
> + return sev_snp_launch_update(sev_snp_guest, hwaddr, hva, size, type);
> +}
> +
> +static bool
> +detect_first_overlap(uint64_t start, uint64_t end, Range *range_list,
> + size_t range_count, Range *overlap_range)
> +{
> + int i;
> + bool overlap = false;
> + Range new;
> +
> + assert(overlap_range);
Also:
assert(end >= start)
assert(range_count == 0 || range_list)
> + range_make_empty(overlap_range);
> + range_init_nofail(&new, start, end - start + 1);
> +
> + for (i = 0; i < range_count; i++) {
> + if (range_overlaps_range(&new, &range_list[i]) &&
> + (range_is_empty(overlap_range) ||
> + range_lob(&range_list[i]) < range_lob(overlap_range))) {
> + *overlap_range = range_list[i];
> + overlap = true;
> + }
> + }
> +
> + return overlap;
> +}
> +
> +static void snp_ovmf_boot_block_setup(void)
> +{
> + SevSnpBootInfoBlock *info;
> + uint32_t start, end, sz;
> + int ret;
> + Range validated_ranges[2];
> +
> + /*
> + * Extract the SNP boot block for the SEV-SNP guests by locating the
> + * SNP_BOOT GUID. The boot block contains the information such as location
> + * of secrets and CPUID page, additionaly it may contain the range of
> + * memory that need to be pre-validated for the boot.
> + */
> + if (!pc_system_ovmf_table_find(SEV_SNP_BOOT_BLOCK_GUID,
> + (uint8_t **)&info, NULL)) {
Fix indent of arguments.
> + error_report("SEV-SNP: failed to find the SNP boot block");
> + exit(1);
> + }
> +
> + trace_kvm_sev_snp_ovmf_boot_block_info(info->secrets_addr,
> + info->secrets_len, info->cpuid_addr,
> + info->cpuid_len,
> + info->pre_validated_start,
> + info->pre_validated_end);
> +
> + /* Populate the secrets page */
> + ret = sev_snp_launch_update_gpa(info->secrets_addr, info->secrets_len,
> + KVM_SEV_SNP_PAGE_TYPE_SECRETS);
> + if (ret) {
> + error_report("SEV-SNP: failed to insert secret page GPA 0x%x",
> + info->secrets_addr);
> + exit(1);
> + }
> +
> + /* Populate the cpuid page */
> + ret = sev_snp_launch_update_gpa(info->cpuid_addr, info->cpuid_len,
> + KVM_SEV_SNP_PAGE_TYPE_CPUID);
> + if (ret) {
> + error_report("SEV-SNP: failed to insert cpuid page GPA 0x%x",
> + info->cpuid_addr);
> + exit(1);
> + }
> +
> + /*
> + * Pre-validate the range using the LAUNCH_UPDATE_DATA, if the
> + * pre-validation range contains the CPUID and Secret page GPA then skip
> + * it. This is because SEV-SNP firmware pre-validates those pages as part
> + * of adding secrets and cpuid LAUNCH_UPDATE type.
> + */
> + range_init_nofail(&validated_ranges[0], info->secrets_addr, info->secrets_len);
> + range_init_nofail(&validated_ranges[1], info->cpuid_addr, info->cpuid_len);
> + start = info->pre_validated_start;
> + end = info->pre_validated_end;
> +
> + while (start < end) {
> + Range overlap_range;
> +
> + /* Check if the requested range overlaps with Secrets and CPUID page */
> + if (detect_first_overlap(start, end, validated_ranges, 2,
Replace the literal 2 with ARRAY_SIZE(validated_ranges).
> + &overlap_range)) {
> + if (start < range_lob(&overlap_range)) {
> + sz = range_lob(&overlap_range) - start;
> + if (sev_snp_launch_update_gpa(start, sz,
> + KVM_SEV_SNP_PAGE_TYPE_UNMEASURED)) {
Fix indent of arguments (if possible).
> + error_report("SEV-SNP: failed to validate gpa 0x%x sz %d",
> + start, sz);
> + exit(1);
> + }
> + }
> +
> + start = range_upb(&overlap_range) + 1;
> + continue;
> + }
> +
> + /* Validate the remaining range */
> + if (sev_snp_launch_update_gpa(start, end - start,
I think the second argument should be: end - start + 1 .
Consider start=0x8000 end=0x8fff (inclusive). In this case you want to
validate exactly 0x1000 bytes starting at 0x8000. So the size should be
0x8fff - 0x8000 + 1.
I assume all this doesn't matter for the underlying calls which operate
at whole pages anyway (are there proper asserts in sev_snp_launch_update
(or in KVM) that verify that start and sz are page-size-aligned?)
> + KVM_SEV_SNP_PAGE_TYPE_UNMEASURED)) {
Fix indent of arguments.
> + error_report("SEV-SNP: failed to validate gpa 0x%x sz %d",
> + start, end - start);
> + exit(1);
> + }
> +
> + start = end;
> + }
> +}
> +
> +static void
> +sev_snp_launch_finish(SevSnpGuestState *sev_snp)
> +{
> + int ret, error;
> + Error *local_err = NULL;
> + struct kvm_sev_snp_launch_finish *finish = &sev_snp->kvm_finish_conf;
> +
> + trace_kvm_sev_snp_launch_finish();
> + ret = sev_ioctl(SEV_COMMON(sev_snp)->sev_fd, KVM_SEV_SNP_LAUNCH_FINISH, finish, &error);
> + if (ret) {
> + error_report("%s: SNP_LAUNCH_FINISH ret=%d fw_error=%d '%s'",
> + __func__, ret, error, fw_error_to_str(error));
> + exit(1);
> + }
> +
> + sev_set_guest_state(SEV_COMMON(sev_snp), SEV_STATE_RUNNING);
> +
> + /* add migration blocker */
> + error_setg(&sev_mig_blocker,
> + "SEV: Migration is not implemented");
> + ret = migrate_add_blocker(sev_mig_blocker, &local_err);
> + if (local_err) {
> + error_report_err(local_err);
> + error_free(sev_mig_blocker);
> + exit(1);
> + }
> +}
> +
> +
> static void
> sev_launch_finish(SevGuestState *sev_guest)
> {
> @@ -1121,7 +1291,12 @@ sev_vm_state_change(void *opaque, bool running, RunState state)
>
> if (running) {
> if (!sev_check_state(sev_common, SEV_STATE_RUNNING)) {
> - sev_launch_finish(SEV_GUEST(sev_common));
> + if (sev_snp_enabled()) {
> + snp_ovmf_boot_block_setup();
> + sev_snp_launch_finish(SEV_SNP_GUEST(sev_common));
> + } else {
> + sev_launch_finish(SEV_GUEST(sev_common));
> + }
> }
> }
> }
> @@ -1236,7 +1411,17 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
> }
>
> ram_block_notifier_add(&sev_ram_notifier);
> - qemu_add_machine_init_done_notifier(&sev_machine_done_notify);
> +
> + /*
> + * The machine done notify event is used by the SEV guest to get the
> + * measurement of the encrypted images. When SEV-SNP is enabled then
> + * measurement is part of the attestation report and the measurement
> + * command does not exist. So skip registering the notifier.
> + */
> + if (!sev_snp_enabled()) {
> + qemu_add_machine_init_done_notifier(&sev_machine_done_notify);
> + }
> +
> qemu_add_vm_change_state_handler(sev_vm_state_change, sev_common);
>
> cgs->ready = true;
> diff --git a/target/i386/trace-events b/target/i386/trace-events
> index 0c2d250206..db91287439 100644
> --- a/target/i386/trace-events
> +++ b/target/i386/trace-events
> @@ -13,3 +13,5 @@ kvm_sev_launch_secret(uint64_t hpa, uint64_t hva, uint64_t secret, int len) "hpa
> 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"
> +kvm_sev_snp_launch_finish(void) ""
> +kvm_sev_snp_ovmf_boot_block_info(uint32_t secrets_gpa, uint32_t slen, uint32_t cpuid_gpa, uint32_t clen, uint32_t s, uint32_t e) "secrets 0x%x+0x%x cpuid 0x%x+0x%x pre-validate 0x%x+0x%x"
In this trace format string you use the notation A+B to indicate addr=A
len=B. But for the pre-validated range the arguments are 'start' and
'end' (and not 'addr' and 'len'), so I suggest choosing a different
notation to log that range.
-Dov
