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

[Marc-André Lureau]

Hi

On Tue, Nov 24, 2015 at 10:56 AM, Markus Armbruster <address@hidden> wrote:
> 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 don't follow you. The point is that you can define your protocol in
the shared memory. Tbh, I think in practice users simply have a kind
of manager to start things, eventually waiting for doorbell to start
coming etc. Don't take my simple example for "the solution", I just
though about one, there might be plenty others.

> 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.

Apparently not for the people using it, or they would certainly fix it
or report bugs. Yes, it could be better, but it's really not that
"horrible" imho.

>
>>> 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.

Thank you :)

>>> 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.

What you call interface change is?:
- the error return in case chardev is provided with shm (this was
initially changed in baefb8bf8 before I unified behaviour)
- the new memdev property (similarly to the new use64 in c08ba66)

Imho it's not such a big deal to have a compatibility interface with
2.5 in the future, and I would rather keep the consistant behaviour
for the error return case.

-- 
Marc-André Lureau