Re: [PATCH v2 02/15] kvm: x86: Introduce KVM APICv state

["Suthikulpanit, Suravee" <Suravee.Suthikulpanit@amd.com>]

Alex,

On 8/19/2019 4:49 AM, Alexander Graf wrote:
> 
> 
> On 15.08.19 18:25, Suthikulpanit, Suravee wrote:
>> Currently, after a VM boots with APICv enabled, it could go into
>> the following states:
>>    * activated   = VM is running w/ APICv
>>    * deactivated = VM deactivate APICv (temporary)
>>    * disabled    = VM deactivate APICv (permanent)
>>
>> Introduce KVM APICv state enum to help keep track of the APICv states
>> along with a new variable struct kvm_arch.apicv_state to store
>> the current state.
>>
>> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> ---
>>   arch/x86/include/asm/kvm_host.h | 11 +++++++++++
>>   arch/x86/kvm/x86.c              | 14 +++++++++++++-
>>   2 files changed, 24 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>> index 56bc702..04d7066 100644
>> --- a/arch/x86/include/asm/kvm_host.h
>> +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -845,6 +845,15 @@ enum kvm_irqchip_mode {
>>       KVM_IRQCHIP_SPLIT,        /* created with KVM_CAP_SPLIT_IRQCHIP */
>>   };
>> +/*
>> + * KVM assumes all vcpus in a VM operate in the same mode.
>> + */
>> +enum kvm_apicv_state {
>> +    APICV_DISABLED,        /* Disabled (such as for Hyper-V case) */
>> +    APICV_DEACTIVATED,    /* Deactivated tempoerary */
> 
> typo
> 
> I'm also not sure the name is 100% obvious. How about something like "suspended" or "paused"?

Ok, I'll change it to APICV_SUSPENDED.

>> ...
>> @@ -9150,13 +9154,18 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
>>           goto fail_free_pio_data;
>>       if (irqchip_in_kernel(vcpu->kvm)) {
>> -        vcpu->arch.apicv_active = kvm_x86_ops->get_enable_apicv(vcpu->kvm);
> 
> Why are you moving this into a locked section?

Since we introduced the apicv_state to track the VM APICv state, which is accessible
by each vcpu initialization code, we need to lock and check the state before setting
the per-vcpu apicv_active.

> 
>>           r = kvm_create_lapic(vcpu, lapic_timer_advance_ns);
>>           if (r < 0)
>>               goto fail_mmu_destroy;
>>       } else
>>           static_key_slow_inc(&kvm_no_apic_vcpu);
>> +    mutex_lock(&vcpu->kvm->arch.apicv_lock);
>> +    if (irqchip_in_kernel(vcpu->kvm) &&
>> +        vcpu->kvm->arch.apicv_state == APICV_ACTIVATED)
>> +        vcpu->arch.apicv_active = kvm_x86_ops->get_enable_apicv(vcpu->kvm);
>> +    mutex_unlock(&vcpu->kvm->arch.apicv_lock);
>> +
>>       vcpu->arch.mce_banks = kzalloc(KVM_MAX_MCE_BANKS * sizeof(u64) * 4,
>>                          GFP_KERNEL_ACCOUNT);
>>       if (!vcpu->arch.mce_banks) {
>> @@ -9255,6 +9264,9 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
>>       kvm_page_track_init(kvm);
>>       kvm_mmu_init_vm(kvm);
>> +    /* APICV initialization */
>> +    mutex_init(&kvm->arch.apicv_lock);
> 
> In fact, the whole lock story is not part of the patch description :).\\

Ok, I'll update the commit log to describe the lock .

Suravee