[PATCH 0/4] KVM: cleanups around memory barriers and irqchip_in_kernel

[PATCH 0/4] KVM: cleanups around memory barriers and irqchip_in_kernel

[Paolo Bonzini]

I took a look at memory barriers in KVM and there are a few that are
unjustified, underdocumented or wrong.  While looking at irqchip_in_kernel
uses (to understand the memory barriers) I also noticed one case where it
is unnecessary to check for irqchip_in_kernel (patch 3).

Most of the users of irqchip_in_kernel do not need the barrier and they
could check vcpu->arch.apic instead of vcpu->kvm->arch.pic.  I plan to
change that by introducing lapic_in_kernel(struct kvm_vcpu *), after
the split irqchip patches are in which will make irqchip_in_kernel more
expensive.

Paolo

Paolo Bonzini (4):
  KVM: x86: remove unnecessary memory barriers for shared MSRs
  KVM: document memory barriers for kvm->vcpus/kvm->online_vcpus
  KVM: i8254: remove unnecessary irqchip_in_kernel check
  KVM: x86: clean/fix memory barriers in irqchip_in_kernel

 arch/x86/kvm/i8254.c     |  2 +-
 arch/x86/kvm/i8259.c     | 15 +++++----------
 arch/x86/kvm/irq.h       |  8 ++++----
 arch/x86/kvm/x86.c       | 21 +++++++--------------
 include/linux/kvm_host.h |  4 ++++
 virt/kvm/kvm_main.c      |  2 ++
 6 files changed, 23 insertions(+), 29 deletions(-)

-- 
1.8.3.1

[PATCH 1/4] KVM: x86: remove unnecessary memory barriers for shared MSRs

[Paolo Bonzini]

There is no smp_rmb matching the smp_wmb.  shared_msr_update is called from
hardware_enable, which in turn is called via on_each_cpu.  on_each_cpu
and must imply a read memory barrier (on x86 the rmb is achieved simply
through asm volatile in native_apic_mem_write).

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/x86.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index c675ea3351cf..2d62229aac26 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -222,11 +222,9 @@ static void shared_msr_update(unsigned slot, u32 msr)
 void kvm_define_shared_msr(unsigned slot, u32 msr)
 {
 	BUG_ON(slot >= KVM_NR_SHARED_MSRS);
+	shared_msrs_global.msrs[slot] = msr;
 	if (slot >= shared_msrs_global.nr)
 		shared_msrs_global.nr = slot + 1;
-	shared_msrs_global.msrs[slot] = msr;
-	/* we need ensured the shared_msr_global have been updated */
-	smp_wmb();
 }
 EXPORT_SYMBOL_GPL(kvm_define_shared_msr);
 
-- 
1.8.3.1

[PATCH 2/4] KVM: document memory barriers for kvm->vcpus/kvm->online_vcpus

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/linux/kvm_host.h | 4 ++++
 virt/kvm/kvm_main.c      | 2 ++
 2 files changed, 6 insertions(+)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index bd1097a95704..81089cf1f0c1 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -427,6 +427,10 @@ struct kvm {
 
 static inline struct kvm_vcpu *kvm_get_vcpu(struct kvm *kvm, int i)
 {
+	/* Pairs with smp_wmb() in kvm_vm_ioctl_create_vcpu, in case
+	 * the caller has read kvm->online_vcpus before (as is the case
+	 * for kvm_for_each_vcpu, for example).
+	 */
 	smp_rmb();
 	return kvm->vcpus[i];
 }
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 8dc4828f623f..093b3d10b411 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2206,6 +2206,8 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, u32 id)
 	}
 
 	kvm->vcpus[atomic_read(&kvm->online_vcpus)] = vcpu;
+
+	/* Pairs with smp_rmb() in kvm_get_vcpu.  */
 	smp_wmb();
 	atomic_inc(&kvm->online_vcpus);
 
-- 
1.8.3.1

[PATCH 3/4] KVM: i8254: remove unnecessary irqchip_in_kernel check

[Paolo Bonzini]

The PIT is only created if irqchip_in_kernel returns true, so the
check is superfluous.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/i8254.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
index f90952f64e79..f588eb7bdf45 100644
--- a/arch/x86/kvm/i8254.c
+++ b/arch/x86/kvm/i8254.c
@@ -333,7 +333,7 @@ static void create_pit_timer(struct kvm *kvm, u32 val, int is_period)
 	struct kvm_kpit_state *ps = &kvm->arch.vpit->pit_state;
 	s64 interval;
 
-	if (!irqchip_in_kernel(kvm) || ps->flags & KVM_PIT_FLAGS_HPET_LEGACY)
+	if (ps->flags & KVM_PIT_FLAGS_HPET_LEGACY)
 		return;
 
 	interval = muldiv64(val, NSEC_PER_SEC, KVM_PIT_FREQ);
-- 
1.8.3.1

[PATCH 4/4] KVM: x86: clean/fix memory barriers in irqchip_in_kernel

[Paolo Bonzini]

The memory barriers are trying to protect against concurrent RCU-based
interrupt injection, but the IRQ routing table is not valid at the time
kvm->arch.vpic is written.  Fix this by writing kvm->arch.vpic last.
kvm_destroy_pic then need not set kvm->arch.vpic to NULL; modify it
to take a struct kvm_pic* and reuse it if the IOAPIC creation fails.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/i8259.c | 15 +++++----------
 arch/x86/kvm/irq.h   |  8 ++++----
 arch/x86/kvm/x86.c   | 17 ++++++-----------
 3 files changed, 15 insertions(+), 25 deletions(-)

diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
index fef922ff2635..7cc2360f1848 100644
--- a/arch/x86/kvm/i8259.c
+++ b/arch/x86/kvm/i8259.c
@@ -651,15 +651,10 @@ fail_unlock:
 	return NULL;
 }
 
-void kvm_destroy_pic(struct kvm *kvm)
+void kvm_destroy_pic(struct kvm_pic *vpic)
 {
-	struct kvm_pic *vpic = kvm->arch.vpic;
-
-	if (vpic) {
-		kvm_io_bus_unregister_dev(kvm, KVM_PIO_BUS, &vpic->dev_master);
-		kvm_io_bus_unregister_dev(kvm, KVM_PIO_BUS, &vpic->dev_slave);
-		kvm_io_bus_unregister_dev(kvm, KVM_PIO_BUS, &vpic->dev_eclr);
-		kvm->arch.vpic = NULL;
-		kfree(vpic);
-	}
+	kvm_io_bus_unregister_dev(vpic->kvm, KVM_PIO_BUS, &vpic->dev_master);
+	kvm_io_bus_unregister_dev(vpic->kvm, KVM_PIO_BUS, &vpic->dev_slave);
+	kvm_io_bus_unregister_dev(vpic->kvm, KVM_PIO_BUS, &vpic->dev_eclr);
+	kfree(vpic);
 }
diff --git a/arch/x86/kvm/irq.h b/arch/x86/kvm/irq.h
index ad68c73008c5..3d782a2c336a 100644
--- a/arch/x86/kvm/irq.h
+++ b/arch/x86/kvm/irq.h
@@ -74,7 +74,7 @@ struct kvm_pic {
 };
 
 struct kvm_pic *kvm_create_pic(struct kvm *kvm);
-void kvm_destroy_pic(struct kvm *kvm);
+void kvm_destroy_pic(struct kvm_pic *vpic);
 int kvm_pic_read_irq(struct kvm *kvm);
 void kvm_pic_update_irq(struct kvm_pic *s);
 
@@ -85,11 +85,11 @@ static inline struct kvm_pic *pic_irqchip(struct kvm *kvm)
 
 static inline int irqchip_in_kernel(struct kvm *kvm)
 {
-	int ret;
+	struct kvm_pic *vpic = pic_irqchip(kvm);
 
-	ret = (pic_irqchip(kvm) != NULL);
+	/* Read vpic before kvm->irq_routing.  */
 	smp_rmb();
-	return ret;
+	return vpic != NULL;
 }
 
 void kvm_pic_reset(struct kvm_kpic_state *s);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 2d62229aac26..23e47a0b054b 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3626,30 +3626,25 @@ long kvm_arch_vm_ioctl(struct file *filp,
 			r = kvm_ioapic_init(kvm);
 			if (r) {
 				mutex_lock(&kvm->slots_lock);
-				kvm_io_bus_unregister_dev(kvm, KVM_PIO_BUS,
-							  &vpic->dev_master);
-				kvm_io_bus_unregister_dev(kvm, KVM_PIO_BUS,
-							  &vpic->dev_slave);
-				kvm_io_bus_unregister_dev(kvm, KVM_PIO_BUS,
-							  &vpic->dev_eclr);
+				kvm_destroy_pic(vpic);
 				mutex_unlock(&kvm->slots_lock);
-				kfree(vpic);
 				goto create_irqchip_unlock;
 			}
 		} else
 			goto create_irqchip_unlock;
-		smp_wmb();
-		kvm->arch.vpic = vpic;
-		smp_wmb();
 		r = kvm_setup_default_irq_routing(kvm);
 		if (r) {
 			mutex_lock(&kvm->slots_lock);
 			mutex_lock(&kvm->irq_lock);
 			kvm_ioapic_destroy(kvm);
-			kvm_destroy_pic(kvm);
+			kvm_destroy_pic(vpic);
 			mutex_unlock(&kvm->irq_lock);
 			mutex_unlock(&kvm->slots_lock);
+			goto create_irqchip_unlock;
 		}
+		/* Write kvm->irq_routing before kvm->arch.vpic.  */
+		smp_wmb();
+		kvm->arch.vpic = vpic;
 	create_irqchip_unlock:
 		mutex_unlock(&kvm->lock);
 		break;
-- 
1.8.3.1

Re: [PATCH 4/4] KVM: x86: clean/fix memory barriers in irqchip_in_kernel

[Steve Rutherford]

On Wed, Jul 29, 2015 at 03:28:58PM +0200, Paolo Bonzini wrote:
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 2d62229aac26..23e47a0b054b 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3626,30 +3626,25 @@ long kvm_arch_vm_ioctl(struct file *filp,
>  			r = kvm_ioapic_init(kvm);
>  			if (r) {
>  				mutex_lock(&kvm->slots_lock);
> -				kvm_io_bus_unregister_dev(kvm, KVM_PIO_BUS,
> -							  &vpic->dev_master);
> -				kvm_io_bus_unregister_dev(kvm, KVM_PIO_BUS,
> -							  &vpic->dev_slave);
> -				kvm_io_bus_unregister_dev(kvm, KVM_PIO_BUS,
> -							  &vpic->dev_eclr);
> +				kvm_destroy_pic(vpic);
>  				mutex_unlock(&kvm->slots_lock);
> -				kfree(vpic);
>  				goto create_irqchip_unlock;
>  			}
>  		} else
>  			goto create_irqchip_unlock;
> -		smp_wmb();
> -		kvm->arch.vpic = vpic;
> -		smp_wmb();
>  		r = kvm_setup_default_irq_routing(kvm);
>  		if (r) {
>  			mutex_lock(&kvm->slots_lock);
>  			mutex_lock(&kvm->irq_lock);
>  			kvm_ioapic_destroy(kvm);
> -			kvm_destroy_pic(kvm);
> +			kvm_destroy_pic(vpic);
>  			mutex_unlock(&kvm->irq_lock);
>  			mutex_unlock(&kvm->slots_lock);
> +			goto create_irqchip_unlock;
>  		}
> +		/* Write kvm->irq_routing before kvm->arch.vpic.  */
> +		smp_wmb();
I assume this pairs with irqchip_in_kernel? 
> +		kvm->arch.vpic = vpic;
>  	create_irqchip_unlock:
>  		mutex_unlock(&kvm->lock);
>  		break;
> -- 
> 1.8.3.1
> 

Re: [PATCH 3/4] KVM: i8254: remove unnecessary irqchip_in_kernel check

[Steve Rutherford]

On Wed, Jul 29, 2015 at 03:28:57PM +0200, Paolo Bonzini wrote:
> The PIT is only created if irqchip_in_kernel returns true, so the
> check is superfluous.
I poked around. Looks to me like the existence of an IOAPIC is not
checked on the creation of the in-kernel PIT. Userspace might limit itself to
that scenario (PIT implies IOAPIC in-kernel), but that isn't enforced at PIT
creation.

It's worth adding that check in.

> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/kvm/i8254.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
> index f90952f64e79..f588eb7bdf45 100644
> --- a/arch/x86/kvm/i8254.c
> +++ b/arch/x86/kvm/i8254.c
> @@ -333,7 +333,7 @@ static void create_pit_timer(struct kvm *kvm, u32 val, int is_period)
>  	struct kvm_kpit_state *ps = &kvm->arch.vpit->pit_state;
>  	s64 interval;
>  
> -	if (!irqchip_in_kernel(kvm) || ps->flags & KVM_PIT_FLAGS_HPET_LEGACY)
> +	if (ps->flags & KVM_PIT_FLAGS_HPET_LEGACY)
>  		return;
>  
>  	interval = muldiv64(val, NSEC_PER_SEC, KVM_PIT_FREQ);
> -- 
> 1.8.3.1
> 
> 

Re: [PATCH 3/4] KVM: i8254: remove unnecessary irqchip_in_kernel check

[Paolo Bonzini]

On 30/07/2015 06:36, Steve Rutherford wrote:
> On Wed, Jul 29, 2015 at 03:28:57PM +0200, Paolo Bonzini wrote:
>> > The PIT is only created if irqchip_in_kernel returns true, so the
>> > check is superfluous.
> I poked around. Looks to me like the existence of an IOAPIC is not
> checked on the creation of the in-kernel PIT.

You're right, and presumably it's also legal to create the PIT before
KVM_CREATE_IRQCHIP.

> Userspace might limit itself to
> that scenario (PIT implies IOAPIC in-kernel), but that isn't enforced at PIT
> creation.

I'll play with the "PIT without IOAPIC" scenario and see if something
breaks badly with this patch.  From reading the code it seems like it
should not introduce any problems (oopses or similar), but I'll set this
patch aside for now.

Paolo

Re: [PATCH 4/4] KVM: x86: clean/fix memory barriers in irqchip_in_kernel

[Paolo Bonzini]

On 30/07/2015 05:32, Steve Rutherford wrote:
>> > +		/* Write kvm->irq_routing before kvm->arch.vpic.  */
>> > +		smp_wmb();
> I assume this pairs with irqchip_in_kernel? 

Yes, see the comment added there by this same patch ("read
kvm->arch.vpic before kvm->irq_routing").

Paolo

Re: [PATCH 2/4] KVM: document memory barriers for kvm->vcpus/kvm->online_vcpus

[Christian Borntraeger]

Am 29.07.2015 um 15:28 schrieb Paolo Bonzini:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  include/linux/kvm_host.h | 4 ++++
>  virt/kvm/kvm_main.c      | 2 ++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index bd1097a95704..81089cf1f0c1 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -427,6 +427,10 @@ struct kvm {
> 
>  static inline struct kvm_vcpu *kvm_get_vcpu(struct kvm *kvm, int i)
>  {
> +	/* Pairs with smp_wmb() in kvm_vm_ioctl_create_vcpu, in case
> +	 * the caller has read kvm->online_vcpus before (as is the case
> +	 * for kvm_for_each_vcpu, for example).
> +	 */
>  	smp_rmb();

Hmmm, wouldnt something like smp_mb__after_atomic

>  	return kvm->vcpus[i];
>  }
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 8dc4828f623f..093b3d10b411 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -2206,6 +2206,8 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, u32 id)
>  	}
> 
>  	kvm->vcpus[atomic_read(&kvm->online_vcpus)] = vcpu;

and  smp_mb__before_atomic

be the better function?


> +
> +	/* Pairs with smp_rmb() in kvm_get_vcpu.  */
>  	smp_wmb();
>  	atomic_inc(&kvm->online_vcpus);
> 

Re: [PATCH 2/4] KVM: document memory barriers for kvm->vcpus/kvm->online_vcpus

[Paolo Bonzini]

On 30/07/2015 13:40, Christian Borntraeger wrote:
>> > +	/* Pairs with smp_wmb() in kvm_vm_ioctl_create_vcpu, in case
>> > +	 * the caller has read kvm->online_vcpus before (as is the case
>> > +	 * for kvm_for_each_vcpu, for example).
>> > +	 */
>> >  	smp_rmb();
> Hmmm, wouldnt something like smp_mb__after_atomic
> 
>> >  	return kvm->vcpus[i];
>> >  }
>> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>> > index 8dc4828f623f..093b3d10b411 100644
>> > --- a/virt/kvm/kvm_main.c
>> > +++ b/virt/kvm/kvm_main.c
>> > @@ -2206,6 +2206,8 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, u32 id)
>> >  	}
>> > 
>> >  	kvm->vcpus[atomic_read(&kvm->online_vcpus)] = vcpu;
> and  smp_mb__before_atomic
> 
> be the better function?
> 
> 
>> > +
>> > +	/* Pairs with smp_rmb() in kvm_get_vcpu.  */
>> >  	smp_wmb();
>> >  	atomic_inc(&kvm->online_vcpus);

The difference between smp_rmb/smp_wmb on one side, and smp_mb on the
other side, is that smp_mb protects previous stores from subsequent loads.

smp_mb__before_atomic would be overkill for atomic_inc, where there's no
race involving the read of online_vcpus (there's only one writer and
it's protected by kvm->lock).  We only care about ordering the
kvm->vcpus store with the kvm->online_vcpus store.  One could use a
hypothetical smp_wmb__before_atomic(), but this is not a fast path and
IIRC only on ia64 it would be different from smp_wmb().

The "after_atomic" bit of smp_mb__after_atomic doesn't include
atomic_read/atomic_set, so it would not be good for kvm_get_vcpu.  But
there is also no previous store to order against, because
kvm->online_vcpus is only written in the KVM_CREATE_VCPU ioctl.  Thus
smp_rmb() is enough.

Paolo

Re: [PATCH 2/4] KVM: document memory barriers for kvm->vcpus/kvm->online_vcpus

[Christian Borntraeger]

Am 30.07.2015 um 14:46 schrieb Paolo Bonzini:
> 
> 
> On 30/07/2015 13:40, Christian Borntraeger wrote:
>>>> +	/* Pairs with smp_wmb() in kvm_vm_ioctl_create_vcpu, in case
>>>> +	 * the caller has read kvm->online_vcpus before (as is the case
>>>> +	 * for kvm_for_each_vcpu, for example).
>>>> +	 */
>>>>  	smp_rmb();
>> Hmmm, wouldnt something like smp_mb__after_atomic
>>
>>>>  	return kvm->vcpus[i];
>>>>  }
>>>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>>>> index 8dc4828f623f..093b3d10b411 100644
>>>> --- a/virt/kvm/kvm_main.c
>>>> +++ b/virt/kvm/kvm_main.c
>>>> @@ -2206,6 +2206,8 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, u32 id)
>>>>  	}
>>>>
>>>>  	kvm->vcpus[atomic_read(&kvm->online_vcpus)] = vcpu;
>> and  smp_mb__before_atomic
>>
>> be the better function?
>>
>>
>>>> +
>>>> +	/* Pairs with smp_rmb() in kvm_get_vcpu.  */
>>>>  	smp_wmb();
>>>>  	atomic_inc(&kvm->online_vcpus);
> 
> The difference between smp_rmb/smp_wmb on one side, and smp_mb on the
> other side, is that smp_mb protects previous stores from subsequent loads.
> 
> smp_mb__before_atomic would be overkill for atomic_inc, where there's no
> race involving the read of online_vcpus (there's only one writer and
> it's protected by kvm->lock).  We only care about ordering the
> kvm->vcpus store with the kvm->online_vcpus store. 

Ok, makes sense, if we only want to order these two stores.

But then the comment

>>>> +	/* Pairs with smp_wmb() in kvm_vm_ioctl_create_vcpu, in case
>>>> +	 * the caller has read kvm->online_vcpus before (as is the case
>>>> +	 * for kvm_for_each_vcpu, for example).
>>>> +	 */

is somewhat distracting because of "read" and "before". So something like

 /* Pairs with smp_wmb() in kvm_vm_ioctl_create_vcpu, to serialize the setting
of kvm->vcpus and setting kvm->online_vcpus....

might be better.

Re: [PATCH 2/4] KVM: document memory barriers for kvm->vcpus/kvm->online_vcpus

[Paolo Bonzini]

On 30/07/2015 15:57, Christian Borntraeger wrote:
>>>>> +	/* Pairs with smp_wmb() in kvm_vm_ioctl_create_vcpu, in case
>>>>> +	 * the caller has read kvm->online_vcpus before (as is the case
>>>>> +	 * for kvm_for_each_vcpu, for example).
>>>>> +	 */
> is somewhat distracting because of "read" and "before". So something like
> 
>  /* Pairs with smp_wmb() in kvm_vm_ioctl_create_vcpu, to serialize the setting
> of kvm->vcpus and setting kvm->online_vcpus....
> 
> might be better.

What you are suggesting would go in kvm_vm_ioctl_create_vcpu, e.g.
rewriting:

	/* Pairs with smp_rmb() in kvm_get_vcpu.  */
	smp_wmb();

like this:

	/*
	 * Pairs with smp_rmb() in kvm_get_vcpu.  Write kvm->vcpus
	 * before kvm->online_vcpu's incremented value.
	 */
	smp_wmb();

Instead, kvm_get_cpu has:

	/*
	 * Pairs with smp_wmb() in kvm_vm_ioctl_create_vcpu, in case
	 * the caller has read kvm->online_vcpus before (as is the case
	 * for kvm_for_each_vcpu, for example).
	 */
	smp_rmb();

which already describes which loads are serialized here.

Is this correct?

Paolo

Re: [PATCH 2/4] KVM: document memory barriers for kvm->vcpus/kvm->online_vcpus

[Christian Borntraeger]

Am 30.07.2015 um 16:02 schrieb Paolo Bonzini:
> 
> 
> On 30/07/2015 15:57, Christian Borntraeger wrote:
>>>>>> +	/* Pairs with smp_wmb() in kvm_vm_ioctl_create_vcpu, in case
>>>>>> +	 * the caller has read kvm->online_vcpus before (as is the case
>>>>>> +	 * for kvm_for_each_vcpu, for example).
>>>>>> +	 */
>> is somewhat distracting because of "read" and "before". So something like
>>
>>  /* Pairs with smp_wmb() in kvm_vm_ioctl_create_vcpu, to serialize the setting
>> of kvm->vcpus and setting kvm->online_vcpus....
>>
>> might be better.
> 
> What you are suggesting would go in kvm_vm_ioctl_create_vcpu, e.g.
> rewriting:
> 
> 	/* Pairs with smp_rmb() in kvm_get_vcpu.  */
> 	smp_wmb();
> 
> like this:
> 
> 	/*
> 	 * Pairs with smp_rmb() in kvm_get_vcpu.  Write kvm->vcpus
> 	 * before kvm->online_vcpu's incremented value.
> 	 */
> 	smp_wmb();
> 
> Instead, kvm_get_cpu has:
> 
> 	/*
> 	 * Pairs with smp_wmb() in kvm_vm_ioctl_create_vcpu, in case
> 	 * the caller has read kvm->online_vcpus before (as is the case
> 	 * for kvm_for_each_vcpu, for example).
> 	 */
> 	smp_rmb();
> 
> which already describes which loads are serialized here.
> 
> Is this correct?

Right you are.