On 07/07/2017 01:19 PM, Paolo Bonzini wrote:
>
>
> On 07/07/2017 12:58, Christian Borntraeger wrote:
>> mark kvm->busses as rcu protected and use the correct access
>> function everywhere.
>>
>> found by sparse
>> virt/kvm/kvm_main.c:3490:15: error: incompatible types in comparison expression (different address spaces)
>> virt/kvm/kvm_main.c:3509:15: error: incompatible types in comparison expression (different address spaces)
>> virt/kvm/kvm_main.c:3561:15: error: incompatible types in comparison expression (different address spaces)
>> virt/kvm/kvm_main.c:3644:15: error: incompatible types in comparison expression (different address spaces)
>>
>> Signed-off-by: Christian Borntraeger <borntraeger@xxxxxxxxxx>
>> ---
>> include/linux/kvm_host.h | 2 +-
>> virt/kvm/eventfd.c | 8 +++++---
>> virt/kvm/kvm_main.c | 17 ++++++++++-------
>> 3 files changed, 16 insertions(+), 11 deletions(-)
>>
>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
>> index 7c32905..220f251 100644
>> --- a/include/linux/kvm_host.h
>> +++ b/include/linux/kvm_host.h
>> @@ -404,7 +404,7 @@ struct kvm {
>> int last_boosted_vcpu;
>> struct list_head vm_list;
>> struct mutex lock;
>> - struct kvm_io_bus *buses[KVM_NR_BUSES];
>> + struct kvm_io_bus __rcu *buses[KVM_NR_BUSES];
>> #ifdef CONFIG_HAVE_KVM_EVENTFD
>> struct {
>> spinlock_t lock;
>> diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
>> index a8d5403..3e08b43 100644
>> --- a/virt/kvm/eventfd.c
>> +++ b/virt/kvm/eventfd.c
>> @@ -825,7 +825,7 @@ static int kvm_assign_ioeventfd_idx(struct kvm *kvm,
>> if (ret < 0)
>> goto unlock_fail;
>>
>> - kvm->buses[bus_idx]->ioeventfd_count++;
>> + rcu_dereference_protected(kvm->buses[bus_idx], 1)->ioeventfd_count++;
>
> Whenever possible, we should use (s)rcu_dereference_check and a better
> predicate than "1". In this case:
>
> static inline struct kvm_io_bus *kvm_get_bus(struct kvm *kvm, int i)
> {
> struct kvm_io_bus **buses =
> srcu_dereference_check(kvm->buses, &kvm->srcu,
> lockdep_is_held(&kvm->slots_lock));
> return buses[i];
> }
Hmm, now this makes me wonder.
does this
struct kvm_io_bus __rcu *buses[KVM_NR_BUSES];
mark
**buses
as rcu protected or does it mark each element of the array
iow
*buses[0]
*buses[1]
as rcu protected?
My code certainly assumes that each single point is protected,
So I would have done something like this on top
diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index 3e08b43..91210ee 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -779,6 +779,12 @@ static enum kvm_bus ioeventfd_bus_from_flags(__u32 flags)
return KVM_MMIO_BUS;
}
+static inline struct kvm_io_bus *kvm_get_bus(struct kvm *kvm, int i)
+{
+ return srcu_dereference_check(kvm->buses[i], &kvm->srcu,
+ lockdep_is_held(&kvm->slots_lock));
+}
+
static int kvm_assign_ioeventfd_idx(struct kvm *kvm,
enum kvm_bus bus_idx,
struct kvm_ioeventfd *args)
@@ -825,7 +831,7 @@ static int kvm_assign_ioeventfd_idx(struct kvm *kvm,
if (ret < 0)
goto unlock_fail;
- rcu_dereference_protected(kvm->buses[bus_idx], 1)->ioeventfd_count++;
+ kvm_get_bus(kvm, bus_idx)->ioeventfd_count++;
list_add_tail(&p->list, &kvm->ioeventfds);
mutex_unlock(&kvm->slots_lock);
> (since you are at it, __kvm_memslots should also use srcu_dereference_check
> instead of rcu_dereference_check!).
Yes, will do a fixup for that patch.
