Re: [Qemu-devel] ivshmem property size should be a size, not a string

[Markus Armbruster]

Marc-André Lureau <address@hidden> writes:

> Hi
>
> ----- Original Message -----
>> On 11/23/2015 07:46 AM, Markus Armbruster wrote:
>> 
>> >>> If it's not broken, please explain to me how the guest should find out
>> >>> whether its ivshmem device sports a doorbell.
>> >>
>> >> If you have received ID, you should be good to use the doorbell.
>> > 
>> > That's not a complete answer, so let me try a different tack.
>> > 
>> > What value will a read of register IVPosition yield
>> > 
>> > * if the device has no doorbell:
>> > 
>> >   I think the answer is -1.
>> > 
>> > * if the device has a doorbell, but it isn't ready, yet:
>> > 
>> >   I think the answer is -1.
>> > 
>> > * if the device has a doorbell, and it is ready.
>> > 
>> >   This is the part you answered already: >= 0.
>> > 
>> > Please correct mistakes.
>> 
>> For what it's worth, I'm agreeing with Markus here - we have a race in
>> the guest learning whether doorbell is supported, and it's better to do
>
> I think there is no race if you communicate this information in the
> shared memory.

The device makes robust detection of the doorbell unnecessarily hard.
You may be able to work around it at higher levels.  This involves
assumptions on peers.  However, that's no excuse not to fix the device.
Regardless, the mechanism you propose, namely signalling presence of the
doorbell in the shared memory, is useless.

Let me elaborate.

Say you know that ivshmem is used for a specific protocol, and that
protocol requires a doorbell.  You simply assume the doorbell is there,
and read of IVPosition yielding -1 means the device hasn't completed its
setup, yet.

Since the shared memory only becomes mapped when IVPosition goes
non-negative, a guest device driver will probably want to delay
completion of the operation to map the shared memory into user space
until IVPosition becomes non-negative, to shield user space from SIGBUS
(or however else the kernel reports the trap to user space) during the
setup window.  Happy polling.

Say you know that ivshmem is used for a specific protocol, and that
protocol signals its use of a doorbell in the shared memory.  If a read
of IVPosition yields a non-negative number, you know you got a doorbell.
If it yields -1, you know nothing.  You can attempt reading shared
memory to find out more.  This *traps* when the BAR hasn't been mapped,
yet.  If it does, you still don't know.  If it doesn't, and IVPosition
is still -1, you got no doorbell, regardless of what the shared memory
tells you.  If IVPosition has meanwhile become non-negative, you got a
doorbell, again regardless of what the shared memory tells you.  Bottom
line: the protocol signalling its use of the doorbell in the shared
memory is of no help.

I figure a robust guest device driver is possible, but it'll involve
dealing with traps and polling IVPosition.

This device is simply an embarrassment.

>> a clean break in API for 2.5 along with ALL the other fixes into using a
>> sane union of types (splitting between ivshmem-plain and
>> ivshmem-doorbell).  If you absolutely want it, you can still maintain
>> 'ivshmem' as a front-end that maps to either ivshmem-plain,
>> ivshmem-doorbell, or an error (nonsensical combination of requests), but
>
> It's not about me. Until now I was not aware of anyone complaining about
> the ivshmem interface, but by its implementation. I tried to adress all the
> issues and comments I have found, and I tried to make sure not to break stuff,
> because I usually receive huge complains whenever I do, and I have to throw
> up the work. So if there is a consensus to break things now, I think it's
> quite late for this cycle, but I am for it.

The remaining flaws in ivshmem are certainly not your fault.

>> having a sane interface as your starting point, rather than an
>> amalgamation of cruft that exists only due to the early implementation,
>> seems like the way to go.
>
> It is just the way it was, isn't it?
>
>> I'm still waiting for any evidence that we even care about backwards
>> compatibility - what apps are out there that are actively using ivshmem
>> with its horrible pre-2.5 interface, and why can they not be fixed to
>
>> use the sane 2.5 interface?  Libvirt is NOT exposing ivshmem yet, in
>> part because the 2.4 interface was not very robust.  We already have a
>
> Afaik it's there since 1.2.10
>
>> clean break now due to all your work for 2.5; let's take advantage of
>> it, and make 2.5 robust, rather than breaking things again in 2.6.
>
> 2.5 should not break the ivshmem interface.

I don't object to keeping the existing ivshmem interface, flaws, warts
and all, to support existing users.

I do object to extending it.  I agree with Eric that 2.5 is the ideal
time for a clean break.  But if we can't pull it off for 2.5, then we
should punt *all* interface changes to 2.6.  Having to support one
legacy interface version in addition to the current one is clearly
better than having to support two of them.