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

[Markus Armbruster]

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

> ----- Original Message -----
>> Eric Blake <address@hidden> writes:
>> 
>> > On 11/20/2015 09:23 AM, Marc-André Lureau wrote:
>> >> Hi
>> >> 
>> >> ----- Original Message -----
>> >>> Everybody's favourite device model has "size" property.  It's declared
>> >>> as *string*
>> >>>
>> >>>     DEFINE_PROP_STRING("size", IVShmemState, sizearg),
>> >>>
>> >>>
>> >>> * In QMP, the size must be given as JSON string instead of JSON number,
>> >>>   and size suffixes are accepted.  Example: "size": "512k" instead of
>> >>>   "size": 524288.
>> >>>
>> >>>   Right now, this violation of QMP rules is tolerable (barely), because
>> >>>   device_add breaks some of these rules already.  However, one goal of
>> >>>   the current work on QAPI is to support a QMP command to plug devices
>> >>>   that doesn't break QMP rules, and then this violation will stand out.
>> >>>
>> >>>   Therefore, I want it fixed now, before ivshmem gets used in anger.
>> >
>> > Was ivshmem even usable in 2.4?  I'd argue that if we are going to
>> > change it, changing it for 2.5 is the ideal time.
>> 
>> Technically, we already have a compatibility break:
>> 
>> $ qemu-system-x86_64 -nodefaults -S -display none -chardev
>> socket,path=/work/armbru/images/ivshmem-socket,id=ivshmem-chr -device
>> ivshmem,size=4,chardev=ivshmem-chr,shm=foo
>> 
>> In 2.3.0, this ignores shm with warning "do not specify both 'chardev'
>> and 'shm' with ivshmem".
>> 
>> In current upstream, it's rejected.
>
> That's a regression, either we should:
> - keep accepting it
> - or keep rejection and remove the "WARNING:" 
>
> Imho in this case, it was an undefined behaviour, warned, so it's fine
> to reject it now.

It's exactly fine if...

>> I suspect we'd find more if we cared to look.
>> 
>> Of course, the only thing that *really* matters is *actual* usage in
>> anger: something of value that breaks.

... it doesn't break something of value.

>> Hash ivshmem been used in anger?  If yes, how?

Still the question to answer.


I had another look at ivshmem's properties, and I can't say I liked the
sight.

Besides the usual PCI properties, we have:

    ivshmem.chardev=str (ID of a chardev to use as a backend)
    ivshmem.size=str
    ivshmem.vectors=uint32
    ivshmem.ioeventfd=bool (on/off)
    ivshmem.msi=bool (on/off)
    ivshmem.shm=str
    ivshmem.role=str
    ivshmem.memdev=link<memory-backend>
    ivshmem.use64=uint32

Exactly one of chardev, shm, memdev must be given.  qemu-doc.info claims
you can omit all three, but that's a bug.

vectors, ioeventfd and msi make sense only with chardev.  The code
appears to silently ignore them unless chardev is given.  Except it
still rejects ioeventfd=on,msi=off.  I wouldn't bet on nonsensical
combinations to actually work.

qemu-doc documents role only with chardev.  The code doesn't care.

size makes sense only with shm and chardev.  If you specify it with
memdev, it's ignored with a warning.

With shm, we first try to create the shared memory object with the
specified size, and if that fails, we try to open it.  The specified
size must not be larger than the shared memory object then.

With chardev, we receive the shared memory object from the server.
Until we get it, BAR isn't mapped.  If the specified size is larger, the
BAR remains unmapped.

use64 sets PCI_BASE_ADDRESS_SPACE_MEMORY.  Not documented in qemu-doc.

This is a byzantine mess even for QEMU standards.

Questions:

* Is it okay to have an unmapped BAR?

* We actually have two distinct device kinds:

  - The basic device: shm with parameter size, or memdev

  - The doorbell-capable device: chardev with parameters size, vectors,
    ioeventfd=on, msi

  Common parameters: use64, role, although the latter is only documented
  for the doorbell-capable device.

  Why is this a single device model?

  It's not even obvious to me how the guest is supposed to figure out
  which kind it has.

* shm appears to be the same as memdev, just less flexible.  Why does it
  exist?

* Are we *sure* this is ready to become ABI?  I doubt it's ABI yet,
  because before Marc-André's massive rework, it was even worse (*much*
  worse), to a degree that makes me doubt anybody could use it
  seriously.