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

[Suthikulpanit, Suravee]

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@xxxxxxx>
>> ---
>> Â 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