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

[Marc-André Lureau]

Hi

----- Original Message -----
>
> >> 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?

The way ivshmem role works is:
- master: will handle migration
- peer: can't migrate

It's not out-of-band, qemu handles the shared memory migration, even if
the memory is owned by the server. In practice, it means you can only
migrate one VM, or you migrate them all but you will migrate the shared
memory N times and will have to synchronize in your guest app. I suppose ivshmem
"role" was designed to only migrate the master. Ability to migrate a pool of
peer would be a significant new feature. I am not aware of such request.

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

It will fall in check_shm_size().

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

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

I think you overstate this, if it would be so bad I don't think anyone could use
it and it wouldn't probably be in qemu. We are mainly talking misconfiguration
and missing features (that people may not actually need). I think
before we split things, we can address your comments with more
options checking and documentation.

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

It sounds like a reasonable thing to do, but I don't think the benefit is so 
important.

cheers