Marc-André Lureau <mlur...@redhat.com> 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.
