[v3,02/24] KVM: x86: new irqchip mode KVM_IRQCHIP_INIT_IN_PROGRESS
diff mbox

Message ID 20170407085041.5257-3-david@redhat.com
State New
Headers show

Commit Message

David Hildenbrand April 7, 2017, 8:50 a.m. UTC
Let's add a new mode and set it while we create the irqchip via
KVM_CREATE_IRQCHIP and KVM_CAP_SPLIT_IRQCHIP.

This mode will be used later to test if adding routes
(in kvm_set_routing_entry()) is already allowed.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 arch/x86/include/asm/kvm_host.h |  1 +
 arch/x86/kvm/irq.h              | 18 +++++++++++++-----
 arch/x86/kvm/x86.c              | 11 ++++++++++-
 3 files changed, 24 insertions(+), 6 deletions(-)

Comments

Radim Krčmář April 12, 2017, 6:26 p.m. UTC | #1
2017-04-07 10:50+0200, David Hildenbrand:
> Let's add a new mode and set it while we create the irqchip via
> KVM_CREATE_IRQCHIP and KVM_CAP_SPLIT_IRQCHIP.
> 
> This mode will be used later to test if adding routes
> (in kvm_set_routing_entry()) is already allowed.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> @@ -3934,9 +3934,14 @@ static int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
>  			goto split_irqchip_unlock;
>  		if (kvm->created_vcpus)
>  			goto split_irqchip_unlock;
> +		kvm->arch.irqchip_mode = KVM_IRQCHIP_INIT_IN_PROGRESS;
>  		r = kvm_setup_empty_irq_routing(kvm);
> -		if (r)
> +		if (r) {
> +			kvm->arch.irqchip_mode = KVM_IRQCHIP_NONE;
> +			/* Pairs with smp_rmb() when reading irqchip_mode */
> +			smp_wmb();
>  			goto split_irqchip_unlock;
> +		}
>  		/* Pairs with irqchip_in_kernel. */
>  		smp_wmb();
>  		kvm->arch.irqchip_mode = KVM_IRQCHIP_SPLIT;
> @@ -4024,8 +4029,12 @@ long kvm_arch_vm_ioctl(struct file *filp,
>  			goto create_irqchip_unlock;
>  		}
>  
> +		kvm->arch.irqchip_mode = KVM_IRQCHIP_INIT_IN_PROGRESS;
>  		r = kvm_setup_default_irq_routing(kvm);
>  		if (r) {
> +			kvm->arch.irqchip_mode = KVM_IRQCHIP_NONE;
> +			/* Pairs with smp_rmb() when reading irqchip_mode */
> +			smp_wmb();

I think these two barriers are pointless.

KVM_IRQCHIP_INIT_IN_PROGRESS and KVM_IRQCHIP_NONE have the same result
for readers of irqchip mode and we don't even have a barrier after
setting KVM_IRQCHIP_INIT_IN_PROGRESS mode, so it looks weird.

I can remove them if you agree.

Thanks.
David Hildenbrand April 12, 2017, 7:55 p.m. UTC | #2
On 12.04.2017 20:26, Radim Krčmář wrote:
> 2017-04-07 10:50+0200, David Hildenbrand:
>> Let's add a new mode and set it while we create the irqchip via
>> KVM_CREATE_IRQCHIP and KVM_CAP_SPLIT_IRQCHIP.
>>
>> This mode will be used later to test if adding routes
>> (in kvm_set_routing_entry()) is already allowed.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> @@ -3934,9 +3934,14 @@ static int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
>>  			goto split_irqchip_unlock;
>>  		if (kvm->created_vcpus)
>>  			goto split_irqchip_unlock;
>> +		kvm->arch.irqchip_mode = KVM_IRQCHIP_INIT_IN_PROGRESS;
>>  		r = kvm_setup_empty_irq_routing(kvm);
>> -		if (r)
>> +		if (r) {
>> +			kvm->arch.irqchip_mode = KVM_IRQCHIP_NONE;
>> +			/* Pairs with smp_rmb() when reading irqchip_mode */
>> +			smp_wmb();
>>  			goto split_irqchip_unlock;
>> +		}
>>  		/* Pairs with irqchip_in_kernel. */
>>  		smp_wmb();
>>  		kvm->arch.irqchip_mode = KVM_IRQCHIP_SPLIT;
>> @@ -4024,8 +4029,12 @@ long kvm_arch_vm_ioctl(struct file *filp,
>>  			goto create_irqchip_unlock;
>>  		}
>>  
>> +		kvm->arch.irqchip_mode = KVM_IRQCHIP_INIT_IN_PROGRESS;
>>  		r = kvm_setup_default_irq_routing(kvm);
>>  		if (r) {
>> +			kvm->arch.irqchip_mode = KVM_IRQCHIP_NONE;
>> +			/* Pairs with smp_rmb() when reading irqchip_mode */
>> +			smp_wmb();
> 
> I think these two barriers are pointless.
> 
> KVM_IRQCHIP_INIT_IN_PROGRESS and KVM_IRQCHIP_NONE have the same result
> for readers of irqchip mode and we don't even have a barrier after
> setting KVM_IRQCHIP_INIT_IN_PROGRESS mode, so it looks weird.
> 
> I can remove them if you agree.

Paolo requested this, and I agreed to rather have a unified handling for
memory barriers with kvm->arch.irqchip_mode, then trying to optimize for
special cases.

(implicit knowledge here is e.g. that this is protected by kvm->lock)

If you think we can drop this, feel free to drop.

Thanks!

> 
> Thanks.
>

Patch
diff mbox

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 74ef58c..164e544 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -727,6 +727,7 @@  struct kvm_hv {
 
 enum kvm_irqchip_mode {
 	KVM_IRQCHIP_NONE,
+	KVM_IRQCHIP_INIT_IN_PROGRESS, /* temporarily set during creation */
 	KVM_IRQCHIP_KERNEL,       /* created with KVM_CREATE_IRQCHIP */
 	KVM_IRQCHIP_SPLIT,        /* created with KVM_CAP_SPLIT_IRQCHIP */
 };
diff --git a/arch/x86/kvm/irq.h b/arch/x86/kvm/irq.h
index 40d5b2c..59e05fe 100644
--- a/arch/x86/kvm/irq.h
+++ b/arch/x86/kvm/irq.h
@@ -93,21 +93,29 @@  static inline int pic_in_kernel(struct kvm *kvm)
 
 static inline int irqchip_split(struct kvm *kvm)
 {
-	return kvm->arch.irqchip_mode == KVM_IRQCHIP_SPLIT;
+	int mode = kvm->arch.irqchip_mode;
+
+	/* Matches smp_wmb() when setting irqchip_mode */
+	smp_rmb();
+	return mode == KVM_IRQCHIP_SPLIT;
 }
 
 static inline int irqchip_kernel(struct kvm *kvm)
 {
-	return kvm->arch.irqchip_mode == KVM_IRQCHIP_KERNEL;
+	int mode = kvm->arch.irqchip_mode;
+
+	/* Matches smp_wmb() when setting irqchip_mode */
+	smp_rmb();
+	return mode == KVM_IRQCHIP_KERNEL;
 }
 
 static inline int irqchip_in_kernel(struct kvm *kvm)
 {
-	bool ret = kvm->arch.irqchip_mode != KVM_IRQCHIP_NONE;
+	int mode = kvm->arch.irqchip_mode;
 
-	/* Matches with wmb after initializing kvm->irq_routing. */
+	/* Matches smp_wmb() when setting irqchip_mode */
 	smp_rmb();
-	return ret;
+	return mode > KVM_IRQCHIP_INIT_IN_PROGRESS;
 }
 
 void kvm_pic_reset(struct kvm_kpic_state *s);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index ccbd45e..8f776f0 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3934,9 +3934,14 @@  static int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
 			goto split_irqchip_unlock;
 		if (kvm->created_vcpus)
 			goto split_irqchip_unlock;
+		kvm->arch.irqchip_mode = KVM_IRQCHIP_INIT_IN_PROGRESS;
 		r = kvm_setup_empty_irq_routing(kvm);
-		if (r)
+		if (r) {
+			kvm->arch.irqchip_mode = KVM_IRQCHIP_NONE;
+			/* Pairs with smp_rmb() when reading irqchip_mode */
+			smp_wmb();
 			goto split_irqchip_unlock;
+		}
 		/* Pairs with irqchip_in_kernel. */
 		smp_wmb();
 		kvm->arch.irqchip_mode = KVM_IRQCHIP_SPLIT;
@@ -4024,8 +4029,12 @@  long kvm_arch_vm_ioctl(struct file *filp,
 			goto create_irqchip_unlock;
 		}
 
+		kvm->arch.irqchip_mode = KVM_IRQCHIP_INIT_IN_PROGRESS;
 		r = kvm_setup_default_irq_routing(kvm);
 		if (r) {
+			kvm->arch.irqchip_mode = KVM_IRQCHIP_NONE;
+			/* Pairs with smp_rmb() when reading irqchip_mode */
+			smp_wmb();
 			mutex_lock(&kvm->slots_lock);
 			mutex_lock(&kvm->irq_lock);
 			kvm_ioapic_destroy(kvm);