> On 13/06/2025 16:22, Roy Hopkins wrote:
> > Adds a handler for the guest policy initialization IGVM section and
> > builds an SEV policy based on this information and the ID block
> > directive if present. The policy is applied using by calling
> > 'set_guest_policy()' on the ConfidentialGuestSupport object.
> >
> > Signed-off-by: Roy Hopkins <roy.hopk...@randomman.co.uk>
> > Acked-by: Michael S. Tsirkin <m...@redhat.com>
> > Acked-by: Stefano Garzarella <sgarz...@redhat.com>
> > Acked-by: Gerd Hoffman <kra...@redhat.com>
> > ---
> > backends/igvm.c | 138 ++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 138 insertions(+)
> >
> > diff --git a/backends/igvm.c b/backends/igvm.c
> > index ebdb4594d1..034f504716 100644
> > --- a/backends/igvm.c
> > +++ b/backends/igvm.c
> > @@ -27,6 +27,33 @@ typedef struct QIgvmParameterData {
> > uint32_t index;
> > } QIgvmParameterData;
> >
> > +/*
> > + * Some directives are specific to particular confidential computing
> > platforms.
> > + * Define required types for each of those platforms here.
> > + */
> > +
> > +/* SEV/SEV-ES/SEV-SNP */
>
>
> Could mention that the following are in Chapter 8 of
> "SEV Secure Nested Paging Firmware ABI Specification"
>
Good suggestion. I'll add it.
>
> > +struct QEMU_PACKED sev_id_block {
> > + uint8_t ld[48];
> > + uint8_t family_id[16];
> > + uint8_t image_id[16];
> > + uint32_t version;
> > + uint32_t guest_svn;
> > + uint64_t policy;
> > +};
> > +
> > +struct QEMU_PACKED sev_id_authentication {
> > + uint32_t id_key_alg;
> > + uint32_t auth_key_algo;
> > + uint8_t reserved[56];
> > + uint8_t id_block_sig[512];
> > + uint8_t id_key[1028];
> > + uint8_t reserved2[60];
> > + uint8_t id_key_sig[512];
> > + uint8_t author_key[1028];
> > + uint8_t reserved3[892];
> > +};
> > +
>
> In the next patch (#15), sev_snp_id_authentication is in target/i386/sev.h
> Should they be all together? (and sev_snp_id_authentication seems
> identical to sev_id_authentication - do we we need both structs?)
>
That does make sense and I hoped to do that, but the two files should not really
depend on each other in their current locations. Perhaps we can look at moving
the SEV definitions into a common location in a future patch?
> > /*
> > * QIgvm contains the information required during processing
> > * of a single IGVM file.
> > @@ -38,6 +65,17 @@ typedef struct QIgvm {
> > uint32_t compatibility_mask;
> > unsigned current_header_index;
> > QTAILQ_HEAD(, QIgvmParameterData) parameter_data;
> > + IgvmPlatformType platform_type;
> > +
> > + /*
> > + * SEV-SNP platforms can contain an ID block and authentication
> > + * that should be verified by the guest.
> > + */
> > + struct sev_id_block *id_block;
> > + struct sev_id_authentication *id_auth;
> > +
> > + /* Define the guest policy for SEV guests */
> > + uint64_t sev_policy;
> >
> > /* These variables keep track of contiguous page regions */
> > IGVM_VHS_PAGE_DATA region_prev_page_data;
> > @@ -67,6 +105,11 @@ static int qigvm_directive_environment_info(QIgvm *ctx,
> > static int qigvm_directive_required_memory(QIgvm *ctx,
> > const uint8_t *header_data,
> > Error **errp);
> > +static int qigvm_directive_snp_id_block(QIgvm *ctx, const uint8_t
> > *header_data,
> > + Error **errp);
> > +static int qigvm_initialization_guest_policy(QIgvm *ctx,
> > + const uint8_t *header_data,
> > + Error **errp);
> >
> > struct QIGVMHandler {
> > uint32_t type;
> > @@ -91,6 +134,10 @@ static struct QIGVMHandler handlers[] = {
> > qigvm_directive_environment_info },
> > { IGVM_VHT_REQUIRED_MEMORY, IGVM_HEADER_SECTION_DIRECTIVE,
> > qigvm_directive_required_memory },
> > + { IGVM_VHT_SNP_ID_BLOCK, IGVM_HEADER_SECTION_DIRECTIVE,
> > + qigvm_directive_snp_id_block },
> > + { IGVM_VHT_GUEST_POLICY, IGVM_HEADER_SECTION_INITIALIZATION,
> > + qigvm_initialization_guest_policy },
> > };
> >
> > static int qigvm_handler(QIgvm *ctx, uint32_t type, Error **errp)
> > @@ -632,6 +679,70 @@ static int qigvm_directive_required_memory(QIgvm *ctx,
> > return 0;
> > }
> >
> > +static int qigvm_directive_snp_id_block(QIgvm *ctx, const uint8_t
> > *header_data,
> > + Error **errp)
> > +{
> > + const IGVM_VHS_SNP_ID_BLOCK *igvm_id =
> > + (const IGVM_VHS_SNP_ID_BLOCK *)header_data;
> > +
> > + if (!(igvm_id->compatibility_mask & ctx->compatibility_mask)) {
> > + return 0;
> > + }
> > +
> > + if (ctx->id_block) {
> > + error_setg(errp, "IGVM: Multiple ID blocks encountered "
> > + "in IGVM file.");
>
> Error string should be on one line to make it easier to search/grep for.
This, and the other formatting comments are this way due to the formatting
requirements in the project. checkpatch.pl fails with an error if I try
to stretch these lines out.
>
> > + return -1;
> > + }
> > + ctx->id_block = g_new0(struct sev_id_block, 1);
> > + ctx->id_auth = g_new0(struct sev_id_authentication, 1);
> > +
> > + memcpy(ctx->id_block->family_id, igvm_id->family_id,
> > + sizeof(ctx->id_block->family_id));
> > + memcpy(ctx->id_block->image_id, igvm_id->image_id,
> > + sizeof(ctx->id_block->image_id));
> > + ctx->id_block->guest_svn = igvm_id->guest_svn;
> > + ctx->id_block->version = 1;
>
> Worth a #define for version?
>
Yes. Done.
> > + memcpy(ctx->id_block->ld, igvm_id->ld, sizeof(ctx->id_block->ld));
> > +
> > + ctx->id_auth->id_key_alg = igvm_id->id_key_algorithm;
> > + memcpy(ctx->id_auth->id_block_sig, &igvm_id->id_key_signature,
> > + sizeof(igvm_id->id_key_signature));
> > +
>
> Should the sizeof be ctx->id_auth->id_block_sig (the dest) ?
>
> > + ctx->id_auth->auth_key_algo = igvm_id->author_key_algorithm;
> > + memcpy(ctx->id_auth->id_key_sig, &igvm_id->author_key_signature,
> > + sizeof(igvm_id->author_key_signature));
> > +
>
> Should the sizeof be ctx->id_auth->id_key_sig ?
>
Unfortunately the two structures are different sizes. I've added an assert
for this and the other one below to catch any future errors.
> > + /*
> > + * SEV and IGVM public key structure population are slightly different.
> > + * See SEV Secure Nested Paging Firmware ABI Specification, Chapter 10.
> > + */
> > + *((uint32_t *)ctx->id_auth->id_key) = igvm_id->id_public_key.curve;
> > + memcpy(&ctx->id_auth->id_key[4], &igvm_id->id_public_key.qx, 72);
> > + memcpy(&ctx->id_auth->id_key[76], &igvm_id->id_public_key.qy, 72);
> > +
> > + *((uint32_t *)ctx->id_auth->author_key) =
> > + igvm_id->author_public_key.curve;
> > + memcpy(&ctx->id_auth->author_key[4], &igvm_id->author_public_key.qx,
> > + 72);
> > + memcpy(&ctx->id_auth->author_key[76], &igvm_id->author_public_key.qy,
> > + 72);
> > +
>
> For both memcpy calls, they could fit on one line and be more readable.
>
> > + return 0;
> > +}
> > +
> > +static int qigvm_initialization_guest_policy(QIgvm *ctx,
> > + const uint8_t *header_data, Error **errp)
> > +{
> > + const IGVM_VHS_GUEST_POLICY *guest =
> > + (const IGVM_VHS_GUEST_POLICY *)header_data;
> > +
> > + if (guest->compatibility_mask & ctx->compatibility_mask) {
> > + ctx->sev_policy = guest->policy;
> > + }
> > + return 0;
> > +}
> > +
> > static int qigvm_supported_platform_compat_mask(QIgvm *ctx, Error **errp)
> > {
> > int32_t header_count;
> > @@ -701,12 +812,16 @@ static int qigvm_supported_platform_compat_mask(QIgvm
> > *ctx, Error **errp)
> > /* Choose the strongest supported isolation technology */
> > if (compatibility_mask_sev_snp != 0) {
> > ctx->compatibility_mask = compatibility_mask_sev_snp;
> > + ctx->platform_type = IGVM_PLATFORM_TYPE_SEV_SNP;
> > } else if (compatibility_mask_sev_es != 0) {
> > ctx->compatibility_mask = compatibility_mask_sev_es;
> > + ctx->platform_type = IGVM_PLATFORM_TYPE_SEV_ES;
> > } else if (compatibility_mask_sev != 0) {
> > ctx->compatibility_mask = compatibility_mask_sev;
> > + ctx->platform_type = IGVM_PLATFORM_TYPE_SEV;
> > } else if (compatibility_mask != 0) {
> > ctx->compatibility_mask = compatibility_mask;
> > + ctx->platform_type = IGVM_PLATFORM_TYPE_NATIVE;
> > } else {
> > error_setg(
> > errp,
> > @@ -716,6 +831,23 @@ static int qigvm_supported_platform_compat_mask(QIgvm
> > *ctx, Error **errp)
> > return 0;
> > }
> >
> > +static int qigvm_handle_policy(QIgvm *ctx, Error **errp)
> > +{
> > + if (ctx->platform_type == IGVM_PLATFORM_TYPE_SEV_SNP) {
> > + int id_block_len = 0;
> > + int id_auth_len = 0;
> > + if (ctx->id_block) {
> > + ctx->id_block->policy = ctx->sev_policy;
> > + id_block_len = sizeof(struct sev_id_block);
> > + id_auth_len = sizeof(struct sev_id_authentication);
> > + }
> > + return ctx->cgsc->set_guest_policy(GUEST_POLICY_SEV, ctx->sev_policy,
> > + ctx->id_block, id_block_len,
> > + ctx->id_auth, id_auth_len, errp);
> > + }
> > + return 0;
> > +}
> > +
> > static IgvmHandle qigvm_file_init(char *filename, Error **errp)
> > {
> > IgvmHandle igvm;
> > @@ -814,12 +946,18 @@ int qigvm_process_file(IgvmCfg *cfg,
> > ConfidentialGuestSupport *cgs,
> > */
> > retval = qigvm_process_mem_page(&ctx, NULL, errp);
> >
> > + if (retval == 0) {
> > + retval = qigvm_handle_policy(&ctx, errp);
> > + }
> > +
> > cleanup_parameters:
> > QTAILQ_FOREACH(parameter, &ctx.parameter_data, next)
> > {
> > g_free(parameter->data);
> > parameter->data = NULL;
> > }
> > + g_free(ctx.id_block);
> > + g_free(ctx.id_auth);
> >
> > cleanup:
> > igvm_free(ctx.file);
