On 07/07/2017 13:49, Christian Borntraeger wrote:
> 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?
Comparing it to
const char *s[10];
it should mark each element of the array.
If the array was dynamically allocated, it would have been
struct kvm_io_bus *__rcu *buses;
> 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));
> +}
Yes, this is correct. My example above is wrong.
Paolo
