| Message ID | 20250227012541.3234589-6-seanjc@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | KVM: SVM: Attempt to cleanup SEV_FEATURES | expand |
On 2/27/2025 2:25 AM, Sean Christopherson wrote: > When handling an "AP Create" event, return an error if the "requested" SEV > features for the vCPU don't exactly match KVM's view of the VM-scoped > features. There is no known use case for heterogeneous SEV features across > vCPUs, and while KVM can't actually enforce an exact match since the value > in RAX isn't guaranteed to match what the guest shoved into the VMSA, KVM > can at least avoid knowingly letting the guest run in an unsupported state. > > E.g. if a VM is created with DebugSwap disabled, KVM will intercept #DBs > and DRs for all vCPUs, even if an AP is "created" with DebugSwap enabled in > its VMSA. > > Note, the GHCB spec only "requires" that "AP use the same interrupt > injection mechanism as the BSP", but given the disaster that is DebugSwap > and SEV_FEATURES in general, it's safe to say that AMD didn't consider all > possible complications with mismatching features between the BSP and APs. > > Opportunistically fold the check into the relevant request flavors; the > "request < AP_DESTROY" check is just a bizarre way of implementing the > AP_CREATE_ON_INIT => AP_CREATE fallthrough. > > Fixes: e366f92ea99e ("KVM: SEV: Support SEV-SNP AP Creation NAE event") > Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com> > Signed-off-by: Sean Christopherson <seanjc@google.com> Looks good. Even makes code simpler. A minor query below. Reviewed-by: Pankaj Gupta <pankaj.gupta@amd.com> > --- > arch/x86/kvm/svm/sev.c | 23 ++++++++--------------- > 1 file changed, 8 insertions(+), 15 deletions(-) > > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c > index 9aad0dae3a80..bad5834ec143 100644 > --- a/arch/x86/kvm/svm/sev.c > +++ b/arch/x86/kvm/svm/sev.c > @@ -3932,6 +3932,7 @@ void sev_snp_init_protected_guest_state(struct kvm_vcpu *vcpu) > > static int sev_snp_ap_creation(struct vcpu_svm *svm) > { > + struct kvm_sev_info *sev = to_kvm_sev_info(svm->vcpu.kvm); > struct kvm_vcpu *vcpu = &svm->vcpu; > struct kvm_vcpu *target_vcpu; > struct vcpu_svm *target_svm; > @@ -3963,26 +3964,18 @@ static int sev_snp_ap_creation(struct vcpu_svm *svm) > > mutex_lock(&target_svm->sev_es.snp_vmsa_mutex); > > - /* Interrupt injection mode shouldn't change for AP creation */ > - if (request < SVM_VMGEXIT_AP_DESTROY) { > - u64 sev_features; > - > - sev_features = vcpu->arch.regs[VCPU_REGS_RAX]; > - sev_features ^= to_kvm_sev_info(svm->vcpu.kvm)->vmsa_features; > - > - if (sev_features & SVM_SEV_FEAT_INT_INJ_MODES) { 'SVM_SEV_FEAT_INT_INJ_MODES' would even be required in any future use-case, maybe? Thanks, Pankaj > - vcpu_unimpl(vcpu, "vmgexit: invalid AP injection mode [%#lx] from guest\n", > - vcpu->arch.regs[VCPU_REGS_RAX]); > - ret = -EINVAL; > - goto out; > - } > - } > - > switch (request) { > case SVM_VMGEXIT_AP_CREATE_ON_INIT: > kick = false; > fallthrough; > case SVM_VMGEXIT_AP_CREATE: > + if (vcpu->arch.regs[VCPU_REGS_RAX] != sev->vmsa_features) { > + vcpu_unimpl(vcpu, "vmgexit: mismatched AP sev_features [%#lx] != [%#llx] from guest\n", > + vcpu->arch.regs[VCPU_REGS_RAX], sev->vmsa_features); > + ret = -EINVAL; > + goto out; > + } > + > if (!page_address_valid(vcpu, svm->vmcb->control.exit_info_2)) { > vcpu_unimpl(vcpu, "vmgexit: invalid AP VMSA address [%#llx] from guest\n", > svm->vmcb->control.exit_info_2);
On Thu, Feb 27, 2025, Pankaj Gupta wrote: > On 2/27/2025 2:25 AM, Sean Christopherson wrote: > > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c > > index 9aad0dae3a80..bad5834ec143 100644 > > --- a/arch/x86/kvm/svm/sev.c > > +++ b/arch/x86/kvm/svm/sev.c > > @@ -3932,6 +3932,7 @@ void sev_snp_init_protected_guest_state(struct kvm_vcpu *vcpu) > > static int sev_snp_ap_creation(struct vcpu_svm *svm) > > { > > + struct kvm_sev_info *sev = to_kvm_sev_info(svm->vcpu.kvm); > > struct kvm_vcpu *vcpu = &svm->vcpu; > > struct kvm_vcpu *target_vcpu; > > struct vcpu_svm *target_svm; > > @@ -3963,26 +3964,18 @@ static int sev_snp_ap_creation(struct vcpu_svm *svm) > > mutex_lock(&target_svm->sev_es.snp_vmsa_mutex); > > - /* Interrupt injection mode shouldn't change for AP creation */ > > - if (request < SVM_VMGEXIT_AP_DESTROY) { > > - u64 sev_features; > > - > > - sev_features = vcpu->arch.regs[VCPU_REGS_RAX]; > > - sev_features ^= to_kvm_sev_info(svm->vcpu.kvm)->vmsa_features; > > - > > - if (sev_features & SVM_SEV_FEAT_INT_INJ_MODES) { > > 'SVM_SEV_FEAT_INT_INJ_MODES' would even be required in any future use-case, > maybe? Can you elaborate? I don't quite follow what you're suggesting.
>>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c >>> index 9aad0dae3a80..bad5834ec143 100644 >>> --- a/arch/x86/kvm/svm/sev.c >>> +++ b/arch/x86/kvm/svm/sev.c >>> @@ -3932,6 +3932,7 @@ void sev_snp_init_protected_guest_state(struct kvm_vcpu *vcpu) >>> static int sev_snp_ap_creation(struct vcpu_svm *svm) >>> { >>> + struct kvm_sev_info *sev = to_kvm_sev_info(svm->vcpu.kvm); >>> struct kvm_vcpu *vcpu = &svm->vcpu; >>> struct kvm_vcpu *target_vcpu; >>> struct vcpu_svm *target_svm; >>> @@ -3963,26 +3964,18 @@ static int sev_snp_ap_creation(struct vcpu_svm *svm) >>> mutex_lock(&target_svm->sev_es.snp_vmsa_mutex); >>> - /* Interrupt injection mode shouldn't change for AP creation */ >>> - if (request < SVM_VMGEXIT_AP_DESTROY) { >>> - u64 sev_features; >>> - >>> - sev_features = vcpu->arch.regs[VCPU_REGS_RAX]; >>> - sev_features ^= to_kvm_sev_info(svm->vcpu.kvm)->vmsa_features; >>> - >>> - if (sev_features & SVM_SEV_FEAT_INT_INJ_MODES) { >> >> 'SVM_SEV_FEAT_INT_INJ_MODES' would even be required in any future use-case, >> maybe? > > Can you elaborate? I don't quite follow what you're suggesting. SVM_SEV_FEAT_INT_INJ_MODES macro is not used anymore? If there is no plan to use it, maybe we can remove that as well? Or I am missing something. Thanks, Pankaj
On Thu, Feb 27, 2025, Pankaj Gupta wrote: > > > > > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c > > > > index 9aad0dae3a80..bad5834ec143 100644 > > > > --- a/arch/x86/kvm/svm/sev.c > > > > +++ b/arch/x86/kvm/svm/sev.c > > > > @@ -3932,6 +3932,7 @@ void sev_snp_init_protected_guest_state(struct kvm_vcpu *vcpu) > > > > static int sev_snp_ap_creation(struct vcpu_svm *svm) > > > > { > > > > + struct kvm_sev_info *sev = to_kvm_sev_info(svm->vcpu.kvm); > > > > struct kvm_vcpu *vcpu = &svm->vcpu; > > > > struct kvm_vcpu *target_vcpu; > > > > struct vcpu_svm *target_svm; > > > > @@ -3963,26 +3964,18 @@ static int sev_snp_ap_creation(struct vcpu_svm *svm) > > > > mutex_lock(&target_svm->sev_es.snp_vmsa_mutex); > > > > - /* Interrupt injection mode shouldn't change for AP creation */ > > > > - if (request < SVM_VMGEXIT_AP_DESTROY) { > > > > - u64 sev_features; > > > > - > > > > - sev_features = vcpu->arch.regs[VCPU_REGS_RAX]; > > > > - sev_features ^= to_kvm_sev_info(svm->vcpu.kvm)->vmsa_features; > > > > - > > > > - if (sev_features & SVM_SEV_FEAT_INT_INJ_MODES) { > > > > > > 'SVM_SEV_FEAT_INT_INJ_MODES' would even be required in any future use-case, > > > maybe? > > > > Can you elaborate? I don't quite follow what you're suggesting. > > SVM_SEV_FEAT_INT_INJ_MODES macro is not used anymore? If there is no plan to > use it, maybe we can remove that as well? Ah, gotcha. I didn't realize it was a compound macro. Yeah, unless someone objects, I'll remove it when applying.
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c index 9aad0dae3a80..bad5834ec143 100644 --- a/arch/x86/kvm/svm/sev.c +++ b/arch/x86/kvm/svm/sev.c @@ -3932,6 +3932,7 @@ void sev_snp_init_protected_guest_state(struct kvm_vcpu *vcpu) static int sev_snp_ap_creation(struct vcpu_svm *svm) { + struct kvm_sev_info *sev = to_kvm_sev_info(svm->vcpu.kvm); struct kvm_vcpu *vcpu = &svm->vcpu; struct kvm_vcpu *target_vcpu; struct vcpu_svm *target_svm; @@ -3963,26 +3964,18 @@ static int sev_snp_ap_creation(struct vcpu_svm *svm) mutex_lock(&target_svm->sev_es.snp_vmsa_mutex); - /* Interrupt injection mode shouldn't change for AP creation */ - if (request < SVM_VMGEXIT_AP_DESTROY) { - u64 sev_features; - - sev_features = vcpu->arch.regs[VCPU_REGS_RAX]; - sev_features ^= to_kvm_sev_info(svm->vcpu.kvm)->vmsa_features; - - if (sev_features & SVM_SEV_FEAT_INT_INJ_MODES) { - vcpu_unimpl(vcpu, "vmgexit: invalid AP injection mode [%#lx] from guest\n", - vcpu->arch.regs[VCPU_REGS_RAX]); - ret = -EINVAL; - goto out; - } - } - switch (request) { case SVM_VMGEXIT_AP_CREATE_ON_INIT: kick = false; fallthrough; case SVM_VMGEXIT_AP_CREATE: + if (vcpu->arch.regs[VCPU_REGS_RAX] != sev->vmsa_features) { + vcpu_unimpl(vcpu, "vmgexit: mismatched AP sev_features [%#lx] != [%#llx] from guest\n", + vcpu->arch.regs[VCPU_REGS_RAX], sev->vmsa_features); + ret = -EINVAL; + goto out; + } + if (!page_address_valid(vcpu, svm->vmcb->control.exit_info_2)) { vcpu_unimpl(vcpu, "vmgexit: invalid AP VMSA address [%#llx] from guest\n", svm->vmcb->control.exit_info_2);
When handling an "AP Create" event, return an error if the "requested" SEV features for the vCPU don't exactly match KVM's view of the VM-scoped features. There is no known use case for heterogeneous SEV features across vCPUs, and while KVM can't actually enforce an exact match since the value in RAX isn't guaranteed to match what the guest shoved into the VMSA, KVM can at least avoid knowingly letting the guest run in an unsupported state. E.g. if a VM is created with DebugSwap disabled, KVM will intercept #DBs and DRs for all vCPUs, even if an AP is "created" with DebugSwap enabled in its VMSA. Note, the GHCB spec only "requires" that "AP use the same interrupt injection mechanism as the BSP", but given the disaster that is DebugSwap and SEV_FEATURES in general, it's safe to say that AMD didn't consider all possible complications with mismatching features between the BSP and APs. Opportunistically fold the check into the relevant request flavors; the "request < AP_DESTROY" check is just a bizarre way of implementing the AP_CREATE_ON_INIT => AP_CREATE fallthrough. Fixes: e366f92ea99e ("KVM: SEV: Support SEV-SNP AP Creation NAE event") Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com> Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/kvm/svm/sev.c | 23 ++++++++--------------- 1 file changed, 8 insertions(+), 15 deletions(-)