Re: [PATCH v13 09/26] KVM: SEV: Add KVM_SEV_SNP_LAUNCH_START command

[Michael Roth]

On Fri, Apr 19, 2024 at 01:52:24PM +0200, Paolo Bonzini wrote:
> On Thu, Apr 18, 2024 at 9:42 PM Michael Roth <michael.roth@xxxxxxx> wrote:
> > +/* As defined by SEV-SNP Firmware ABI, under "Guest Policy". */
> > +#define SNP_POLICY_MASK_API_MAJOR      GENMASK_ULL(15, 8)
> > +#define SNP_POLICY_MASK_API_MINOR      GENMASK_ULL(7, 0)
> > +
> > +#define SNP_POLICY_MASK_VALID          (SNP_POLICY_MASK_SMT            | \
> > +                                        SNP_POLICY_MASK_RSVD_MBO       | \
> > +                                        SNP_POLICY_MASK_DEBUG          | \
> > +                                        SNP_POLICY_MASK_SINGLE_SOCKET  | \
> > +                                        SNP_POLICY_MASK_API_MAJOR      | \
> > +                                        SNP_POLICY_MASK_API_MINOR)
> > +
> > +/* KVM's SNP support is compatible with 1.51 of the SEV-SNP Firmware ABI. */
> > +#define SNP_POLICY_API_MAJOR           1
> > +#define SNP_POLICY_API_MINOR           51
> 
> > +static inline bool sev_version_greater_or_equal(u8 major, u8 minor)
> > +{
> > +       if (major < SNP_POLICY_API_MAJOR)
> > +               return true;
> 
> Should it perhaps refuse version 0.x? With something like a
> 
> #define SNP_POLICY_API_MAJOR_MIN    1
> 
> to make it a bit more future proof (and testable).
> 
> > +       major = (params.policy & SNP_POLICY_MASK_API_MAJOR);
> 
> This should be >> 8. Do the QEMU patches not set the API version? :)

Argh...it does if you set it via the -object sev-snp-guest,policy=0x...
option. I tested with reserved ranges and other flags, but not with
non-zero major/minor API fields. =/

But I'm having 2nd thoughts about trying to enforce API version via
KVM_SEV_SNP_LAUNCH_START. In practice, the only sensible way to really
interpret it is as "the minimum firmware version that userspace decides
it is comfortable running a particular guest" on. And that enforcement
is already handled as part of the SNP_LAUNCH_START firmware command in
the SNP Firmware ABI: if the policy specifies a higher minimum version
than what firmware is currently running then it will return as error
that will be reported by QEMU as:

  sev_snp_launch_start: SNP_LAUNCH_START ret=-5 fw_error=7 'Policy is not allowed'

On the firmware driver side (drivers/crypto/ccp/sev-dev.c), we already
enforce 1.51 as minimum supported SNP firmware, so that sort of already
covers the SNP_POLICY_API_MAJOR_MIN case as well. E.g. the test surface
KVM needs to concern itself with is already effectively 1.51+. In that
sense, whether the user decides to be less restrictive with what minimum
firmware version they allow is then totally up to the user, and has no
bearing on what firmware versions KVM needs to concern itself with.

Then the question of whether or not KVM fully *exposes* a particular
user-visible feature of a newer version of the firmware/ABI would be a
separate thing to be handled via the normal KVM capabilities/attributes
mechanisms.

So my current leaning is to send a v14 that backs out the major/minor
policy enforcement and let firmware handle that aspect. (and also
address your other comments).

But let me know if you think that should be handled differently.

Thanks!

-Mike

> 
> Paolo
>