| Message ID | 20240209183743.22030-10-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: SEV: allow customizing VMSA features | expand |
On Fri, Feb 09, 2024 at 01:37:41PM -0500, Paolo Bonzini wrote: > The idea that no parameter would ever be necessary when enabling SEV or > SEV-ES for a VM was decidedly optimistic. In fact, in some sense it's > already a parameter whether SEV or SEV-ES is desired. Another possible > source of variability is the desired set of VMSA features, as that affects > the measurement of the VM's initial state and cannot be changed > arbitrarily by the hypervisor. > > Create a new sub-operation for KVM_MEM_ENCRYPT_OP that can take a struct, > and put the new op to work by including the VMSA features as a field of the > struct. The existing KVM_SEV_INIT and KVM_SEV_ES_INIT use the full set of > supported VMSA features for backwards compatibility. > > The struct also includes the usual bells and whistles for future > extensibility: a flags field that must be zero for now, and some padding > at the end. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > .../virt/kvm/x86/amd-memory-encryption.rst | 41 ++++++++++++++-- > arch/x86/include/uapi/asm/kvm.h | 10 ++++ > arch/x86/kvm/svm/sev.c | 48 +++++++++++++++++-- > 3 files changed, 92 insertions(+), 7 deletions(-) > > diff --git a/Documentation/virt/kvm/x86/amd-memory-encryption.rst b/Documentation/virt/kvm/x86/amd-memory-encryption.rst > index 5ed11bc16b96..a4291e7bd8ed 100644 > --- a/Documentation/virt/kvm/x86/amd-memory-encryption.rst > +++ b/Documentation/virt/kvm/x86/amd-memory-encryption.rst > @@ -75,15 +75,50 @@ are defined in ``<linux/psp-dev.h>``. > KVM implements the following commands to support common lifecycle events of SEV > guests, such as launching, running, snapshotting, migrating and decommissioning. > > -1. KVM_SEV_INIT > ---------------- > +1. KVM_SEV_INIT2 > +---------------- > > -The KVM_SEV_INIT command is used by the hypervisor to initialize the SEV platform > +The KVM_SEV_INIT2 command is used by the hypervisor to initialize the SEV platform > context. In a typical workflow, this command should be the first command issued. > > +For this command to be accepted, either KVM_X86_SEV_VM or KVM_X86_SEV_ES_VM > +must have been passed to the KVM_CREATE_VM ioctl. A virtual machine created > +with those machine types in turn cannot be run until KVM_SEV_INIT2 is invoked. > + > +Parameters: struct kvm_sev_init (in) > > Returns: 0 on success, -negative on error > > +:: > + > + struct struct kvm_sev_init { Missing the vm_type param here. > + __u32 flags; /* must be 0 */ > + __u64 vmsa_features; /* initial value of features field in VMSA */ > + __u32 pad[8]; > + }; > + > +It is an error if the hypervisor does not support any of the bits that > +are set in ``flags`` or ``vmsa_features``. > + > +This command replaces the deprecated KVM_SEV_INIT and KVM_SEV_ES_INIT commands. > +The commands did not have any parameters (the ```data``` field was unused) and > +only work for the KVM_X86_DEFAULT_VM machine type (0). > + > +They behave as if: > + > +* the VM type is KVM_X86_SEV_VM for KVM_SEV_INIT, or KVM_X86_SEV_ES_VM for > + KVM_SEV_ES_INIT > + > +* the ``flags`` field of ``struct kvm_sev_init`` is set to zero > + > +* the ``vmsa_features`` field of ``struct kvm_sev_init`` is set to all features > + supported by the hypervisor (as returned by ``KVM_GET_DEVICE_ATTR`` when > + passed group 0 and attribute id ``KVM_X86_SEV_VMSA_FEATURES``). > + > +If the ``KVM_X86_SEV_VMSA_FEATURES`` attribute does not exist, the hypervisor only > +supports KVM_SEV_INIT and KVM_SEV_ES_INIT. In that case the set of VMSA features is > +undefined. It's hard to imagine userspace implementation support for querying KVM_X86_SEV_VMSA_FEATURES but still insisting on KVM_SEV_INIT. Maybe it would be better to just lock in that VMSA_FEATURES at what is currently supported: DEBUG_SWAP=on/off depending on the kvm_amd module param, and then for all other features require opt-in via KVM_SEV_INIT2, and then bake that into the documentation. That way way they could still reference this documentation to properly calculate measurements for older/existing VMM implementations. -Mike > + > 2. KVM_SEV_LAUNCH_START > ----------------------- > > diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h > index 7c46e96cfe62..6baf18335c7b 100644 > --- a/arch/x86/include/uapi/asm/kvm.h > +++ b/arch/x86/include/uapi/asm/kvm.h > @@ -683,6 +683,9 @@ enum sev_cmd_id { > /* Guest Migration Extension */ > KVM_SEV_SEND_CANCEL, > > + /* Second time is the charm; improved versions of the above ioctls. */ > + KVM_SEV_INIT2, > + > KVM_SEV_NR_MAX, > }; > > @@ -694,6 +697,13 @@ struct kvm_sev_cmd { > __u32 sev_fd; > }; > > +struct kvm_sev_init { > + __u32 vm_type; > + __u32 flags; > + __u64 vmsa_features; > + __u32 pad[8]; > +}; > + > struct kvm_sev_launch_start { > __u32 handle; > __u32 policy; > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c > index acf5c45ef14e..78c52764453f 100644 > --- a/arch/x86/kvm/svm/sev.c > +++ b/arch/x86/kvm/svm/sev.c > @@ -252,7 +252,9 @@ static void sev_unbind_asid(struct kvm *kvm, unsigned int handle) > sev_decommission(handle); > } > > -static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp) > +static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp, > + struct kvm_sev_init *data, > + unsigned long vm_type) > { > struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info; > int asid, ret; > @@ -260,7 +262,10 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp) > if (kvm->created_vcpus) > return -EINVAL; > > - if (kvm->arch.vm_type != KVM_X86_DEFAULT_VM) > + if (data->flags) > + return -EINVAL; > + > + if (data->vmsa_features & ~sev_supported_vmsa_features) > return -EINVAL; > > ret = -EBUSY; > @@ -268,8 +273,8 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp) > return ret; > > sev->active = true; > - sev->es_active = argp->id == KVM_SEV_ES_INIT; > - sev->vmsa_features = sev_supported_vmsa_features; > + sev->es_active = (vm_type & __KVM_X86_PROTECTED_STATE_TYPE) != 0; > + sev->vmsa_features = data->vmsa_features; > > asid = sev_asid_new(sev); > if (asid < 0) > @@ -298,6 +303,38 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp) > return ret; > } > > +static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp) > +{ > + struct kvm_sev_init data = { > + .vmsa_features = sev_supported_vmsa_features, > + }; > + unsigned long vm_type; > + > + if (kvm->arch.vm_type != KVM_X86_DEFAULT_VM) > + return -EINVAL; > + > + vm_type = (argp->id == KVM_SEV_INIT ? KVM_X86_SEV_VM : KVM_X86_SEV_ES_VM); > + return __sev_guest_init(kvm, argp, &data, vm_type); > +} > + > +static int sev_guest_init2(struct kvm *kvm, struct kvm_sev_cmd *argp) > +{ > + struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info; > + struct kvm_sev_init data; > + > + if (!sev->need_init) > + return -EINVAL; > + > + if (kvm->arch.vm_type != KVM_X86_SEV_VM && > + kvm->arch.vm_type != KVM_X86_SEV_ES_VM) > + return -EINVAL; > + > + if (copy_from_user(&data, (void __user *)(uintptr_t)argp->data, sizeof(data))) > + return -EFAULT; > + > + return __sev_guest_init(kvm, argp, &data, kvm->arch.vm_type); > +} > + > static int sev_bind_asid(struct kvm *kvm, unsigned int handle, int *error) > { > struct sev_data_activate activate; > @@ -1915,6 +1952,9 @@ int sev_mem_enc_ioctl(struct kvm *kvm, void __user *argp) > case KVM_SEV_INIT: > r = sev_guest_init(kvm, &sev_cmd); > break; > + case KVM_SEV_INIT2: > + r = sev_guest_init2(kvm, &sev_cmd); > + break; > case KVM_SEV_LAUNCH_START: > r = sev_launch_start(kvm, &sev_cmd); > break; > -- > 2.39.0 > >
On 10/2/24 05:37, Paolo Bonzini wrote: > The idea that no parameter would ever be necessary when enabling SEV or > SEV-ES for a VM was decidedly optimistic. In fact, in some sense it's > already a parameter whether SEV or SEV-ES is desired. Another possible > source of variability is the desired set of VMSA features, as that affects > the measurement of the VM's initial state and cannot be changed > arbitrarily by the hypervisor. > > Create a new sub-operation for KVM_MEM_ENCRYPT_OP that can take a struct, a typo here: KVM_MEMORY_ENCRYPT_OP.
On 2/15/24 02:34, Michael Roth wrote: >> + struct struct kvm_sev_init { > Missing the vm_type param here. It can go away in the struct actually. Also, "struct struct". >> +If the ``KVM_X86_SEV_VMSA_FEATURES`` attribute does not exist, the hypervisor only >> +supports KVM_SEV_INIT and KVM_SEV_ES_INIT. In that case the set of VMSA features is >> +undefined. > > It's hard to imagine userspace implementation support for querying > KVM_X86_SEV_VMSA_FEATURES but still insisting on KVM_SEV_INIT. ... except for backwards compatibility with old kernels. For example, the VMM could first invoke HAS_DEVICE_ATTR, and then fall back to KVM_SEV_INIT after checking that the user did not explicitly request or forbid one or more VMSA features. > Maybe it > would be better to just lock in that VMSA_FEATURES at what is currently > supported: DEBUG_SWAP=on/off depending on the kvm_amd module param, and > then for all other features require opt-in via KVM_SEV_INIT2, and then > bake that into the documentation. That way way they could still reference > this documentation to properly calculate measurements for older/existing > VMM implementations. Thinking more about it, I think all features including debug_swap should be disabled with the old SEV_INIT/SEV_ES_INIT. Because the features need to match between the VMM and the measurement calculation, they need to be added explicitly on e.g. the QEMU command line. Paolo
On Thu, Feb 15, 2024 at 02:44:47PM +0100, Paolo Bonzini wrote: > On 2/15/24 02:34, Michael Roth wrote: > > > + struct struct kvm_sev_init { > > Missing the vm_type param here. > > It can go away in the struct actually. Also, "struct struct". > > > > +If the ``KVM_X86_SEV_VMSA_FEATURES`` attribute does not exist, the hypervisor only > > > +supports KVM_SEV_INIT and KVM_SEV_ES_INIT. In that case the set of VMSA features is > > > +undefined. > > > > It's hard to imagine userspace implementation support for querying > > KVM_X86_SEV_VMSA_FEATURES but still insisting on KVM_SEV_INIT. > > ... except for backwards compatibility with old kernels. For example, the > VMM could first invoke HAS_DEVICE_ATTR, and then fall back to KVM_SEV_INIT > after checking that the user did not explicitly request or forbid one or > more VMSA features. What I mean is that if userspace is modified for these checks, it's reasonable to also inform them that only VMSA features present in those older kernels (i.e. debug-swap) will be available via KVM_SEV_INIT, and for anything else they will need to use KVM_SEV_INIT. That way we can provide clear documentation on what to expect regarding VMSA features for KVM_SEV_INIT and not have to have the "undefined" wording: it'll never use anything other than debug-swap depending on the module param setting. > > > Maybe it > > would be better to just lock in that VMSA_FEATURES at what is currently > > supported: DEBUG_SWAP=on/off depending on the kvm_amd module param, and > > then for all other features require opt-in via KVM_SEV_INIT2, and then > > bake that into the documentation. That way way they could still reference > > this documentation to properly calculate measurements for older/existing > > VMM implementations. > > Thinking more about it, I think all features including debug_swap should be > disabled with the old SEV_INIT/SEV_ES_INIT. Because the features need to > match between the VMM and the measurement calculation, they need to be added > explicitly on e.g. the QEMU command line. That seems reasonable, but the main thing I was hoping to avoid was another round of VMSA features changing out from underneath the covers again. The module param setting is something we've needed to convey internally/externally a good bit due to the fallout and making this change would lead to another repeat. Not the end of the world but would be nice to avoid if possible. -Mike > > Paolo >
On Thu, Feb 15, 2024 at 3:44 PM Michael Roth <michael.roth@amd.com> wrote: > What I mean is that if userspace is modified for these checks, it's > reasonable to also inform them that only VMSA features present in > those older kernels (i.e. debug-swap) will be available via KVM_SEV_INIT, > and for anything else they will need to use KVM_SEV_INIT. > > That way we can provide clear documentation on what to expect regarding > VMSA features for KVM_SEV_INIT and not have to have the "undefined" > wording: it'll never use anything other than debug-swap depending on the > module param setting. Ah, I agree. > That seems reasonable, but the main thing I was hoping to avoid was > another round of VMSA features changing out from underneath the covers > again. The module param setting is something we've needed to convey > internally/externally a good bit due to the fallout and making this > change would lead to another repeat. Not the end of the world but would > be nice to avoid if possible. The fallout was caused by old kernels not supporting debug-swap and now by failing measurements. As far as I know there is no downside of leaving it disabled by default, and it will fix booting old guest kernels. Paolo
On Thu, Feb 15, 2024 at 06:28:18PM +0100, Paolo Bonzini wrote: > On Thu, Feb 15, 2024 at 3:44 PM Michael Roth <michael.roth@amd.com> wrote: > > What I mean is that if userspace is modified for these checks, it's > > reasonable to also inform them that only VMSA features present in > > those older kernels (i.e. debug-swap) will be available via KVM_SEV_INIT, > > and for anything else they will need to use KVM_SEV_INIT. > > > > That way we can provide clear documentation on what to expect regarding > > VMSA features for KVM_SEV_INIT and not have to have the "undefined" > > wording: it'll never use anything other than debug-swap depending on the > > module param setting. > > Ah, I agree. > > > That seems reasonable, but the main thing I was hoping to avoid was > > another round of VMSA features changing out from underneath the covers > > again. The module param setting is something we've needed to convey > > internally/externally a good bit due to the fallout and making this > > change would lead to another repeat. Not the end of the world but would > > be nice to avoid if possible. > > The fallout was caused by old kernels not supporting debug-swap and > now by failing measurements. As far as I know there is no downside of > leaving it disabled by default, and it will fix booting old guest > kernels. Yah, agreed on older guest kernels, but it's the measurement side of things where we'd expect some additional fallout. The guidance was essentially that if you run a newer host kernel with debug-swap support, you need either need to: a) update your measurements to account for the additional VMSA feature b) disable debug-swap param to maintain previous behavior/measurement So those who'd taken approach a) would see another unexpected measurement change when they eventually update to a newer kernel. -Mike > > Paolo >
On Thu, Feb 15, 2024 at 6:55 PM Michael Roth <michael.roth@amd.com> wrote: > > The fallout was caused by old kernels not supporting debug-swap and > > now by failing measurements. As far as I know there is no downside of > > leaving it disabled by default, and it will fix booting old guest > > kernels. > > Yah, agreed on older guest kernels, but it's the measurement side of things > where we'd expect some additional fallout. The guidance was essentially that > if you run a newer host kernel with debug-swap support, you need either need > to: > > a) update your measurements to account for the additional VMSA feature > b) disable debug-swap param to maintain previous behavior/measurement Out of curiosity, where was this documented? While debug-swap was a pretty obvious culprit of the failed measurement, I didn't see any mention to it anywhere (and also didn't see any mention that old kernels would fail to boot in the KVM patches---which would have been a pretty clear indication that something like these patches was needed). > So those who'd taken approach a) would see another unexpected measurement > change when they eventually update to a newer kernel. But they'd see it anyway if userspace starts disabling it by default. In general, enabling _anything_ by default is a mistake in either KVM or userspace if you care about guest ABI (which you obviously do in the case of confidential computing). Paolo
On Thu, Feb 15, 2024 at 07:08:06PM +0100, Paolo Bonzini wrote: > On Thu, Feb 15, 2024 at 6:55 PM Michael Roth <michael.roth@amd.com> wrote: > > > The fallout was caused by old kernels not supporting debug-swap and > > > now by failing measurements. As far as I know there is no downside of > > > leaving it disabled by default, and it will fix booting old guest > > > kernels. > > > > Yah, agreed on older guest kernels, but it's the measurement side of things > > where we'd expect some additional fallout. The guidance was essentially that > > if you run a newer host kernel with debug-swap support, you need either need > > to: > > > > a) update your measurements to account for the additional VMSA feature > > b) disable debug-swap param to maintain previous behavior/measurement > > Out of curiosity, where was this documented? While debug-swap was a > pretty obvious culprit of the failed measurement, I didn't see any > mention to it anywhere (and also didn't see any mention that old > kernels would fail to boot in the KVM patches---which would have been > a pretty clear indication that something like these patches was > needed). Yes, this was reactive rather than proactive guidance unfortunately, resulting from various internal/external bug reports where we needed to suggest the above-mentioned options. In retrospect, I think we would've handled things differently as well. Which is why I'm hoping it's possible to ease the pain of another potential measurement change for those who've since incorporated debug-swap into their measurement workflow. But maybe it's not realistic... > > > So those who'd taken approach a) would see another unexpected measurement > > change when they eventually update to a newer kernel. > > But they'd see it anyway if userspace starts disabling it by default. My thinking was that this wording would be specific to KVM_SEV_INIT, as opposed to KVM_SEV_INIT2 where disabling all features by default should absolutely be the way to go. But realistically, it's not easy for a user to tell whether their VMM is using KVM_SEV_INIT vs KVM_SEV_INIT2, and it does seem possible that having the defaults be different between the 2 would also cause some pain down the road since even in looking at the documentation it wouldn't be immediately clear whether or not debug-swap would be enabled. So maybe your approach of always disabling by default and requiring userspace to opt-in would be better in the long-run since this behavior is fairly new from a distro perspective and it's likely only developers/early adopters that we'd be sticking the genie back in the bottle for. -Mike > In general, enabling _anything_ by default is a mistake in either KVM > or userspace if you care about guest ABI (which you obviously do in > the case of confidential computing). > > Paolo >
On 2/9/24 12:37, Paolo Bonzini wrote: > The idea that no parameter would ever be necessary when enabling SEV or > SEV-ES for a VM was decidedly optimistic. In fact, in some sense it's > already a parameter whether SEV or SEV-ES is desired. Another possible > source of variability is the desired set of VMSA features, as that affects > the measurement of the VM's initial state and cannot be changed > arbitrarily by the hypervisor. > > Create a new sub-operation for KVM_MEM_ENCRYPT_OP that can take a struct, > and put the new op to work by including the VMSA features as a field of the > struct. The existing KVM_SEV_INIT and KVM_SEV_ES_INIT use the full set of > supported VMSA features for backwards compatibility. > > The struct also includes the usual bells and whistles for future > extensibility: a flags field that must be zero for now, and some padding > at the end. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > .../virt/kvm/x86/amd-memory-encryption.rst | 41 ++++++++++++++-- > arch/x86/include/uapi/asm/kvm.h | 10 ++++ > arch/x86/kvm/svm/sev.c | 48 +++++++++++++++++-- > 3 files changed, 92 insertions(+), 7 deletions(-) > ... > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c > index acf5c45ef14e..78c52764453f 100644 > --- a/arch/x86/kvm/svm/sev.c > +++ b/arch/x86/kvm/svm/sev.c > @@ -252,7 +252,9 @@ static void sev_unbind_asid(struct kvm *kvm, unsigned int handle) > sev_decommission(handle); > } > > -static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp) > +static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp, > + struct kvm_sev_init *data, > + unsigned long vm_type) > { > struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info; > int asid, ret; > @@ -260,7 +262,10 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp) > if (kvm->created_vcpus) > return -EINVAL; > > - if (kvm->arch.vm_type != KVM_X86_DEFAULT_VM) > + if (data->flags) > + return -EINVAL; > + > + if (data->vmsa_features & ~sev_supported_vmsa_features) An SEV guest doesn't have protected state and so it doesn't have a VMSA to which you can apply features. So this should be: if (vm_type != KVM_X86_SEV_VM && (data->vmsa_features & ~sev_supported_vmsa_features)) Thanks, Tom > return -EINVAL; > > ret = -EBUSY; > @@ -268,8 +273,8 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp) > return ret; > > sev->active = true; > - sev->es_active = argp->id == KVM_SEV_ES_INIT; > - sev->vmsa_features = sev_supported_vmsa_features; > + sev->es_active = (vm_type & __KVM_X86_PROTECTED_STATE_TYPE) != 0; > + sev->vmsa_features = data->vmsa_features; > > asid = sev_asid_new(sev); > if (asid < 0) > @@ -298,6 +303,38 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp) > return ret; > } > > +static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp) > +{ > + struct kvm_sev_init data = { > + .vmsa_features = sev_supported_vmsa_features, > + }; > + unsigned long vm_type; > + > + if (kvm->arch.vm_type != KVM_X86_DEFAULT_VM) > + return -EINVAL; > + > + vm_type = (argp->id == KVM_SEV_INIT ? KVM_X86_SEV_VM : KVM_X86_SEV_ES_VM); > + return __sev_guest_init(kvm, argp, &data, vm_type); > +} > + > +static int sev_guest_init2(struct kvm *kvm, struct kvm_sev_cmd *argp) > +{ > + struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info; > + struct kvm_sev_init data; > + > + if (!sev->need_init) > + return -EINVAL; > + > + if (kvm->arch.vm_type != KVM_X86_SEV_VM && > + kvm->arch.vm_type != KVM_X86_SEV_ES_VM) > + return -EINVAL; > + > + if (copy_from_user(&data, (void __user *)(uintptr_t)argp->data, sizeof(data))) > + return -EFAULT; > + > + return __sev_guest_init(kvm, argp, &data, kvm->arch.vm_type); > +} > + > static int sev_bind_asid(struct kvm *kvm, unsigned int handle, int *error) > { > struct sev_data_activate activate; > @@ -1915,6 +1952,9 @@ int sev_mem_enc_ioctl(struct kvm *kvm, void __user *argp) > case KVM_SEV_INIT: > r = sev_guest_init(kvm, &sev_cmd); > break; > + case KVM_SEV_INIT2: > + r = sev_guest_init2(kvm, &sev_cmd); > + break; > case KVM_SEV_LAUNCH_START: > r = sev_launch_start(kvm, &sev_cmd); > break;
diff --git a/Documentation/virt/kvm/x86/amd-memory-encryption.rst b/Documentation/virt/kvm/x86/amd-memory-encryption.rst index 5ed11bc16b96..a4291e7bd8ed 100644 --- a/Documentation/virt/kvm/x86/amd-memory-encryption.rst +++ b/Documentation/virt/kvm/x86/amd-memory-encryption.rst @@ -75,15 +75,50 @@ are defined in ``<linux/psp-dev.h>``. KVM implements the following commands to support common lifecycle events of SEV guests, such as launching, running, snapshotting, migrating and decommissioning. -1. KVM_SEV_INIT ---------------- +1. KVM_SEV_INIT2 +---------------- -The KVM_SEV_INIT command is used by the hypervisor to initialize the SEV platform +The KVM_SEV_INIT2 command is used by the hypervisor to initialize the SEV platform context. In a typical workflow, this command should be the first command issued. +For this command to be accepted, either KVM_X86_SEV_VM or KVM_X86_SEV_ES_VM +must have been passed to the KVM_CREATE_VM ioctl. A virtual machine created +with those machine types in turn cannot be run until KVM_SEV_INIT2 is invoked. + +Parameters: struct kvm_sev_init (in) Returns: 0 on success, -negative on error +:: + + struct struct kvm_sev_init { + __u32 flags; /* must be 0 */ + __u64 vmsa_features; /* initial value of features field in VMSA */ + __u32 pad[8]; + }; + +It is an error if the hypervisor does not support any of the bits that +are set in ``flags`` or ``vmsa_features``. + +This command replaces the deprecated KVM_SEV_INIT and KVM_SEV_ES_INIT commands. +The commands did not have any parameters (the ```data``` field was unused) and +only work for the KVM_X86_DEFAULT_VM machine type (0). + +They behave as if: + +* the VM type is KVM_X86_SEV_VM for KVM_SEV_INIT, or KVM_X86_SEV_ES_VM for + KVM_SEV_ES_INIT + +* the ``flags`` field of ``struct kvm_sev_init`` is set to zero + +* the ``vmsa_features`` field of ``struct kvm_sev_init`` is set to all features + supported by the hypervisor (as returned by ``KVM_GET_DEVICE_ATTR`` when + passed group 0 and attribute id ``KVM_X86_SEV_VMSA_FEATURES``). + +If the ``KVM_X86_SEV_VMSA_FEATURES`` attribute does not exist, the hypervisor only +supports KVM_SEV_INIT and KVM_SEV_ES_INIT. In that case the set of VMSA features is +undefined. + 2. KVM_SEV_LAUNCH_START ----------------------- diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h index 7c46e96cfe62..6baf18335c7b 100644 --- a/arch/x86/include/uapi/asm/kvm.h +++ b/arch/x86/include/uapi/asm/kvm.h @@ -683,6 +683,9 @@ enum sev_cmd_id { /* Guest Migration Extension */ KVM_SEV_SEND_CANCEL, + /* Second time is the charm; improved versions of the above ioctls. */ + KVM_SEV_INIT2, + KVM_SEV_NR_MAX, }; @@ -694,6 +697,13 @@ struct kvm_sev_cmd { __u32 sev_fd; }; +struct kvm_sev_init { + __u32 vm_type; + __u32 flags; + __u64 vmsa_features; + __u32 pad[8]; +}; + struct kvm_sev_launch_start { __u32 handle; __u32 policy; diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c index acf5c45ef14e..78c52764453f 100644 --- a/arch/x86/kvm/svm/sev.c +++ b/arch/x86/kvm/svm/sev.c @@ -252,7 +252,9 @@ static void sev_unbind_asid(struct kvm *kvm, unsigned int handle) sev_decommission(handle); } -static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp) +static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp, + struct kvm_sev_init *data, + unsigned long vm_type) { struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info; int asid, ret; @@ -260,7 +262,10 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp) if (kvm->created_vcpus) return -EINVAL; - if (kvm->arch.vm_type != KVM_X86_DEFAULT_VM) + if (data->flags) + return -EINVAL; + + if (data->vmsa_features & ~sev_supported_vmsa_features) return -EINVAL; ret = -EBUSY; @@ -268,8 +273,8 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp) return ret; sev->active = true; - sev->es_active = argp->id == KVM_SEV_ES_INIT; - sev->vmsa_features = sev_supported_vmsa_features; + sev->es_active = (vm_type & __KVM_X86_PROTECTED_STATE_TYPE) != 0; + sev->vmsa_features = data->vmsa_features; asid = sev_asid_new(sev); if (asid < 0) @@ -298,6 +303,38 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp) return ret; } +static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp) +{ + struct kvm_sev_init data = { + .vmsa_features = sev_supported_vmsa_features, + }; + unsigned long vm_type; + + if (kvm->arch.vm_type != KVM_X86_DEFAULT_VM) + return -EINVAL; + + vm_type = (argp->id == KVM_SEV_INIT ? KVM_X86_SEV_VM : KVM_X86_SEV_ES_VM); + return __sev_guest_init(kvm, argp, &data, vm_type); +} + +static int sev_guest_init2(struct kvm *kvm, struct kvm_sev_cmd *argp) +{ + struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info; + struct kvm_sev_init data; + + if (!sev->need_init) + return -EINVAL; + + if (kvm->arch.vm_type != KVM_X86_SEV_VM && + kvm->arch.vm_type != KVM_X86_SEV_ES_VM) + return -EINVAL; + + if (copy_from_user(&data, (void __user *)(uintptr_t)argp->data, sizeof(data))) + return -EFAULT; + + return __sev_guest_init(kvm, argp, &data, kvm->arch.vm_type); +} + static int sev_bind_asid(struct kvm *kvm, unsigned int handle, int *error) { struct sev_data_activate activate; @@ -1915,6 +1952,9 @@ int sev_mem_enc_ioctl(struct kvm *kvm, void __user *argp) case KVM_SEV_INIT: r = sev_guest_init(kvm, &sev_cmd); break; + case KVM_SEV_INIT2: + r = sev_guest_init2(kvm, &sev_cmd); + break; case KVM_SEV_LAUNCH_START: r = sev_launch_start(kvm, &sev_cmd); break;
The idea that no parameter would ever be necessary when enabling SEV or SEV-ES for a VM was decidedly optimistic. In fact, in some sense it's already a parameter whether SEV or SEV-ES is desired. Another possible source of variability is the desired set of VMSA features, as that affects the measurement of the VM's initial state and cannot be changed arbitrarily by the hypervisor. Create a new sub-operation for KVM_MEM_ENCRYPT_OP that can take a struct, and put the new op to work by including the VMSA features as a field of the struct. The existing KVM_SEV_INIT and KVM_SEV_ES_INIT use the full set of supported VMSA features for backwards compatibility. The struct also includes the usual bells and whistles for future extensibility: a flags field that must be zero for now, and some padding at the end. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- .../virt/kvm/x86/amd-memory-encryption.rst | 41 ++++++++++++++-- arch/x86/include/uapi/asm/kvm.h | 10 ++++ arch/x86/kvm/svm/sev.c | 48 +++++++++++++++++-- 3 files changed, 92 insertions(+), 7 deletions(-)