Re: [Qemu-devel] [PATCH v5 3/3] ivshmem: add check on protocol version in QEMU

[Stefan Hajnoczi]

On Fri, Sep 05, 2014 at 02:25:05PM +0200, David Marchand wrote:
> Hello Stefan,
> 
> On 09/05/2014 12:29 PM, Stefan Hajnoczi wrote:
> >On Thu, Sep 04, 2014 at 02:51:01PM +0200, David Marchand wrote:
> >>+                "server using a different protocol please check your 
> >>setup\n");
> >>+        qemu_chr_delete(s->server_chr);
> >>+        s->server_chr = NULL;
> >>+        return;
> >>+    }
> >>+
> >>+    IVSHMEM_DPRINTF("version check ok, finish init and switch to real 
> >>chardev "
> >>+                    "handler\n");
> >>+
> >>+    pci_register_bar(dev, 2, s->ivshmem_attr, &s->bar);
> >
> >Not sure if it is okay to delay PCI initialization to a fd hander
> >callback.
> >
> >If the version message is too slow the guest could see the PCI adapter
> >without the BAR!
> >
> >Did you move this code in order to prevent the guest from accessing the
> >device before it has connected to the server?  Perhaps the device needs
> >a state field that tracks whether or not it is ready for operation.  Any
> >access before RUNNING state is reached will be ignored (?).
> 
> Yes, exactly.
> 
> There already is a synchronisation mechanism described in the documentation:
> "When using the server, since the server is a separate process, the VM ID
> will only be set when the device is ready (shared memory is received from
> the server and accessible via the device).  If the device is not ready, the
> IVPosition will return -1.
> Applications should ensure that they have a valid VM ID before accessing the
> shared memory."
> 
> So actually, this move is unneeded if ivshmem users comply to this.
> 
> I will let the init stuff (pci_register_bar + gmalloc) where it was before,
> ivshmem_check_version will only switch the chardev handler.
> 
> What do you think about this ?

Sounds good.

pgpmYiijDfyiJ.pgp
Description: PGP signature