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

[Markus Armbruster]

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

> Hi
>
> ----- Original Message -----
>> >> Hash ivshmem been used in anger?  If yes, how?
>> 
>> Still the question to answer.
>
> I don't expect users to read this ML everyday (anybody
> actually). Personally, I have no clue how widespread ivshmem usage is.
>
>> 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.
>
> interesting, I guess the doc must be updated

One-liner.

>> 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.
>
> I haven't tried all combinations, to me it's not a bug if an argument
> is silently ignored, but we are missing a warning.

In general, configuration that doesn't make sense should be flat-out
rejected.

A warning is appropriate when we feel rejecting would break backward
compatibility.

Non-sensical option combinations can signify badly designed interfaces.
I believe this is the case here.  More on that below.

>> qemu-doc documents role only with chardev.  The code doesn't care.
>
> yeah, role is only really useful with a server. Another missing warning.

I think it makes sense only when we can migrate the shared memory
contents out-of-band.  Vaguely similar to block devices: either you
migrate them along (block migration), or you have to ensure they have
identical contents on the target in some other way.

Is the latter possible only with a server?

>> size makes sense only with shm and chardev.  If you specify it with
>> memdev, it's ignored with a warning.
>
> Ah! it's probably fine then?

For a definition of "fine"....

>> 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.
>
> yep
>
>> use64 sets PCI_BASE_ADDRESS_SPACE_MEMORY.  Not documented in qemu-doc.
>
> I don't think all properties are documented, are they? Gerd added this
> in c08ba66f with pretty good commit message that we could adapt to add
> in documentation.

Yes.

>> This is a byzantine mess even for QEMU standards.
>> 
>> Questions:
>> 
>> * Is it okay to have an unmapped BAR?
>
> I can't say much, though I remember I did some tests and it was ok.

Did you try chardev=...,size=S, where S is larger than what the server
provides?

A guest that fails there probably works for sufficiently small S only
when it loses the race with the server.

But my question is actually whether it's okay for a PCI device to
advertize a BAR, and then not map anything there.

Should realize wait for the server providing the shared memory?

>> * 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?
>
> No idea, but I agree it would make sense to have two different devices.
>
>>   It's not even obvious to me how the guest is supposed to figure out
>>   which kind it has.
>
> I don't think it can easily: I can imagine sending a doorbell to
> yourself, but that wouldn't be really reliable either.

Ugh!

>> * shm appears to be the same as memdev, just less flexible.  Why does it
>>   exist?
>
> It was there before.

Not only is memdev more flexible, it also provides the clean split
between frontend and backend we generally want.

>> * 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.
>
> It's supposed to be the same ABI as before, with the memdev addition.

Well, it's *crap*.  We shouldn't extend and known crap so it can get
used more widely, we should deprecate it in favour of something less
crappy.

Here's my attempt:

1. Split ivshmem into ivshmem-plain and ivshmem-doorbell.

   Each one gets its own PCI device ID, so that guests can trivially see
   whether the device got a doorbell.

   Both have property use64.

   ivshmem-plain additionally has property memdev.

   ivshmem-doorbell additionally has chardev, size, vectors, ioeventfd,
   msi.

   Undecided: property role (see above).

   The only non-sensical property combination is ioeventfd=on,msi=off,
   which we cleanly reject.

   Undecided: behavior before the server provides the shared memory, and
   what to do when it's less than size (see above).  This is the *only*
   part of my proposal that may require code changes beyond
   configuration.  If we can't do this before the release, punt
   ivshmem-doorbell to the next cycle.

2. Drop ivshmem property memdev.

   Use ivshmem-plain instead.

3. Deprecate ivshmem.