Re: [PATCH v3 15/15] sev: Provide sev_features flags from IGVM VMSA to KVM_SEV_INIT2

[Roy Hopkins]

On Mon, 2024-06-24 at 15:14 +0100, Daniel P. Berrangé wrote:
> On Fri, Jun 21, 2024 at 03:29:18PM +0100, Roy Hopkins wrote:
> > IGVM files can contain an initial VMSA that should be applied to each
> > vcpu as part of the initial guest state. The sev_features flags are
> > provided as part of the VMSA structure. However, KVM only allows
> > sev_features to be set during initialization and not as the guest is
> > being prepared for launch.
> > 
> > This patch queries KVM for the supported set of sev_features flags and
> > processes the IGVM file during kvm_init to determine any sev_features
> > flags set in the IGVM file. These are then provided in the call to
> > KVM_SEV_INIT2 to ensure the guest state matches that specified in the
> > IGVM file.
> > 
> > This does cause the IGVM file to be processed twice. Firstly to extract
> > the sev_features then secondly to actually configure the guest. However,
> > the first pass is largely ignored meaning the overhead is minimal.
> > 
> > Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
> > ---
> >  target/i386/sev.c | 145 ++++++++++++++++++++++++++++++++++++++++------
> >  1 file changed, 126 insertions(+), 19 deletions(-)
> > 
> 
> >  static int sev_common_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
> >  {
> >      char *devname;
> > @@ -1743,6 +1783,10 @@ static int
> > sev_common_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
> >          }
> >      }
> >  
> > +    if (sev_init_supported_features(sev_common, errp) < 0) {
> > +        return -1;
> > +    }
> > +
> >      trace_kvm_sev_init();
> >      if (x86_klass->kvm_type(X86_CONFIDENTIAL_GUEST(sev_common)) ==
> > KVM_X86_DEFAULT_VM) {
> >          cmd = sev_es_enabled() ? KVM_SEV_ES_INIT : KVM_SEV_INIT;
> > @@ -1750,6 +1794,38 @@ static int
> > sev_common_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
> >          ret = sev_ioctl(sev_common->sev_fd, cmd, NULL, &fw_error);
> >      } else {
> >          struct kvm_sev_init args = { 0 };
> > +        MachineState *machine = MACHINE(qdev_get_machine());
> > +
> > +        /*
> > +         * If configuration is provided via an IGVM file then the IGVM file
> > +         * might contain configuration of the initial vcpu context. For SEV
> > +         * the vcpu context includes the sev_features which should be
> > applied
> > +         * to the vcpu.
> > +         *
> > +         * KVM does not synchronize sev_features from CPU state. Instead it
> > +         * requires sev_features to be provided as part of this
> > initialization
> > +         * call which is subsequently automatically applied to the VMSA of
> > +         * each vcpu.
> > +         *
> > +         * The IGVM file is normally processed after initialization.
> > Therefore
> > +         * we need to pre-process it here to extract sev_features in order
> > to
> > +         * provide it to KVM_SEV_INIT2. Each cgs_* function that is called
> > by
> > +         * the IGVM processor detects this pre-process by observing the
> > state
> > +         * as SEV_STATE_UNINIT.
> > +         */
> > +        if (machine->igvm) {
> > +            if (IGVM_CFG_GET_CLASS(machine->igvm)
> > +                    ->process(machine->igvm, machine->cgs, errp) == -1) {
> > +                return -1;
> > +            }
> > +            /*
> > +             * KVM maintains a bitmask of allowed sev_features. This does
> > not
> > +             * include SVM_SEV_FEAT_SNP_ACTIVE which is set accordingly by
> > KVM
> > +             * itself. Therefore we need to clear this flag.
> > +             */
> > +            args.vmsa_features = sev_common->sev_features &
> > +                                 ~SVM_SEV_FEAT_SNP_ACTIVE;
> > +        }
> >  
> >          ret = sev_ioctl(sev_common->sev_fd, KVM_SEV_INIT2, &args,
> > &fw_error);
> >      }
> 
> What happens if the code path takes us down the KVM_SEV_INIT
> route, rather than KVM_SEV_INIT2 ?  Should we be reporting an
> error indicating that IGVM usage is incompatible with the legacy
> KVM_SEV_INIT path ?
> 
> 
> With regards,
> Daniel

The idea is that sev_common->supported_sev_features is initialised to 0 for the
legacy path meaning that the call to check_sev_features() that occurs when the
IGVM file is parsed will flag an error stating 'VMSA contains unsupported
sev_features' if any features are set. This should ensure the IGVM file matches
the settings in the legacy kernel.

However, I've just run this against an older kernel and found an error: the
kvm_ioctl in sev_init_supported_features() is only supported in newer kernels
meaning that the launch is aborted on older kernels. I'll update this patch to
fix this and ensure supported_sev_features is 0 for the KVM_SEV_INIT case.

Regards,
Roy