diff mbox series

kvm/i386: Set proper nested state format for SVM

Message ID fe53d00fe0d884e812960781284cd48ae9206acc.1605546140.git.thomas.lendacky@amd.com (mailing list archive)
State New, archived
Headers show
Series kvm/i386: Set proper nested state format for SVM | expand

Commit Message

Tom Lendacky Nov. 16, 2020, 5:02 p.m. UTC 
From: Tom Lendacky <thomas.lendacky@amd.com>

Currently, the nested state format is hardcoded to VMX. This will result
in kvm_put_nested_state() returning an error because the KVM SVM support
checks for the nested state to be KVM_STATE_NESTED_FORMAT_SVM. As a
result, kvm_arch_put_registers() errors out early.

Update the setting of the format based on the virtualization feature:
  VMX - KVM_STATE_NESTED_FORMAT_VMX
  SVM - KVM_STATE_NESTED_FORMAT_SVM

Also, fix the code formatting while at it.

Fixes: b16c0e20c7 ("KVM: add support for AMD nested live migration")
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Richard Henderson <richard.henderson@linaro.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Marcelo Tosatti <mtosatti@redhat.com>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
 target/i386/kvm.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

Comments

Paolo Bonzini Nov. 16, 2020, 6:09 p.m. UTC | #1 
On 16/11/20 18:02, Tom Lendacky wrote:
> From: Tom Lendacky<thomas.lendacky@amd.com>
> 
> Currently, the nested state format is hardcoded to VMX. This will result
> in kvm_put_nested_state() returning an error because the KVM SVM support
> checks for the nested state to be KVM_STATE_NESTED_FORMAT_SVM. As a
> result, kvm_arch_put_registers() errors out early.
> 
> Update the setting of the format based on the virtualization feature:
>    VMX - KVM_STATE_NESTED_FORMAT_VMX
>    SVM - KVM_STATE_NESTED_FORMAT_SVM

Looks good, but what are the symptoms of this in practice?

Paolo
Tom Lendacky Nov. 16, 2020, 6:25 p.m. UTC | #2 
On 11/16/20 12:09 PM, Paolo Bonzini wrote:
> On 16/11/20 18:02, Tom Lendacky wrote:
>> From: Tom Lendacky<thomas.lendacky@amd.com>
>>
>> Currently, the nested state format is hardcoded to VMX. This will result
>> in kvm_put_nested_state() returning an error because the KVM SVM support
>> checks for the nested state to be KVM_STATE_NESTED_FORMAT_SVM. As a
>> result, kvm_arch_put_registers() errors out early.
>>
>> Update the setting of the format based on the virtualization feature:
>>    VMX - KVM_STATE_NESTED_FORMAT_VMX
>>    SVM - KVM_STATE_NESTED_FORMAT_SVM
> 
> Looks good, but what are the symptoms of this in practice?

I discovered this while testing my SEV-ES patches. When I specified the
'+svm' feature, the new SEV-ES reset address for the APs wasn't getting
set because kvm_arch_put_registers() erred out before it could call
kvm_getput_regs(). This resulted in the guest crashing when OVMF tried to
start the APs.

For a non-SEV-ES guest, I'm not sure if other updates could be missed,
potentially.

Thanks,
Tom

> 
> Paolo
>
Paolo Bonzini Nov. 16, 2020, 7:24 p.m. UTC | #3 
On 16/11/20 19:25, Tom Lendacky wrote:
> On 11/16/20 12:09 PM, Paolo Bonzini wrote:
>> On 16/11/20 18:02, Tom Lendacky wrote:
>>> From: Tom Lendacky<thomas.lendacky@amd.com>
>>>
>>> Currently, the nested state format is hardcoded to VMX. This will result
>>> in kvm_put_nested_state() returning an error because the KVM SVM support
>>> checks for the nested state to be KVM_STATE_NESTED_FORMAT_SVM. As a
>>> result, kvm_arch_put_registers() errors out early.
>>>
>>> Update the setting of the format based on the virtualization feature:
>>>     VMX - KVM_STATE_NESTED_FORMAT_VMX
>>>     SVM - KVM_STATE_NESTED_FORMAT_SVM
>>
>> Looks good, but what are the symptoms of this in practice?
> 
> I discovered this while testing my SEV-ES patches. When I specified the
> '+svm' feature, the new SEV-ES reset address for the APs wasn't getting
> set because kvm_arch_put_registers() erred out before it could call
> kvm_getput_regs(). This resulted in the guest crashing when OVMF tried to
> start the APs.
> 
> For a non-SEV-ES guest, I'm not sure if other updates could be missed,
> potentially.

Ok, thanks.  It's certainly a potential source of bugs, I've queued the 
patch.

Paolo
diff mbox series

Patch

diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index cf46259534..a2934dda02 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -1820,12 +1820,14 @@  int kvm_arch_init_vcpu(CPUState *cs)
 
             env->nested_state = g_malloc0(max_nested_state_len);
             env->nested_state->size = max_nested_state_len;
-            env->nested_state->format = KVM_STATE_NESTED_FORMAT_VMX;
 
             if (cpu_has_vmx(env)) {
-                    vmx_hdr = &env->nested_state->hdr.vmx;
-                    vmx_hdr->vmxon_pa = -1ull;
-                    vmx_hdr->vmcs12_pa = -1ull;
+                env->nested_state->format = KVM_STATE_NESTED_FORMAT_VMX;
+                vmx_hdr = &env->nested_state->hdr.vmx;
+                vmx_hdr->vmxon_pa = -1ull;
+                vmx_hdr->vmcs12_pa = -1ull;
+            } else {
+                env->nested_state->format = KVM_STATE_NESTED_FORMAT_SVM;
             }
         }
     }