Re: [Qemu-devel] [RFC] vhost: Can we change synchronize_rcu to call_rcu in vhost_set_memory() in vhost kernel module?

[Paolo Bonzini <pbonzini@redhat.com>]

Il 12/05/2014 13:07, Michael S. Tsirkin ha scritto:
> On Mon, May 12, 2014 at 12:25:35PM +0200, Paolo Bonzini wrote:
>> Il 12/05/2014 12:18, Michael S. Tsirkin ha scritto:
>>> On Mon, May 12, 2014 at 12:14:25PM +0200, Paolo Bonzini wrote:
>>>> Il 12/05/2014 12:08, Michael S. Tsirkin ha scritto:
>>>>> On Mon, May 12, 2014 at 11:57:32AM +0200, Paolo Bonzini wrote:
>>>>>> Perhaps we can check for cases where only the address is changing,
>>>>>> and poke at an existing struct kvm_kernel_irq_routing_entry without
>>>>>> doing any RCU synchronization?
>>>>>
>>>>> I suspect interrupts can get lost then: e.g. if address didn't match any
>>>>> cpus, now it matches some. No?
>>>>
>>>> Can you explain the problem more verbosely? :)
>>>>
>>>> Multiple writers would still be protected by the mutex, so you
>>>> cannot have an "in-place update" writer racing with a "copy the
>>>> array" writer.
>>>
>>> I am not sure really.
>>> I'm worried about reader vs writer.
>>> If reader sees a stale msi value msi will be sent to a wrong
>>> address.
>>
>> That shouldn't happen on any cache-coherent system, no?
>>
>> Or at least, it shouldn't become any worse than what can already
>> happen with RCU.
>
> Meaning guest must do some synchronization anyway?

Yes, I think so.  The simplest would be to mask the interrupt around MSI 
configuration changes.  Radim was looking at a similar bug.  He couldn't 
replicate on bare metal, but I don't see why it shouldn't be possible 
there too.

> But I am not sure this works correctly in all cases,
> synchronization with guest VCPU does not have to be
> the same thing as synchronization with host CPU.
> For example, can not guest VCPU1 detect that
> VCPU2 is idle and avoid any synchronization?

So guest VCPU1 would be the one that sets the MSI word, and VCPU2 would 
be the old destination?  That would also be racy, the moment after you 
"check that VCPU2 is idle" an interrupt could come in and the VCPU 
wouldn't be idle anymore.

> In any case I'd like to see a patch like this
> accompanied by some argument explaining why it's
> a safe thing to do.

Yes, of course.

Paolo