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

[Markus Armbruster]

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

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

Facts:

* We have a half-initialized state that is visible to the guest.

* To detect the doorbell, you have to wait for the device to exit this
  state.

* Recognizing this state is cumbersome unless you already know you got a
  doorbell.

* Most guests most of the time should take longer to boot to the point
  where they look at the device than the device takes to exit this
  state.

This is a recipe for rare & obscure guest failure.  I doubt actual guest
software gets it right.  Moreover, I think it shouldn't have to get this
right!  It's a pointless trap for unwary guests.

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

We fix all kind of bugs, the ones that bite every time, and the ones
that can bite only when you're sufficiently unlucky.  Applies do design
bugs just as much as to coding bugs.

>>>> 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 :)

Sizable chunk of work, thank *you*!

f7a199b ivshmem: use little-endian int64_t for the protocol
660c97e ivshmem: use kvm irqfd for msi notifications
0f57350 ivshmem: rename MSI eventfd_table
d160f3f ivshmem: remove EventfdEntry.vector
d9453c9 ivshmem: add hostmem backend
2c04752 ivshmem: use qemu_strtosz()
f689d28 ivshmem: do not keep shm_fd open

 1 file changed, 285 insertions(+), 91 deletions(-)

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

As far as I can tell, this is a bug fix.  Before, we first report the
missing chardev, then the fail shm_open(), then exit().  Now, we fail it
right away, and the right way, i.e. not with exit().

> - the new memdev property (similarly to the new use64 in c08ba66)

I would like to take that out, yes.

If we must have it in 2.5, mark it experimental: x-memdev.  Anybody
who'd want that, please speak up.

Post-2.5, deprecate the existing device model for something more sane.
I'm thinking of a pair of device models, one with and one without the
doorbell.  Make sure they have a clean guest ABI, a clean set of
properties, and a reasonable split between frontend and backend.  If we
kept x-memdev, drop it from the deprecated device.

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

The fewer compatibility interfaces we maintain, and the simpler they
are, the better.  I don't see why we must complicate this one before we
deprecate it when we can it the other way round.