Re: [PATCH v2] kvm: nVMX: VMCLEAR an active shadow VMCS after last use

[Jim Mattson <jmattson@google.com>]

Sorry; I changed the title after reworking the patch. V1 was "[PATCH]
kvm: nVMX: Track active shadow VMCSs" (15 July 2016).

On Wed, Nov 2, 2016 at 10:23 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 28/10/2016 17:29, Jim Mattson wrote:
>> After a successful VM-entry with the "VMCS shadowing" VM-execution
>> control set, the shadow VMCS referenced by the VMCS link pointer field
>> in the current VMCS becomes active on the logical processor.
>>
>> A VMCS that is made active on more than one logical processor may become
>> corrupted. Therefore, before an active VMCS can be migrated to another
>> logical processor, the first logical processor must execute a VMCLEAR
>> for the active VMCS. VMCLEAR both ensures that all VMCS data are written
>> to memory and makes the VMCS inactive.
>>
>> Signed-off-by: Jim Mattson <jmattson@google.com>
>> Reviewed-By: David Matlack <dmatlack@google.com>
>
> The patch looks good but---sorry for the possibly stupid
> question---where was v1?
>
> Paolo
>
>> ---
>>  arch/x86/kvm/vmx.c | 22 +++++++++++++++-------
>>  1 file changed, 15 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index cf0b7be..046b03f 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -187,6 +187,7 @@ struct vmcs {
>>   */
>>  struct loaded_vmcs {
>>       struct vmcs *vmcs;
>> +     struct vmcs *shadow_vmcs;
>>       int cpu;
>>       int launched;
>>       struct list_head loaded_vmcss_on_cpu_link;
>> @@ -411,7 +412,6 @@ struct nested_vmx {
>>        * memory during VMXOFF, VMCLEAR, VMPTRLD.
>>        */
>>       struct vmcs12 *cached_vmcs12;
>> -     struct vmcs *current_shadow_vmcs;
>>       /*
>>        * Indicates if the shadow vmcs must be updated with the
>>        * data hold by vmcs12
>> @@ -1419,6 +1419,8 @@ static void vmcs_clear(struct vmcs *vmcs)
>>  static inline void loaded_vmcs_init(struct loaded_vmcs *loaded_vmcs)
>>  {
>>       vmcs_clear(loaded_vmcs->vmcs);
>> +     if (loaded_vmcs->shadow_vmcs && loaded_vmcs->launched)
>> +             vmcs_clear(loaded_vmcs->shadow_vmcs);
>>       loaded_vmcs->cpu = -1;
>>       loaded_vmcs->launched = 0;
>>  }
>> @@ -3562,6 +3564,7 @@ static void free_loaded_vmcs(struct loaded_vmcs *loaded_vmcs)
>>       loaded_vmcs_clear(loaded_vmcs);
>>       free_vmcs(loaded_vmcs->vmcs);
>>       loaded_vmcs->vmcs = NULL;
>> +     WARN_ON(loaded_vmcs->shadow_vmcs != NULL);
>>  }
>>
>>  static void free_kvm_area(void)
>> @@ -6696,6 +6699,7 @@ static struct loaded_vmcs *nested_get_current_vmcs02(struct vcpu_vmx *vmx)
>>       if (!item)
>>               return NULL;
>>       item->vmcs02.vmcs = alloc_vmcs();
>> +     item->vmcs02.shadow_vmcs = NULL;
>>       if (!item->vmcs02.vmcs) {
>>               kfree(item);
>>               return NULL;
>> @@ -7072,7 +7076,7 @@ static int handle_vmon(struct kvm_vcpu *vcpu)
>>               shadow_vmcs->revision_id |= (1u << 31);
>>               /* init shadow vmcs */
>>               vmcs_clear(shadow_vmcs);
>> -             vmx->nested.current_shadow_vmcs = shadow_vmcs;
>> +             vmx->vmcs01.shadow_vmcs = shadow_vmcs;
>>       }
>>
>>       INIT_LIST_HEAD(&(vmx->nested.vmcs02_pool));
>> @@ -7174,8 +7178,11 @@ static void free_nested(struct vcpu_vmx *vmx)
>>               free_page((unsigned long)vmx->nested.msr_bitmap);
>>               vmx->nested.msr_bitmap = NULL;
>>       }
>> -     if (enable_shadow_vmcs)
>> -             free_vmcs(vmx->nested.current_shadow_vmcs);
>> +     if (enable_shadow_vmcs) {
>> +             vmcs_clear(vmx->vmcs01.shadow_vmcs);
>> +             free_vmcs(vmx->vmcs01.shadow_vmcs);
>> +             vmx->vmcs01.shadow_vmcs = NULL;
>> +     }
>>       kfree(vmx->nested.cached_vmcs12);
>>       /* Unpin physical memory we referred to in current vmcs02 */
>>       if (vmx->nested.apic_access_page) {
>> @@ -7352,7 +7359,7 @@ static void copy_shadow_to_vmcs12(struct vcpu_vmx *vmx)
>>       int i;
>>       unsigned long field;
>>       u64 field_value;
>> -     struct vmcs *shadow_vmcs = vmx->nested.current_shadow_vmcs;
>> +     struct vmcs *shadow_vmcs = vmx->vmcs01.shadow_vmcs;
>>       const unsigned long *fields = shadow_read_write_fields;
>>       const int num_fields = max_shadow_read_write_fields;
>>
>> @@ -7401,7 +7408,7 @@ static void copy_vmcs12_to_shadow(struct vcpu_vmx *vmx)
>>       int i, q;
>>       unsigned long field;
>>       u64 field_value = 0;
>> -     struct vmcs *shadow_vmcs = vmx->nested.current_shadow_vmcs;
>> +     struct vmcs *shadow_vmcs = vmx->vmcs01.shadow_vmcs;
>>
>>       vmcs_load(shadow_vmcs);
>>
>> @@ -7591,7 +7598,7 @@ static int handle_vmptrld(struct kvm_vcpu *vcpu)
>>                       vmcs_set_bits(SECONDARY_VM_EXEC_CONTROL,
>>                                     SECONDARY_EXEC_SHADOW_VMCS);
>>                       vmcs_write64(VMCS_LINK_POINTER,
>> -                                  __pa(vmx->nested.current_shadow_vmcs));
>> +                                  __pa(vmx->vmcs01.shadow_vmcs));
>>                       vmx->nested.sync_shadow_vmcs = true;
>>               }
>>       }
>> @@ -9156,6 +9163,7 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id)
>>
>>       vmx->loaded_vmcs = &vmx->vmcs01;
>>       vmx->loaded_vmcs->vmcs = alloc_vmcs();
>> +     vmx->loaded_vmcs->shadow_vmcs = NULL;
>>       if (!vmx->loaded_vmcs->vmcs)
>>               goto free_msrs;
>>       if (!vmm_exclusive)
>>