Hi Brijesh,
On 10/07/2021 0:55, Brijesh Singh wrote:
> During the SNP guest launch sequence, a special secrets and cpuid page
> needs to be populated by the SEV-SNP firmware. 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 LAUNCH_UPDATE command to validate those address range
s/LAUNCH_UPDATE/SNP_LAUNCH_UPDATE/
(to show it's the same command you refer to above)
> 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>
> ---
> target/i386/sev.c | 184 ++++++++++++++++++++++++++++++++++++++-
> target/i386/trace-events | 2 +
> 2 files changed, 184 insertions(+), 2 deletions(-)
>
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 41dcb084d1..f438e09d33 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -93,6 +93,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;
> + /* CPUID page address */
> + uint32_t cpuid_addr;
> + uint32_t cpuid_len;
> +} SevSnpBootInfoBlock;
> +
> static SevGuestState *sev_guest;
> static Error *sev_mig_blocker;
>
> @@ -1014,6 +1027,158 @@ 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)
hwaddr is a confusing name here because it is also a typedef (which is
most likely uint64_t...). Maybe call this argument `gpa` ?
> +{
> + void *hva;
> + MemoryRegion *mr = NULL;
> +
> + 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_guest, hva, size, type);
> +}
> +
> +struct snp_pre_validated_range {
> + uint32_t start;
> + uint32_t end;
> +};
> +
> +static struct snp_pre_validated_range pre_validated[2];
> +
> +static bool
> +detectoverlap(uint32_t start, uint32_t end,
> + struct snp_pre_validated_range *overlap)
naming conventions dictate: detect_overlap
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(pre_validated); i++) {
> + if (pre_validated[i].start < end && start < pre_validated[i].end) {
> + memcpy(overlap, &pre_validated[i], sizeof(*overlap));
Maybe simpler than memcpy:
*overlap = pre_validated[i];
> + return true;
> + }
> + }
> +
> + return false;
> +}
> +
> +static void snp_ovmf_boot_block_setup(void)
> +{
> + struct snp_pre_validated_range overlap;
> + SevSnpBootInfoBlock *info;
> + uint32_t start, end, sz;
> + int ret;
> +
> + /*
> + * 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)) {
> + 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.
> + */
> + pre_validated[0].start = info->secrets_addr;
> + pre_validated[0].end = info->secrets_addr + info->secrets_len;
> + pre_validated[1].start = info->cpuid_addr;
> + pre_validated[1].end = info->cpuid_addr + info->cpuid_len;
> + start = info->pre_validated_start;
> + end = info->pre_validated_end;
> +
> + while (start < end) {
> + /* Check if the requested range overlaps with Secrets and CPUID page */
> + if (detectoverlap(start, end, &overlap)) {
> + if (start < overlap.start) {
> + sz = overlap.start - start;
> + if (sev_snp_launch_update_gpa(start, sz,
> + KVM_SEV_SNP_PAGE_TYPE_UNMEASURED)) {
> + error_report("SEV-SNP: failed to validate gpa 0x%x sz %d",
> + start, sz);
> + exit(1);
> + }
> + }
> +
> + start = overlap.end;
> + continue;
> + }
> +
> + /* Validate the remaining range */
> + if (sev_snp_launch_update_gpa(start, end - start,
> + KVM_SEV_SNP_PAGE_TYPE_UNMEASURED)) {
> + 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(SevGuestState *sev)
> +{
> + int ret, error;
> + Error *local_err = NULL;
> + struct kvm_sev_snp_launch_finish *finish = &sev->snp_config.finish;
> +
> + trace_kvm_sev_snp_launch_finish();
Maybe the trace should show some info about the snp_config.finish fields?
> + ret = sev_ioctl(sev->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, 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)
> {
> @@ -1048,7 +1213,12 @@ sev_vm_state_change(void *opaque, bool running, RunState state)
>
> if (running) {
> if (!sev_check_state(sev, SEV_STATE_RUNNING)) {
> - sev_launch_finish(sev);
> + if (sev_snp_enabled()) {
> + snp_ovmf_boot_block_setup();
> + sev_snp_launch_finish(sev);
> + } else {
> + sev_launch_finish(sev);
> + }
> }
> }
> }
> @@ -1164,7 +1334,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);
>
> 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"
The last argument is an end-addr (not a length), so maybe the format
string should end with:
".... pre-validate 0x%x - 0x%x"
Also I'd prefer to log the SevSnpBootInfoBlock fields in the order they
appear in the struct.
-Dov
