| Message ID | 20210707183616.5620-23-brijesh.singh@amd.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | Add AMD Secure Nested Paging (SEV-SNP) Hypervisor Support | expand |
On Wed, Jul 07, 2021, Brijesh Singh wrote: > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h > index 3fd9a7e9d90c..989a64aa1ae5 100644 > --- a/include/uapi/linux/kvm.h > +++ b/include/uapi/linux/kvm.h > @@ -1678,6 +1678,9 @@ enum sev_cmd_id { > /* Guest Migration Extension */ > KVM_SEV_SEND_CANCEL, > > + /* SNP specific commands */ > + KVM_SEV_SNP_INIT = 256, Is there any meaning behind '256'? If not, why skip a big chunk? I wouldn't be concerned if it weren't for KVM_SEV_NR_MAX, whose existence arguably implies that 0-KVM_SEV_NR_MAX-1 are all valid SEV commands. > + > KVM_SEV_NR_MAX, > };
On 7/16/21 2:33 PM, Sean Christopherson wrote: > On Wed, Jul 07, 2021, Brijesh Singh wrote: >> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h >> index 3fd9a7e9d90c..989a64aa1ae5 100644 >> --- a/include/uapi/linux/kvm.h >> +++ b/include/uapi/linux/kvm.h >> @@ -1678,6 +1678,9 @@ enum sev_cmd_id { >> /* Guest Migration Extension */ >> KVM_SEV_SEND_CANCEL, >> >> + /* SNP specific commands */ >> + KVM_SEV_SNP_INIT = 256, > Is there any meaning behind '256'? If not, why skip a big chunk? I wouldn't be > concerned if it weren't for KVM_SEV_NR_MAX, whose existence arguably implies that > 0-KVM_SEV_NR_MAX-1 are all valid SEV commands. In previous patches, Peter highlighted that we should keep some gap between the SEV/ES and SNP to leave room for legacy SEV/ES expansion. I was not sure how many we need to reserve without knowing what will come in the future; especially recently some of the command additional are not linked to the firmware. I am okay to reduce the gap or remove the gap all together.
On Fri, Jul 16, 2021, Brijesh Singh wrote: > > On 7/16/21 2:33 PM, Sean Christopherson wrote: > > On Wed, Jul 07, 2021, Brijesh Singh wrote: > >> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h > >> index 3fd9a7e9d90c..989a64aa1ae5 100644 > >> --- a/include/uapi/linux/kvm.h > >> +++ b/include/uapi/linux/kvm.h > >> @@ -1678,6 +1678,9 @@ enum sev_cmd_id { > >> /* Guest Migration Extension */ > >> KVM_SEV_SEND_CANCEL, > >> > >> + /* SNP specific commands */ > >> + KVM_SEV_SNP_INIT = 256, > > Is there any meaning behind '256'? If not, why skip a big chunk? I wouldn't be > > concerned if it weren't for KVM_SEV_NR_MAX, whose existence arguably implies that > > 0-KVM_SEV_NR_MAX-1 are all valid SEV commands. > > In previous patches, Peter highlighted that we should keep some gap > between the SEV/ES and SNP to leave room for legacy SEV/ES expansion. I > was not sure how many we need to reserve without knowing what will come > in the future; especially recently some of the command additional are > not linked to the firmware. I am okay to reduce the gap or remove the > gap all together. Unless the numbers themselves have meaning, which I don't think they do, I vote to keep the arbitrary numbers contiguous. KVM_SEV_NR_MAX makes me nervous, and there are already cases of related commands being discontiguous, e.g. KVM_SEND_CANCEL. Peter or Paolo, any thoughts?
diff --git a/Documentation/virt/kvm/amd-memory-encryption.rst b/Documentation/virt/kvm/amd-memory-encryption.rst index 5c081c8c7164..75ca60b6d40a 100644 --- a/Documentation/virt/kvm/amd-memory-encryption.rst +++ b/Documentation/virt/kvm/amd-memory-encryption.rst @@ -427,6 +427,22 @@ issued by the hypervisor to make the guest ready for execution. Returns: 0 on success, -negative on error +18. KVM_SNP_INIT +---------------- + +The KVM_SNP_INIT command can be used by the hypervisor to initialize SEV-SNP +context. In a typical workflow, this command should be the first command issued. + +Parameters (in): struct kvm_snp_init + +Returns: 0 on success, -negative on error + +:: + + struct kvm_snp_init { + __u64 flags; /* must be zero */ + }; + References ========== diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c index abca2b9dee83..be31221f0a47 100644 --- a/arch/x86/kvm/svm/sev.c +++ b/arch/x86/kvm/svm/sev.c @@ -228,10 +228,24 @@ static void sev_unbind_asid(struct kvm *kvm, unsigned int handle) sev_guest_decommission(&decommission, NULL); } +static int verify_snp_init_flags(struct kvm *kvm, struct kvm_sev_cmd *argp) +{ + struct kvm_snp_init params; + + if (copy_from_user(¶ms, (void __user *)(uintptr_t)argp->data, sizeof(params))) + return -EFAULT; + + if (params.flags) + return -EINVAL; + + return 0; +} + static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp) { + bool es_active = (argp->id == KVM_SEV_ES_INIT || argp->id == KVM_SEV_SNP_INIT); struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info; - bool es_active = argp->id == KVM_SEV_ES_INIT; + bool snp_active = argp->id == KVM_SEV_SNP_INIT; int asid, ret; if (kvm->created_vcpus) @@ -242,12 +256,22 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp) return ret; sev->es_active = es_active; + sev->snp_active = snp_active; asid = sev_asid_new(sev); if (asid < 0) goto e_no_asid; sev->asid = asid; - ret = sev_platform_init(&argp->error); + if (snp_active) { + ret = verify_snp_init_flags(kvm, argp); + if (ret) + goto e_free; + + ret = sev_snp_init(&argp->error); + } else { + ret = sev_platform_init(&argp->error); + } + if (ret) goto e_free; @@ -591,6 +615,9 @@ static int sev_es_sync_vmsa(struct vcpu_svm *svm) save->pkru = svm->vcpu.arch.pkru; save->xss = svm->vcpu.arch.ia32_xss; + if (sev_snp_guest(svm->vcpu.kvm)) + save->sev_features |= SVM_SEV_FEATURES_SNP_ACTIVE; + return 0; } @@ -1523,6 +1550,12 @@ int svm_mem_enc_op(struct kvm *kvm, void __user *argp) } switch (sev_cmd.id) { + case KVM_SEV_SNP_INIT: + if (!sev_snp_enabled) { + r = -ENOTTY; + goto out; + } + fallthrough; case KVM_SEV_ES_INIT: if (!sev_es_enabled) { r = -ENOTTY; diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h index 3fd9a7e9d90c..989a64aa1ae5 100644 --- a/include/uapi/linux/kvm.h +++ b/include/uapi/linux/kvm.h @@ -1678,6 +1678,9 @@ enum sev_cmd_id { /* Guest Migration Extension */ KVM_SEV_SEND_CANCEL, + /* SNP specific commands */ + KVM_SEV_SNP_INIT = 256, + KVM_SEV_NR_MAX, }; @@ -1774,6 +1777,10 @@ struct kvm_sev_receive_update_data { __u32 trans_len; }; +struct kvm_snp_init { + __u64 flags; +}; + #define KVM_DEV_ASSIGN_ENABLE_IOMMU (1 << 0) #define KVM_DEV_ASSIGN_PCI_2_3 (1 << 1) #define KVM_DEV_ASSIGN_MASK_INTX (1 << 2)
The KVM_SNP_INIT command is used by the hypervisor to initialize the SEV-SNP platform context. In a typical workflow, this command should be the first command issued. When creating SEV-SNP guest, the VMM must use this command instead of the KVM_SEV_INIT or KVM_SEV_ES_INIT. The flags value must be zero, it will be extended in future SNP support to communicate the optional features (such as restricted INT injection etc). Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- .../virt/kvm/amd-memory-encryption.rst | 16 ++++++++ arch/x86/kvm/svm/sev.c | 37 ++++++++++++++++++- include/uapi/linux/kvm.h | 7 ++++ 3 files changed, 58 insertions(+), 2 deletions(-)