* [PATCH] target/i386: kvm: Fix when nested state is needed for migration
@ 2019-06-24 23:05 Liran Alon
2019-07-02 16:39 ` Paolo Bonzini
0 siblings, 1 reply; 5+ messages in thread
From: Liran Alon @ 2019-06-24 23:05 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, kvm, Liran Alon, Karl Heubaum
When vCPU is in VMX operation and enters SMM mode,
it temporarily exits VMX operation but KVM maintained nested-state
still stores the VMXON region physical address, i.e. even when the
vCPU is in SMM mode then (nested_state->hdr.vmx.vmxon_pa != -1ull).
Therefore, there is no need to explicitly check for
KVM_STATE_NESTED_SMM_VMXON to determine if it is necessary
to save nested-state as part of migration stream.
In addition, destination must enable eVMCS if it is enabled on
source as specified by the KVM_STATE_NESTED_EVMCS flag, even if
the VMXON region is not set. Thus, change the code to require saving
nested-state as part of migration stream in case it is set.
Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
target/i386/machine.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/i386/machine.c b/target/i386/machine.c
index 851b249d1a39..e7d72faf9e24 100644
--- a/target/i386/machine.c
+++ b/target/i386/machine.c
@@ -999,7 +999,7 @@ static bool vmx_nested_state_needed(void *opaque)
return ((nested_state->format == KVM_STATE_NESTED_FORMAT_VMX) &&
((nested_state->hdr.vmx.vmxon_pa != -1ull) ||
- (nested_state->hdr.vmx.smm.flags & KVM_STATE_NESTED_SMM_VMXON)));
+ (nested_state->flags & KVM_STATE_NESTED_EVMCS)));
}
static const VMStateDescription vmstate_vmx_nested_state = {
--
2.20.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] target/i386: kvm: Fix when nested state is needed for migration
2019-06-24 23:05 [PATCH] target/i386: kvm: Fix when nested state is needed for migration Liran Alon
@ 2019-07-02 16:39 ` Paolo Bonzini
2019-07-04 14:31 ` Liran Alon
0 siblings, 1 reply; 5+ messages in thread
From: Paolo Bonzini @ 2019-07-02 16:39 UTC (permalink / raw)
To: Liran Alon, qemu-devel; +Cc: kvm, Karl Heubaum
On 25/06/19 01:05, Liran Alon wrote:
> When vCPU is in VMX operation and enters SMM mode,
> it temporarily exits VMX operation but KVM maintained nested-state
> still stores the VMXON region physical address, i.e. even when the
> vCPU is in SMM mode then (nested_state->hdr.vmx.vmxon_pa != -1ull).
>
> Therefore, there is no need to explicitly check for
> KVM_STATE_NESTED_SMM_VMXON to determine if it is necessary
> to save nested-state as part of migration stream.
>
> In addition, destination must enable eVMCS if it is enabled on
> source as specified by the KVM_STATE_NESTED_EVMCS flag, even if
> the VMXON region is not set. Thus, change the code to require saving
> nested-state as part of migration stream in case it is set.
>
> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
> Signed-off-by: Liran Alon <liran.alon@oracle.com>
> ---
> target/i386/machine.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/i386/machine.c b/target/i386/machine.c
> index 851b249d1a39..e7d72faf9e24 100644
> --- a/target/i386/machine.c
> +++ b/target/i386/machine.c
> @@ -999,7 +999,7 @@ static bool vmx_nested_state_needed(void *opaque)
>
> return ((nested_state->format == KVM_STATE_NESTED_FORMAT_VMX) &&
> ((nested_state->hdr.vmx.vmxon_pa != -1ull) ||
> - (nested_state->hdr.vmx.smm.flags & KVM_STATE_NESTED_SMM_VMXON)));
> + (nested_state->flags & KVM_STATE_NESTED_EVMCS)));
> }
>
> static const VMStateDescription vmstate_vmx_nested_state = {
>
Queued, thanks.
Paolo
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] target/i386: kvm: Fix when nested state is needed for migration
2019-07-02 16:39 ` Paolo Bonzini
@ 2019-07-04 14:31 ` Liran Alon
2019-07-04 15:29 ` Paolo Bonzini
0 siblings, 1 reply; 5+ messages in thread
From: Liran Alon @ 2019-07-04 14:31 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, kvm, Karl Heubaum
> On 2 Jul 2019, at 19:39, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 25/06/19 01:05, Liran Alon wrote:
>> When vCPU is in VMX operation and enters SMM mode,
>> it temporarily exits VMX operation but KVM maintained nested-state
>> still stores the VMXON region physical address, i.e. even when the
>> vCPU is in SMM mode then (nested_state->hdr.vmx.vmxon_pa != -1ull).
>>
>> Therefore, there is no need to explicitly check for
>> KVM_STATE_NESTED_SMM_VMXON to determine if it is necessary
>> to save nested-state as part of migration stream.
>>
>> In addition, destination must enable eVMCS if it is enabled on
>> source as specified by the KVM_STATE_NESTED_EVMCS flag, even if
>> the VMXON region is not set. Thus, change the code to require saving
>> nested-state as part of migration stream in case it is set.
>>
>> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
>> Signed-off-by: Liran Alon <liran.alon@oracle.com>
>> ---
>> target/i386/machine.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/target/i386/machine.c b/target/i386/machine.c
>> index 851b249d1a39..e7d72faf9e24 100644
>> --- a/target/i386/machine.c
>> +++ b/target/i386/machine.c
>> @@ -999,7 +999,7 @@ static bool vmx_nested_state_needed(void *opaque)
>>
>> return ((nested_state->format == KVM_STATE_NESTED_FORMAT_VMX) &&
>> ((nested_state->hdr.vmx.vmxon_pa != -1ull) ||
>> - (nested_state->hdr.vmx.smm.flags & KVM_STATE_NESTED_SMM_VMXON)));
>> + (nested_state->flags & KVM_STATE_NESTED_EVMCS)));
>> }
>>
>> static const VMStateDescription vmstate_vmx_nested_state = {
>>
>
> Queued, thanks.
>
> Paolo
Actually Paolo after I have created KVM patch
("KVM: nVMX: Change KVM_STATE_NESTED_EVMCS to signal vmcs12 is copied from eVMCS”)
I think I realised that KVM_STATE_NESTED_EVMCS is actually not a requirement for nested-state to be sent.
I suggest to replace this commit with another one that just change vmx_nested_state_needed() to return true
In case format is FORMAT_VMX and vmxon_pa != -1ull and that’s it.
As anyway, QEMU provisioned on destination side is going to enable the relevant eVMCS capability.
I’m going to send another series that refines QEMU nested-migration a bit more so I will do it along the way.
But I think this patch should be un-queued. Sorry for realizing this later but at least it’s before it was merged to master :)
-Liran
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] target/i386: kvm: Fix when nested state is needed for migration
2019-07-04 14:31 ` Liran Alon
@ 2019-07-04 15:29 ` Paolo Bonzini
2019-07-04 15:34 ` Liran Alon
0 siblings, 1 reply; 5+ messages in thread
From: Paolo Bonzini @ 2019-07-04 15:29 UTC (permalink / raw)
To: Liran Alon; +Cc: qemu-devel, kvm, Karl Heubaum
On 04/07/19 16:31, Liran Alon wrote:
>
>
>> On 2 Jul 2019, at 19:39, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>> On 25/06/19 01:05, Liran Alon wrote:
>>> When vCPU is in VMX operation and enters SMM mode,
>>> it temporarily exits VMX operation but KVM maintained nested-state
>>> still stores the VMXON region physical address, i.e. even when the
>>> vCPU is in SMM mode then (nested_state->hdr.vmx.vmxon_pa != -1ull).
>>>
>>> Therefore, there is no need to explicitly check for
>>> KVM_STATE_NESTED_SMM_VMXON to determine if it is necessary
>>> to save nested-state as part of migration stream.
>>>
>>> In addition, destination must enable eVMCS if it is enabled on
>>> source as specified by the KVM_STATE_NESTED_EVMCS flag, even if
>>> the VMXON region is not set. Thus, change the code to require saving
>>> nested-state as part of migration stream in case it is set.
>>>
>>> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
>>> Signed-off-by: Liran Alon <liran.alon@oracle.com>
>>> ---
>>> target/i386/machine.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/target/i386/machine.c b/target/i386/machine.c
>>> index 851b249d1a39..e7d72faf9e24 100644
>>> --- a/target/i386/machine.c
>>> +++ b/target/i386/machine.c
>>> @@ -999,7 +999,7 @@ static bool vmx_nested_state_needed(void *opaque)
>>>
>>> return ((nested_state->format == KVM_STATE_NESTED_FORMAT_VMX) &&
>>> ((nested_state->hdr.vmx.vmxon_pa != -1ull) ||
>>> - (nested_state->hdr.vmx.smm.flags & KVM_STATE_NESTED_SMM_VMXON)));
>>> + (nested_state->flags & KVM_STATE_NESTED_EVMCS)));
>>> }
>>>
>>> static const VMStateDescription vmstate_vmx_nested_state = {
>>>
>>
>> Queued, thanks.
>>
>> Paolo
>
> Actually Paolo after I have created KVM patch
> ("KVM: nVMX: Change KVM_STATE_NESTED_EVMCS to signal vmcs12 is copied from eVMCS”)
> I think I realised that KVM_STATE_NESTED_EVMCS is actually not a requirement for nested-state to be sent.
> I suggest to replace this commit with another one that just change vmx_nested_state_needed() to return true
> In case format is FORMAT_VMX and vmxon_pa != -1ull and that’s it.
>
> As anyway, QEMU provisioned on destination side is going to enable the relevant eVMCS capability.
> I’m going to send another series that refines QEMU nested-migration a bit more so I will do it along the way.
> But I think this patch should be un-queued. Sorry for realizing this later but at least it’s before it was merged to master :)
Replaced with
diff --git a/target/i386/machine.c b/target/i386/machine.c
index 851b249d1a..704ba6de46 100644
--- a/target/i386/machine.c
+++ b/target/i386/machine.c
@@ -997,9 +997,8 @@ static bool vmx_nested_state_needed(void *opaque)
{
struct kvm_nested_state *nested_state = opaque;
- return ((nested_state->format == KVM_STATE_NESTED_FORMAT_VMX) &&
- ((nested_state->hdr.vmx.vmxon_pa != -1ull) ||
- (nested_state->hdr.vmx.smm.flags & KVM_STATE_NESTED_SMM_VMXON)));
+ return (nested_state->format == KVM_STATE_NESTED_FORMAT_VMX &&
+ nested_state->hdr.vmx.vmxon_pa != -1ull);
}
static const VMStateDescription vmstate_vmx_nested_state = {
and dropped the last paragraph of the commit message.
Paolo
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] target/i386: kvm: Fix when nested state is needed for migration
2019-07-04 15:29 ` Paolo Bonzini
@ 2019-07-04 15:34 ` Liran Alon
0 siblings, 0 replies; 5+ messages in thread
From: Liran Alon @ 2019-07-04 15:34 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, kvm, Karl Heubaum
> On 4 Jul 2019, at 18:29, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 04/07/19 16:31, Liran Alon wrote:
>>
>>
>>> On 2 Jul 2019, at 19:39, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>
>>> On 25/06/19 01:05, Liran Alon wrote:
>>>> When vCPU is in VMX operation and enters SMM mode,
>>>> it temporarily exits VMX operation but KVM maintained nested-state
>>>> still stores the VMXON region physical address, i.e. even when the
>>>> vCPU is in SMM mode then (nested_state->hdr.vmx.vmxon_pa != -1ull).
>>>>
>>>> Therefore, there is no need to explicitly check for
>>>> KVM_STATE_NESTED_SMM_VMXON to determine if it is necessary
>>>> to save nested-state as part of migration stream.
>>>>
>>>> In addition, destination must enable eVMCS if it is enabled on
>>>> source as specified by the KVM_STATE_NESTED_EVMCS flag, even if
>>>> the VMXON region is not set. Thus, change the code to require saving
>>>> nested-state as part of migration stream in case it is set.
>>>>
>>>> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
>>>> Signed-off-by: Liran Alon <liran.alon@oracle.com>
>>>> ---
>>>> target/i386/machine.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/target/i386/machine.c b/target/i386/machine.c
>>>> index 851b249d1a39..e7d72faf9e24 100644
>>>> --- a/target/i386/machine.c
>>>> +++ b/target/i386/machine.c
>>>> @@ -999,7 +999,7 @@ static bool vmx_nested_state_needed(void *opaque)
>>>>
>>>> return ((nested_state->format == KVM_STATE_NESTED_FORMAT_VMX) &&
>>>> ((nested_state->hdr.vmx.vmxon_pa != -1ull) ||
>>>> - (nested_state->hdr.vmx.smm.flags & KVM_STATE_NESTED_SMM_VMXON)));
>>>> + (nested_state->flags & KVM_STATE_NESTED_EVMCS)));
>>>> }
>>>>
>>>> static const VMStateDescription vmstate_vmx_nested_state = {
>>>>
>>>
>>> Queued, thanks.
>>>
>>> Paolo
>>
>> Actually Paolo after I have created KVM patch
>> ("KVM: nVMX: Change KVM_STATE_NESTED_EVMCS to signal vmcs12 is copied from eVMCS”)
>> I think I realised that KVM_STATE_NESTED_EVMCS is actually not a requirement for nested-state to be sent.
>> I suggest to replace this commit with another one that just change vmx_nested_state_needed() to return true
>> In case format is FORMAT_VMX and vmxon_pa != -1ull and that’s it.
>>
>> As anyway, QEMU provisioned on destination side is going to enable the relevant eVMCS capability.
>> I’m going to send another series that refines QEMU nested-migration a bit more so I will do it along the way.
>> But I think this patch should be un-queued. Sorry for realizing this later but at least it’s before it was merged to master :)
>
> Replaced with
>
> diff --git a/target/i386/machine.c b/target/i386/machine.c
> index 851b249d1a..704ba6de46 100644
> --- a/target/i386/machine.c
> +++ b/target/i386/machine.c
> @@ -997,9 +997,8 @@ static bool vmx_nested_state_needed(void *opaque)
> {
> struct kvm_nested_state *nested_state = opaque;
>
> - return ((nested_state->format == KVM_STATE_NESTED_FORMAT_VMX) &&
> - ((nested_state->hdr.vmx.vmxon_pa != -1ull) ||
> - (nested_state->hdr.vmx.smm.flags & KVM_STATE_NESTED_SMM_VMXON)));
> + return (nested_state->format == KVM_STATE_NESTED_FORMAT_VMX &&
> + nested_state->hdr.vmx.vmxon_pa != -1ull);
> }
>
> static const VMStateDescription vmstate_vmx_nested_state = {
>
> and dropped the last paragraph of the commit message.
>
> Paolo
>
Yep that’s what I wanted to do.
Thanks. I have some more patches pending anyway but unrelated to this. This now seems good to me.
-Liran
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-07-04 15:35 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-24 23:05 [PATCH] target/i386: kvm: Fix when nested state is needed for migration Liran Alon
2019-07-02 16:39 ` Paolo Bonzini
2019-07-04 14:31 ` Liran Alon
2019-07-04 15:29 ` Paolo Bonzini
2019-07-04 15:34 ` Liran Alon
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).