On Thu, Sep 03, 2026 at 03:55:31PM +0200, Stefano Garzarella wrote: > On Thu, Sep 03, 2026 at 12:47:00PM +0200, Luigi Leonardi wrote: > > On Thu, Sep 03, 2026 at 12:32:21PM +0200, Stefano Garzarella wrote: > > > On Tue, Sep 01, 2026 at 12:09:18PM +0200, Luigi Leonardi wrote: > > > > The guest policy must be provided at guest launch start: LAUNCH_START > > > > for > > > > SEV/SEV-ES and SNP_LAUNCH_START for SEV-SNP. See the SEV API > > > > specification, chapter 3 (Guest Policy), and the SEV-SNP firmware ABI > > > > specification, section 4.3 (Guest Policy). > > > > > > > > The policy parameter in set_guest_policy was never effective: by the > > > > time this callback runs, LAUNCH_START has already been issued for both > > > > SEV/SEV-ES and SEV-SNP, so writing to kvm_start_conf.policy or > > > > sev_guest->policy has no effect. In practice the only thing this > > > > callback actually does is set the ID block and ID auth for SNP's > > > > LAUNCH_FINISH, so rename it to set_id_block to reflect its real > > > > purpose, remove the unused policy parameter, and drop the non-SNP code > > > > path which was entirely dead. > > > > > > > > This is preliminary work: actually forwarding the guest policy to the > > > > platform before LAUNCH_START is added in a later commit. > > > > > > IIUC the behaviour is the same after this patch, but IMO better to > > > clarify. > > > > Yep, will do. > > > > > > > > > > > > > Signed-off-by: Luigi Leonardi <[email protected]> > > > > --- > > > > backends/confidential-guest-support.c | 12 ++- > > > > backends/igvm.c | 6 +- > > > > include/system/confidential-guest-support.h | 28 ++++--- > > > > target/i386/sev.c | 118 > > > > +++++++++++----------------- > > > > 4 files changed, 67 insertions(+), 97 deletions(-) > > > > > > > > diff --git a/backends/confidential-guest-support.c > > > > b/backends/confidential-guest-support.c > > > > index 156dd15e66..a0b36d2da5 100644 > > > > --- a/backends/confidential-guest-support.c > > > > +++ b/backends/confidential-guest-support.c > > > > @@ -38,14 +38,12 @@ static int set_guest_state(hwaddr gpa, uint8_t > > > > *ptr, uint64_t len, > > > > return -1; > > > > } > > > > > > > > -static int set_guest_policy(ConfidentialGuestPolicyType policy_type, > > > > - uint64_t policy, > > > > - void *policy_data1, uint32_t > > > > policy_data1_size, > > > > - void *policy_data2, uint32_t > > > > policy_data2_size, > > > > - Error **errp) > > > > +static int set_id_block(void *id_block, uint32_t id_block_size, > > > > + void *id_auth, uint32_t id_auth_size, > > > > + Error **errp) > > > > { > > > > error_setg(errp, > > > > - "Setting confidential guest policy is not supported for > > > > this platform"); > > > > + "Setting ID block is not supported for this platform"); > > > > return -1; > > > > } > > > > > > > > @@ -64,7 +62,7 @@ static void > > > > confidential_guest_support_class_init(ObjectClass *oc, > > > > ConfidentialGuestSupportClass *cgsc = > > > > CONFIDENTIAL_GUEST_SUPPORT_CLASS(oc); > > > > cgsc->check_support = check_support; > > > > cgsc->set_guest_state = set_guest_state; > > > > - cgsc->set_guest_policy = set_guest_policy; > > > > + cgsc->set_id_block = set_id_block; > > > > cgsc->get_mem_map_entry = get_mem_map_entry; > > > > } > > > > > > > > diff --git a/backends/igvm.c b/backends/igvm.c > > > > index 7b7bdc72b7..85de0d54ec 100644 > > > > --- a/backends/igvm.c > > > > +++ b/backends/igvm.c > > > > @@ -968,9 +968,9 @@ static int qigvm_handle_policy(QIgvm *ctx, Error > > > > **errp) > > > > 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 ctx->cgsc->set_id_block(ctx->id_block, id_block_len, > > > > + ctx->id_auth, id_auth_len, > > > > errp); > > > > } > > > > return 0; > > > > } > > > > diff --git a/include/system/confidential-guest-support.h > > > > b/include/system/confidential-guest-support.h > > > > index 5dca717308..6d35ddb97a 100644 > > > > --- a/include/system/confidential-guest-support.h > > > > +++ b/include/system/confidential-guest-support.h > > > > @@ -128,21 +128,23 @@ typedef struct ConfidentialGuestSupportClass { > > > > uint16_t cpu_index, Error **errp); > > > > > > > > /* > > > > - * Set the guest policy. The policy can be used to configure the > > > > - * confidential platform, such as if debug is enabled or not and > > > > can contain > > > > - * information about expected launch measurements, signed > > > > verification of > > > > - * guest configuration and other platform data. > > > > - * > > > > - * The format of the policy data is specific to each platform. For > > > > example, > > > > - * SEV-SNP uses a policy bitfield in the 'policy' argument and > > > > provides an > > > > - * ID block and ID authentication in the 'policy_data' parameters. > > > > The type > > > > - * of policy data is identified by the 'policy_type' argument. > > > > + * Set the guest policy for the confidential platform. The policy > > > > + * configures properties of the guest, such as whether debug is > > > > + * enabled. Its format is platform-specific; for SEV/SEV-ES and > > > > + * SEV-SNP it is a policy bitfield. Must be called before > > > > LAUNCH_START > > > > + * so the policy is in effect for launch. > > > > */ > > > > int (*set_guest_policy)(ConfidentialGuestPolicyType policy_type, > > > > > > I'm confused, this commit says "rename set_guest_policy to set_id_block" > > > so why this callback is still here? > > > > > > > We will need this callback in a following patch, so I thought that removing > > it to then add it again was pointless. If you prefer I can go this > > route, or I can specify it in the commit message. > > I see your point about avoiding remove and re-add the callback, but > self-contained commits aren't a matter of preference or taste, they're > a maintainability requirement. A callback declared but unimplemented > across three commits leaves the API in an incomplete state. > > Please move the set_guest_policy declaration where it's first used.
A typical and good way to handle cases like this (a fix needs some code reorganization) is to split that into two patches: First patch does the code reorganization without functional change (in this case: split one callback into two), then have a relatively small patch with the actual bugfix (move one of the two callback calls to another place). take care, Gerd
