* [PATCH] KVM: nVMX: Eliminate vmcs02 pool
@ 2017-11-21 17:22 Mark Kanda
2017-11-23 16:59 ` David Hildenbrand
2017-11-23 23:46 ` Paolo Bonzini
0 siblings, 2 replies; 14+ messages in thread
From: Mark Kanda @ 2017-11-21 17:22 UTC (permalink / raw)
To: kvm; +Cc: pbonzini, rkrcmar, ameya.more, Mark Kanda, Jim Mattson
The potential performance advantages of a vmcs02 pool have never been
realized. To simplify the code, eliminate the pool. Instead, a single
vmcs02 is allocated per VCPU when the VCPU enters VMX operation.
Signed-off-by: Jim Mattson <jmattson@google.com>
Signed-off-by: Mark Kanda <mark.kanda@oracle.com>
Reviewed-by: Ameya More <ameya.more@oracle.com>
---
arch/x86/kvm/vmx.c | 146 +++++++++--------------------------------------------
1 file changed, 23 insertions(+), 123 deletions(-)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 7c3522a..400ddd7 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -181,7 +181,6 @@
extern const ulong vmx_return;
#define NR_AUTOLOAD_MSRS 8
-#define VMCS02_POOL_SIZE 1
struct vmcs {
u32 revision_id;
@@ -218,7 +217,7 @@ struct shared_msr_entry {
* stored in guest memory specified by VMPTRLD, but is opaque to the guest,
* which must access it using VMREAD/VMWRITE/VMCLEAR instructions.
* More than one of these structures may exist, if L1 runs multiple L2 guests.
- * nested_vmx_run() will use the data here to build a vmcs02: a VMCS for the
+ * nested_vmx_run() will use the data here to build the vmcs02: a VMCS for the
* underlying hardware which will be used to run L2.
* This structure is packed to ensure that its layout is identical across
* machines (necessary for live migration).
@@ -401,13 +400,6 @@ struct __packed vmcs12 {
*/
#define VMCS12_SIZE 0x1000
-/* Used to remember the last vmcs02 used for some recently used vmcs12s */
-struct vmcs02_list {
- struct list_head list;
- gpa_t vmptr;
- struct loaded_vmcs vmcs02;
-};
-
/*
* The nested_vmx structure is part of vcpu_vmx, and holds information we need
* for correct emulation of VMX (i.e., nested VMX) on this vcpu.
@@ -432,15 +424,15 @@ struct nested_vmx {
*/
bool sync_shadow_vmcs;
- /* vmcs02_list cache of VMCSs recently used to run L2 guests */
- struct list_head vmcs02_pool;
- int vmcs02_num;
bool change_vmcs01_virtual_x2apic_mode;
/* L2 must run next, and mustn't decide to exit to L1. */
bool nested_run_pending;
+
+ struct loaded_vmcs vmcs02;
+
/*
- * Guest pages referred to in vmcs02 with host-physical pointers, so
- * we must keep them pinned while L2 runs.
+ * Guest pages referred to in the vmcs02 with host-physical
+ * pointers, so we must keep them pinned while L2 runs.
*/
struct page *apic_access_page;
struct page *virtual_apic_page;
@@ -6930,94 +6922,6 @@ static int handle_monitor(struct kvm_vcpu *vcpu)
}
/*
- * To run an L2 guest, we need a vmcs02 based on the L1-specified vmcs12.
- * We could reuse a single VMCS for all the L2 guests, but we also want the
- * option to allocate a separate vmcs02 for each separate loaded vmcs12 - this
- * allows keeping them loaded on the processor, and in the future will allow
- * optimizations where prepare_vmcs02 doesn't need to set all the fields on
- * every entry if they never change.
- * So we keep, in vmx->nested.vmcs02_pool, a cache of size VMCS02_POOL_SIZE
- * (>=0) with a vmcs02 for each recently loaded vmcs12s, most recent first.
- *
- * The following functions allocate and free a vmcs02 in this pool.
- */
-
-/* Get a VMCS from the pool to use as vmcs02 for the current vmcs12. */
-static struct loaded_vmcs *nested_get_current_vmcs02(struct vcpu_vmx *vmx)
-{
- struct vmcs02_list *item;
- list_for_each_entry(item, &vmx->nested.vmcs02_pool, list)
- if (item->vmptr == vmx->nested.current_vmptr) {
- list_move(&item->list, &vmx->nested.vmcs02_pool);
- return &item->vmcs02;
- }
-
- if (vmx->nested.vmcs02_num >= max(VMCS02_POOL_SIZE, 1)) {
- /* Recycle the least recently used VMCS. */
- item = list_last_entry(&vmx->nested.vmcs02_pool,
- struct vmcs02_list, list);
- item->vmptr = vmx->nested.current_vmptr;
- list_move(&item->list, &vmx->nested.vmcs02_pool);
- return &item->vmcs02;
- }
-
- /* Create a new VMCS */
- item = kmalloc(sizeof(struct vmcs02_list), GFP_KERNEL);
- if (!item)
- return NULL;
- item->vmcs02.vmcs = alloc_vmcs();
- item->vmcs02.shadow_vmcs = NULL;
- if (!item->vmcs02.vmcs) {
- kfree(item);
- return NULL;
- }
- loaded_vmcs_init(&item->vmcs02);
- item->vmptr = vmx->nested.current_vmptr;
- list_add(&(item->list), &(vmx->nested.vmcs02_pool));
- vmx->nested.vmcs02_num++;
- return &item->vmcs02;
-}
-
-/* Free and remove from pool a vmcs02 saved for a vmcs12 (if there is one) */
-static void nested_free_vmcs02(struct vcpu_vmx *vmx, gpa_t vmptr)
-{
- struct vmcs02_list *item;
- list_for_each_entry(item, &vmx->nested.vmcs02_pool, list)
- if (item->vmptr == vmptr) {
- free_loaded_vmcs(&item->vmcs02);
- list_del(&item->list);
- kfree(item);
- vmx->nested.vmcs02_num--;
- return;
- }
-}
-
-/*
- * Free all VMCSs saved for this vcpu, except the one pointed by
- * vmx->loaded_vmcs. We must be running L1, so vmx->loaded_vmcs
- * must be &vmx->vmcs01.
- */
-static void nested_free_all_saved_vmcss(struct vcpu_vmx *vmx)
-{
- struct vmcs02_list *item, *n;
-
- WARN_ON(vmx->loaded_vmcs != &vmx->vmcs01);
- list_for_each_entry_safe(item, n, &vmx->nested.vmcs02_pool, list) {
- /*
- * Something will leak if the above WARN triggers. Better than
- * a use-after-free.
- */
- if (vmx->loaded_vmcs == &item->vmcs02)
- continue;
-
- free_loaded_vmcs(&item->vmcs02);
- list_del(&item->list);
- kfree(item);
- vmx->nested.vmcs02_num--;
- }
-}
-
-/*
* The following 3 functions, nested_vmx_succeed()/failValid()/failInvalid(),
* set the success or error code of an emulated VMX instruction, as specified
* by Vol 2B, VMX Instruction Reference, "Conventions".
@@ -7198,6 +7102,12 @@ static int enter_vmx_operation(struct kvm_vcpu *vcpu)
struct vcpu_vmx *vmx = to_vmx(vcpu);
struct vmcs *shadow_vmcs;
+ vmx->nested.vmcs02.vmcs = alloc_vmcs();
+ vmx->nested.vmcs02.shadow_vmcs = NULL;
+ if (!vmx->nested.vmcs02.vmcs)
+ goto out_vmcs02;
+ loaded_vmcs_init(&vmx->nested.vmcs02);
+
if (cpu_has_vmx_msr_bitmap()) {
vmx->nested.msr_bitmap =
(unsigned long *)__get_free_page(GFP_KERNEL);
@@ -7220,9 +7130,6 @@ static int enter_vmx_operation(struct kvm_vcpu *vcpu)
vmx->vmcs01.shadow_vmcs = shadow_vmcs;
}
- INIT_LIST_HEAD(&(vmx->nested.vmcs02_pool));
- vmx->nested.vmcs02_num = 0;
-
hrtimer_init(&vmx->nested.preemption_timer, CLOCK_MONOTONIC,
HRTIMER_MODE_REL_PINNED);
vmx->nested.preemption_timer.function = vmx_preemption_timer_fn;
@@ -7237,6 +7144,9 @@ static int enter_vmx_operation(struct kvm_vcpu *vcpu)
free_page((unsigned long)vmx->nested.msr_bitmap);
out_msr_bitmap:
+ free_loaded_vmcs(&vmx->nested.vmcs02);
+
+out_vmcs02:
return -ENOMEM;
}
@@ -7389,7 +7299,7 @@ static void free_nested(struct vcpu_vmx *vmx)
vmx->vmcs01.shadow_vmcs = NULL;
}
kfree(vmx->nested.cached_vmcs12);
- /* Unpin physical memory we referred to in current vmcs02 */
+ /* Unpin physical memory we referred to in the vmcs02 */
if (vmx->nested.apic_access_page) {
kvm_release_page_dirty(vmx->nested.apic_access_page);
vmx->nested.apic_access_page = NULL;
@@ -7405,7 +7315,7 @@ static void free_nested(struct vcpu_vmx *vmx)
vmx->nested.pi_desc = NULL;
}
- nested_free_all_saved_vmcss(vmx);
+ free_loaded_vmcs(&vmx->nested.vmcs02);
}
/* Emulate the VMXOFF instruction */
@@ -7448,8 +7358,6 @@ static int handle_vmclear(struct kvm_vcpu *vcpu)
vmptr + offsetof(struct vmcs12, launch_state),
&zero, sizeof(zero));
- nested_free_vmcs02(vmx, vmptr);
-
nested_vmx_succeed(vcpu);
return kvm_skip_emulated_instruction(vcpu);
}
@@ -8360,10 +8268,11 @@ static bool nested_vmx_exit_reflected(struct kvm_vcpu *vcpu, u32 exit_reason)
/*
* The host physical addresses of some pages of guest memory
- * are loaded into VMCS02 (e.g. L1's Virtual APIC Page). The CPU
- * may write to these pages via their host physical address while
- * L2 is running, bypassing any address-translation-based dirty
- * tracking (e.g. EPT write protection).
+ * are loaded into the vmcs02 (e.g. vmcs12's Virtual APIC
+ * Page). The CPU may write to these pages via their host
+ * physical address while L2 is running, bypassing any
+ * address-translation-based dirty tracking (e.g. EPT write
+ * protection).
*
* Mark them dirty on every exit from L2 to prevent them from
* getting out of sync with dirty tracking.
@@ -10809,20 +10718,15 @@ static int enter_vmx_non_root_mode(struct kvm_vcpu *vcpu, bool from_vmentry)
{
struct vcpu_vmx *vmx = to_vmx(vcpu);
struct vmcs12 *vmcs12 = get_vmcs12(vcpu);
- struct loaded_vmcs *vmcs02;
u32 msr_entry_idx;
u32 exit_qual;
- vmcs02 = nested_get_current_vmcs02(vmx);
- if (!vmcs02)
- return -ENOMEM;
-
enter_guest_mode(vcpu);
if (!(vmcs12->vm_entry_controls & VM_ENTRY_LOAD_DEBUG_CONTROLS))
vmx->nested.vmcs01_debugctl = vmcs_read64(GUEST_IA32_DEBUGCTL);
- vmx_switch_vmcs(vcpu, vmcs02);
+ vmx_switch_vmcs(vcpu, &vmx->nested.vmcs02);
vmx_segment_cache_clear(vmx);
if (prepare_vmcs02(vcpu, vmcs12, from_vmentry, &exit_qual)) {
@@ -11434,10 +11338,6 @@ static void nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 exit_reason,
vm_exit_controls_reset_shadow(vmx);
vmx_segment_cache_clear(vmx);
- /* if no vmcs02 cache requested, remove the one we used */
- if (VMCS02_POOL_SIZE == 0)
- nested_free_vmcs02(vmx, vmx->nested.current_vmptr);
-
/* Update any VMCS fields that might have changed while L2 ran */
vmcs_write32(VM_EXIT_MSR_LOAD_COUNT, vmx->msr_autoload.nr);
vmcs_write32(VM_ENTRY_MSR_LOAD_COUNT, vmx->msr_autoload.nr);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] KVM: nVMX: Eliminate vmcs02 pool
2017-11-21 17:22 [PATCH] KVM: nVMX: Eliminate vmcs02 pool Mark Kanda
@ 2017-11-23 16:59 ` David Hildenbrand
2017-11-23 17:17 ` Paolo Bonzini
2017-11-27 17:17 ` Jim Mattson
2017-11-23 23:46 ` Paolo Bonzini
1 sibling, 2 replies; 14+ messages in thread
From: David Hildenbrand @ 2017-11-23 16:59 UTC (permalink / raw)
To: Mark Kanda, kvm; +Cc: pbonzini, rkrcmar, ameya.more, Jim Mattson
On 21.11.2017 18:22, Mark Kanda wrote:
> The potential performance advantages of a vmcs02 pool have never been
> realized. To simplify the code, eliminate the pool. Instead, a single
> vmcs02 is allocated per VCPU when the VCPU enters VMX operation.
Did you do any performance measurement to come to the conclusion that we
can remove it? :)
>
> Signed-off-by: Jim Mattson <jmattson@google.com>
-> why is that in here?
> Signed-off-by: Mark Kanda <mark.kanda@oracle.com>
> Reviewed-by: Ameya More <ameya.more@oracle.com>
> ---
> arch/x86/kvm/vmx.c | 146
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] KVM: nVMX: Eliminate vmcs02 pool
2017-11-23 16:59 ` David Hildenbrand
@ 2017-11-23 17:17 ` Paolo Bonzini
2017-11-23 17:25 ` David Hildenbrand
2017-11-27 17:17 ` Jim Mattson
1 sibling, 1 reply; 14+ messages in thread
From: Paolo Bonzini @ 2017-11-23 17:17 UTC (permalink / raw)
To: David Hildenbrand, Mark Kanda, kvm; +Cc: rkrcmar, ameya.more, Jim Mattson
On 23/11/2017 17:59, David Hildenbrand wrote:
> On 21.11.2017 18:22, Mark Kanda wrote:
>> The potential performance advantages of a vmcs02 pool have never been
>> realized. To simplify the code, eliminate the pool. Instead, a single
>> vmcs02 is allocated per VCPU when the VCPU enters VMX operation.
>
> Did you do any performance measurement to come to the conclusion that we
> can remove it? :)
This is enough I guess:
#define VMCS02_POOL_SIZE 1
so there wasn't really a pool of them...
Paolo
>>
>> Signed-off-by: Jim Mattson <jmattson@google.com>
>
> -> why is that in here?
>
>> Signed-off-by: Mark Kanda <mark.kanda@oracle.com>
>> Reviewed-by: Ameya More <ameya.more@oracle.com>
>> ---
>> arch/x86/kvm/vmx.c | 146
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] KVM: nVMX: Eliminate vmcs02 pool
2017-11-23 17:17 ` Paolo Bonzini
@ 2017-11-23 17:25 ` David Hildenbrand
0 siblings, 0 replies; 14+ messages in thread
From: David Hildenbrand @ 2017-11-23 17:25 UTC (permalink / raw)
To: Paolo Bonzini, Mark Kanda, kvm; +Cc: rkrcmar, ameya.more, Jim Mattson
On 23.11.2017 18:17, Paolo Bonzini wrote:
> On 23/11/2017 17:59, David Hildenbrand wrote:
>> On 21.11.2017 18:22, Mark Kanda wrote:
>>> The potential performance advantages of a vmcs02 pool have never been
>>> realized. To simplify the code, eliminate the pool. Instead, a single
>>> vmcs02 is allocated per VCPU when the VCPU enters VMX operation.
>>
>> Did you do any performance measurement to come to the conclusion that we
>> can remove it? :)
>
> This is enough I guess:
>
> #define VMCS02_POOL_SIZE 1
>
> so there wasn't really a pool of them...
>
> Paolo
>
Then, getting rid of it makes perfect sense :)
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] KVM: nVMX: Eliminate vmcs02 pool
2017-11-21 17:22 [PATCH] KVM: nVMX: Eliminate vmcs02 pool Mark Kanda
2017-11-23 16:59 ` David Hildenbrand
@ 2017-11-23 23:46 ` Paolo Bonzini
2017-11-27 17:43 ` Mark Kanda
1 sibling, 1 reply; 14+ messages in thread
From: Paolo Bonzini @ 2017-11-23 23:46 UTC (permalink / raw)
To: Mark Kanda, kvm; +Cc: rkrcmar, ameya.more, Jim Mattson
On 21/11/2017 18:22, Mark Kanda wrote:
> - nested_free_all_saved_vmcss(vmx);
> + free_loaded_vmcs(&vmx->nested.vmcs02);
Please add to free_loaded_vmcs a WARN that the passed vmcs is not
vmx->loaded_vmcs.
Paolo
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] KVM: nVMX: Eliminate vmcs02 pool
2017-11-23 16:59 ` David Hildenbrand
2017-11-23 17:17 ` Paolo Bonzini
@ 2017-11-27 17:17 ` Jim Mattson
2017-11-27 17:18 ` Paolo Bonzini
1 sibling, 1 reply; 14+ messages in thread
From: Jim Mattson @ 2017-11-27 17:17 UTC (permalink / raw)
To: David Hildenbrand
Cc: Mark Kanda, kvm list, Paolo Bonzini, Radim Krčmář,
ameya.more
Mark was kind enough to upstream my change for me.
On Thu, Nov 23, 2017 at 8:59 AM, David Hildenbrand <david@redhat.com> wrote:
> On 21.11.2017 18:22, Mark Kanda wrote:
>> The potential performance advantages of a vmcs02 pool have never been
>> realized. To simplify the code, eliminate the pool. Instead, a single
>> vmcs02 is allocated per VCPU when the VCPU enters VMX operation.
>
> Did you do any performance measurement to come to the conclusion that we
> can remove it? :)
>
>>
>> Signed-off-by: Jim Mattson <jmattson@google.com>
>
> -> why is that in here?
>
>> Signed-off-by: Mark Kanda <mark.kanda@oracle.com>
>> Reviewed-by: Ameya More <ameya.more@oracle.com>
>> ---
>> arch/x86/kvm/vmx.c | 146
>
> --
>
> Thanks,
>
> David / dhildenb
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] KVM: nVMX: Eliminate vmcs02 pool
2017-11-27 17:17 ` Jim Mattson
@ 2017-11-27 17:18 ` Paolo Bonzini
2017-11-27 22:01 ` David Hildenbrand
0 siblings, 1 reply; 14+ messages in thread
From: Paolo Bonzini @ 2017-11-27 17:18 UTC (permalink / raw)
To: Jim Mattson, David Hildenbrand
Cc: Mark Kanda, kvm list, Radim Krčmář, ameya.more
On 27/11/2017 18:17, Jim Mattson wrote:
> Mark was kind enough to upstream my change for me.
Ok, then it should have you as the author. I'll fix that.
Paolo
> On Thu, Nov 23, 2017 at 8:59 AM, David Hildenbrand <david@redhat.com> wrote:
>> On 21.11.2017 18:22, Mark Kanda wrote:
>>> The potential performance advantages of a vmcs02 pool have never been
>>> realized. To simplify the code, eliminate the pool. Instead, a single
>>> vmcs02 is allocated per VCPU when the VCPU enters VMX operation.
>>
>> Did you do any performance measurement to come to the conclusion that we
>> can remove it? :)
>>
>>>
>>> Signed-off-by: Jim Mattson <jmattson@google.com>
>>
>> -> why is that in here?
>>
>>> Signed-off-by: Mark Kanda <mark.kanda@oracle.com>
>>> Reviewed-by: Ameya More <ameya.more@oracle.com>
>>> ---
>>> arch/x86/kvm/vmx.c | 146
>>
>> --
>>
>> Thanks,
>>
>> David / dhildenb
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] KVM: nVMX: Eliminate vmcs02 pool
2017-11-23 23:46 ` Paolo Bonzini
@ 2017-11-27 17:43 ` Mark Kanda
2017-11-27 17:51 ` Paolo Bonzini
0 siblings, 1 reply; 14+ messages in thread
From: Mark Kanda @ 2017-11-27 17:43 UTC (permalink / raw)
To: Paolo Bonzini, kvm; +Cc: rkrcmar, ameya.more, Jim Mattson
On 11/23/2017 5:46 PM, Paolo Bonzini wrote:
> On 21/11/2017 18:22, Mark Kanda wrote:
>> - nested_free_all_saved_vmcss(vmx);
>> + free_loaded_vmcs(&vmx->nested.vmcs02);
>
> Please add to free_loaded_vmcs a WARN that the passed vmcs is not
> vmx->loaded_vmcs.
>
Sure.
However, I don't see a way to access vmx from the passed in vmcs, which
would necessitate passing in vmx for the WARN (multiple callers) - I may
be missing something..
I'm happy to do this, but it seems possibly excessive for the sole
purpose of adding the WARN. Thoughts?
Thanks,
-Mark
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] KVM: nVMX: Eliminate vmcs02 pool
2017-11-27 17:43 ` Mark Kanda
@ 2017-11-27 17:51 ` Paolo Bonzini
2017-11-27 17:56 ` Mark Kanda
2017-11-27 20:04 ` Mark Kanda
0 siblings, 2 replies; 14+ messages in thread
From: Paolo Bonzini @ 2017-11-27 17:51 UTC (permalink / raw)
To: Mark Kanda, kvm; +Cc: rkrcmar, ameya.more, Jim Mattson
On 27/11/2017 18:43, Mark Kanda wrote:
> On 11/23/2017 5:46 PM, Paolo Bonzini wrote:
>> On 21/11/2017 18:22, Mark Kanda wrote:
>>> - nested_free_all_saved_vmcss(vmx);
>>> + free_loaded_vmcs(&vmx->nested.vmcs02);
>>
>> Please add to free_loaded_vmcs a WARN that the passed vmcs is not
>> vmx->loaded_vmcs.
>
> Sure.
>
> However, I don't see a way to access vmx from the passed in vmcs, which
> would necessitate passing in vmx for the WARN (multiple callers) - I may
> be missing something..
free_loaded_vmcs is only ever used on VMCS02's, so you can change it to
static void vmx_nested_free_vmcs02(struct vcpu_vmx *vmx)
{
struct loaded_vmcs *loaded_vmcs = &vmx->nested.vmcs02;
/*
* Just leak the VMCS02 if the WARN triggers. Better than
* a use-after-free.
*/
if (WARN_ON(vmx->loaded_vmcs == loaded_vmcs))
return;
...
}
> I'm happy to do this, but it seems possibly excessive for the sole
> purpose of adding the WARN. Thoughts?
We've had this kind of bug in the past, so I prefer to err on the side
of safety.
Paolo
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] KVM: nVMX: Eliminate vmcs02 pool
2017-11-27 17:51 ` Paolo Bonzini
@ 2017-11-27 17:56 ` Mark Kanda
2017-11-27 20:04 ` Mark Kanda
1 sibling, 0 replies; 14+ messages in thread
From: Mark Kanda @ 2017-11-27 17:56 UTC (permalink / raw)
To: Paolo Bonzini, kvm; +Cc: rkrcmar, ameya.more, Jim Mattson
On 11/27/2017 11:51 AM, Paolo Bonzini wrote:
> On 27/11/2017 18:43, Mark Kanda wrote:
>> On 11/23/2017 5:46 PM, Paolo Bonzini wrote:
>>> On 21/11/2017 18:22, Mark Kanda wrote:
>>>> - nested_free_all_saved_vmcss(vmx);
>>>> + free_loaded_vmcs(&vmx->nested.vmcs02);
>>>
>>> Please add to free_loaded_vmcs a WARN that the passed vmcs is not
>>> vmx->loaded_vmcs.
>>
>> Sure.
>>
>> However, I don't see a way to access vmx from the passed in vmcs, which
>> would necessitate passing in vmx for the WARN (multiple callers) - I may
>> be missing something..
>
> free_loaded_vmcs is only ever used on VMCS02's, so you can change it to
>
> static void vmx_nested_free_vmcs02(struct vcpu_vmx *vmx)
> {
> struct loaded_vmcs *loaded_vmcs = &vmx->nested.vmcs02;
>
> /*
> * Just leak the VMCS02 if the WARN triggers. Better than
> * a use-after-free.
> */
> if (WARN_ON(vmx->loaded_vmcs == loaded_vmcs))
> return;
> ...
> }
>
>> I'm happy to do this, but it seems possibly excessive for the sole
>> purpose of adding the WARN. Thoughts?
>
> We've had this kind of bug in the past, so I prefer to err on the side
> of safety.
>
Got it. I'll make this change for v2.
Thanks,
-Mark
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] KVM: nVMX: Eliminate vmcs02 pool
2017-11-27 17:51 ` Paolo Bonzini
2017-11-27 17:56 ` Mark Kanda
@ 2017-11-27 20:04 ` Mark Kanda
2017-11-27 20:36 ` Mark Kanda
1 sibling, 1 reply; 14+ messages in thread
From: Mark Kanda @ 2017-11-27 20:04 UTC (permalink / raw)
To: Paolo Bonzini, kvm; +Cc: rkrcmar, ameya.more, Jim Mattson
On 11/27/2017 11:51 AM, Paolo Bonzini wrote:
> On 27/11/2017 18:43, Mark Kanda wrote:
>> On 11/23/2017 5:46 PM, Paolo Bonzini wrote:
>>> On 21/11/2017 18:22, Mark Kanda wrote:
>>>> - nested_free_all_saved_vmcss(vmx);
>>>> + free_loaded_vmcs(&vmx->nested.vmcs02);
>>>
>>> Please add to free_loaded_vmcs a WARN that the passed vmcs is not
>>> vmx->loaded_vmcs.
>>
>> Sure.
>>
>> However, I don't see a way to access vmx from the passed in vmcs, which
>> would necessitate passing in vmx for the WARN (multiple callers) - I may
>> be missing something..
>
> free_loaded_vmcs is only ever used on VMCS02's, so you can change it to
Perhaps I'm missing something, but it seems the free_loaded_vmcs() use
in vmx_create_vcpu() (and perhaps vmx_free_vcpu()) is for VMCS01 ..correct?
If so, I guess I shouldn't rename the routine to be VMCS02 specific (but
I think it's fine otherwise).
Thanks,
-Mark
> static void vmx_nested_free_vmcs02(struct vcpu_vmx *vmx)
> {
> struct loaded_vmcs *loaded_vmcs = &vmx->nested.vmcs02;
>
> /*
> * Just leak the VMCS02 if the WARN triggers. Better than
> * a use-after-free.
> */
> if (WARN_ON(vmx->loaded_vmcs == loaded_vmcs))
> return;
> ...
> }
>
>> I'm happy to do this, but it seems possibly excessive for the sole
>> purpose of adding the WARN. Thoughts?
>
> We've had this kind of bug in the past, so I prefer to err on the side
> of safety.
>
> Paolo
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] KVM: nVMX: Eliminate vmcs02 pool
2017-11-27 20:04 ` Mark Kanda
@ 2017-11-27 20:36 ` Mark Kanda
2017-11-27 20:54 ` Paolo Bonzini
0 siblings, 1 reply; 14+ messages in thread
From: Mark Kanda @ 2017-11-27 20:36 UTC (permalink / raw)
To: Paolo Bonzini, kvm; +Cc: rkrcmar, ameya.more, Jim Mattson
On 11/27/2017 2:04 PM, Mark Kanda wrote:
> On 11/27/2017 11:51 AM, Paolo Bonzini wrote:
>> On 27/11/2017 18:43, Mark Kanda wrote:
>>> On 11/23/2017 5:46 PM, Paolo Bonzini wrote:
>>>> On 21/11/2017 18:22, Mark Kanda wrote:
>>>>> - nested_free_all_saved_vmcss(vmx);
>>>>> + free_loaded_vmcs(&vmx->nested.vmcs02);
>>>>
>>>> Please add to free_loaded_vmcs a WARN that the passed vmcs is not
>>>> vmx->loaded_vmcs.
>>>
>>> Sure.
>>>
>>> However, I don't see a way to access vmx from the passed in vmcs, which
>>> would necessitate passing in vmx for the WARN (multiple callers) - I may
>>> be missing something..
>>
>> free_loaded_vmcs is only ever used on VMCS02's, so you can change it to
>
> Perhaps I'm missing something, but it seems the free_loaded_vmcs() use
> in vmx_create_vcpu() (and perhaps vmx_free_vcpu()) is for VMCS01 ..correct?
>
How about I leave vmx_create_vcpu(), vmx_free_vcpu(), and
free_loaded_vmcs() unmodified, and use the following for VMCS02 cases
(in enter_vmx_operation() and free_nested()):
static void vmx_nested_free_vmcs02(struct vcpu_vmx *vmx)
{
struct loaded_vmcs *loaded_vmcs = &vmx->nested.vmcs02;
/*
* Just leak the VMCS02 if the WARN triggers. Better than
* a use-after-free.
*/
if (WARN_ON(vmx->loaded_vmcs == loaded_vmcs))
return;
free_loaded_vmcs(loaded_vmcs);
}
..okay?
Thanks,
-Mark
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] KVM: nVMX: Eliminate vmcs02 pool
2017-11-27 20:36 ` Mark Kanda
@ 2017-11-27 20:54 ` Paolo Bonzini
0 siblings, 0 replies; 14+ messages in thread
From: Paolo Bonzini @ 2017-11-27 20:54 UTC (permalink / raw)
To: Mark Kanda, kvm; +Cc: rkrcmar, ameya.more, Jim Mattson
On 27/11/2017 21:36, Mark Kanda wrote:
>>>
>>
>> Perhaps I'm missing something, but it seems the free_loaded_vmcs() use
>> in vmx_create_vcpu() (and perhaps vmx_free_vcpu()) is for VMCS01
>> ..correct?
>>
>
> How about I leave vmx_create_vcpu(), vmx_free_vcpu(), and
> free_loaded_vmcs() unmodified, and use the following for VMCS02 cases
> (in enter_vmx_operation() and free_nested()):
>
> static void vmx_nested_free_vmcs02(struct vcpu_vmx *vmx)
> {
> struct loaded_vmcs *loaded_vmcs = &vmx->nested.vmcs02;
>
> /*
> * Just leak the VMCS02 if the WARN triggers. Better than
> * a use-after-free.
> */
> if (WARN_ON(vmx->loaded_vmcs == loaded_vmcs))
> return;
> free_loaded_vmcs(loaded_vmcs);
> }
Yes, perfect.
Paolo
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] KVM: nVMX: Eliminate vmcs02 pool
2017-11-27 17:18 ` Paolo Bonzini
@ 2017-11-27 22:01 ` David Hildenbrand
0 siblings, 0 replies; 14+ messages in thread
From: David Hildenbrand @ 2017-11-27 22:01 UTC (permalink / raw)
To: Paolo Bonzini, Jim Mattson
Cc: Mark Kanda, kvm list, Radim Krčmář, ameya.more
On 27.11.2017 18:18, Paolo Bonzini wrote:
> On 27/11/2017 18:17, Jim Mattson wrote:
>> Mark was kind enough to upstream my change for me.
>
> Ok, then it should have you as the author. I'll fix that.
>
> Paolo
Feel free to add my
Reviewed-by: David Hildenbrand <david@redhat.com>
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2017-11-27 22:02 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-21 17:22 [PATCH] KVM: nVMX: Eliminate vmcs02 pool Mark Kanda
2017-11-23 16:59 ` David Hildenbrand
2017-11-23 17:17 ` Paolo Bonzini
2017-11-23 17:25 ` David Hildenbrand
2017-11-27 17:17 ` Jim Mattson
2017-11-27 17:18 ` Paolo Bonzini
2017-11-27 22:01 ` David Hildenbrand
2017-11-23 23:46 ` Paolo Bonzini
2017-11-27 17:43 ` Mark Kanda
2017-11-27 17:51 ` Paolo Bonzini
2017-11-27 17:56 ` Mark Kanda
2017-11-27 20:04 ` Mark Kanda
2017-11-27 20:36 ` Mark Kanda
2017-11-27 20:54 ` Paolo Bonzini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.