Re: [RFC PATCH v2 07/12] i386/sev: populate secrets and cpuid page and finalize the SNP launch
On Fri, Sep 03, 2021 at 11:24:20PM +0300, Dov Murik wrote:
> Hi Michael,
Hi Dov,
Thanks for reviewing!
>
> On 27/08/2021 1:26, Michael Roth wrote:
> > From: Brijesh Singh
> >
> > 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).
Will do.
>
> > 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
> > Signed-off-by: Michael Roth
> > ---
> > 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.
I believe you can specify a contiguous range of pages for cpuid_len (in
which case you'll get multiple self-contained CPUID tables), but I'm not
sure about the secrets page. I've tried to avoid the need for multiple
CPUID tables/pages in the initial implementation by allowing 0 entries
to be left out, like how KVM_SET_CPUID2 does it; guest kernels will then
check the CPUID ranges to determine if a 0 entry is a valid all-0 entry
vs. an invalid one.
>
>
> > +/* 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(, 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(, start, end - start + 1);
> > +
> > +for (i = 0; i < range_count; i++) {
> > +if (range_overlaps_range(, _list[i]) &&
> > +(range_is_empty(overlap_range) ||
> > + range_lob(_list[i]) < range_lob(overlap_range))) {
> > +*overlap_range = range_list[i];
> > +overlap = true;
> > +}
> > +}
> > +
> > +return overlap;
> > +}
> > +
> > +static void snp_ovmf_boot_block_setup(void)
> > +{
> > +SevSnpBootInfoBlock
Re: [RFC PATCH v2 07/12] i386/sev: populate secrets and cpuid page and finalize the SNP launch
Hi Michael,
On 27/08/2021 1:26, Michael Roth wrote:
> From: Brijesh Singh
>
> 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
> Signed-off-by: Michael Roth
> ---
> 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(, 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(, start, end - start + 1);
> +
> +for (i = 0; i < range_count; i++) {
> +if (range_overlaps_range(, _list[i]) &&
> +(range_is_empty(overlap_range) ||
> + range_lob(_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 **), 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,
> +
