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

2019-08-26 Thread 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 
>> ---
>>   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(_no_apic_vcpu);
>> +    mutex_lock(>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(>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(>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


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

2019-08-19 Thread Alexander Graf




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 
---
  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"?



+   APICV_ACTIVATED,/* Default status when APICV is enabled */
+};
+
  struct kvm_arch {
unsigned long n_used_mmu_pages;
unsigned long n_requested_mmu_pages;
@@ -873,6 +882,8 @@ struct kvm_arch {
struct kvm_apic_map *apic_map;
  
  	bool apic_access_page_done;

+   struct mutex apicv_lock;
+   enum kvm_apicv_state apicv_state;
  
  	gpa_t wall_clock;
  
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c

index 7daf0dd..f9c3f63 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4584,6 +4584,8 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
kvm->arch.irqchip_mode = KVM_IRQCHIP_SPLIT;
kvm->arch.nr_reserved_ioapic_pins = cap->args[0];
r = 0;
+   if (kvm_x86_ops->get_enable_apicv(kvm))
+   kvm->arch.apicv_state = APICV_ACTIVATED;
  split_irqchip_unlock:
mutex_unlock(>lock);
break;
@@ -4701,6 +4703,8 @@ long kvm_arch_vm_ioctl(struct file *filp,
/* Write kvm->irq_routing before enabling irqchip_in_kernel. */
smp_wmb();
kvm->arch.irqchip_mode = KVM_IRQCHIP_KERNEL;
+   if (kvm_x86_ops->get_enable_apicv(kvm))
+   kvm->arch.apicv_state = APICV_ACTIVATED;
create_irqchip_unlock:
mutex_unlock(>lock);
break;
@@ -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?


r = kvm_create_lapic(vcpu, lapic_timer_advance_ns);
if (r < 0)
goto fail_mmu_destroy;
} else
static_key_slow_inc(_no_apic_vcpu);
  
+	mutex_lock(>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(>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(>arch.apicv_lock);


In fact, the whole lock story is not part of the patch description :).


Alex


+
if (kvm_x86_ops->vm_init)
return kvm_x86_ops->vm_init(kvm);