From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54792) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XPsZo-0008Gb-Ig for qemu-devel@nongnu.org; Fri, 05 Sep 2014 08:25:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XPsZi-0006Qe-Dl for qemu-devel@nongnu.org; Fri, 05 Sep 2014 08:25:16 -0400 Received: from mail-we0-f179.google.com ([74.125.82.179]:39963) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XPsZi-0006QQ-8G for qemu-devel@nongnu.org; Fri, 05 Sep 2014 08:25:10 -0400 Received: by mail-we0-f179.google.com with SMTP id t60so11626566wes.38 for ; Fri, 05 Sep 2014 05:25:09 -0700 (PDT) Message-ID: <5409ABA1.3070900@6wind.com> Date: Fri, 05 Sep 2014 14:25:05 +0200 From: David Marchand MIME-Version: 1.0 References: <1409835061-19989-1-git-send-email-david.marchand@6wind.com> <1409835061-19989-4-git-send-email-david.marchand@6wind.com> <20140905102908.GA27649@stefanha-thinkpad.redhat.com> In-Reply-To: <20140905102908.GA27649@stefanha-thinkpad.redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v5 3/3] ivshmem: add check on protocol version in QEMU List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: kvm@vger.kernel.org, claudio.fontana@huawei.com, qemu-devel@nongnu.org, armbru@redhat.com, arei.gonglei@huawei.com, mkletzan@redhat.com, pbonzini@redhat.com, jani.kokkonen@huawei.com, cam@cs.ualberta.ca 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: >> diff --git a/contrib/ivshmem-client/ivshmem-client.c b/contrib/ivshmem-client/ivshmem-client.c >> index ad210c8..0c4e016 100644 >> --- a/contrib/ivshmem-client/ivshmem-client.c >> +++ b/contrib/ivshmem-client/ivshmem-client.c >> @@ -184,10 +184,18 @@ ivshmem_client_connect(IvshmemClient *client) >> goto err_close; >> } >> >> - /* first, we expect our index + a fd == -1 */ >> + /* first, we expect a protocol version */ >> + if (read_one_msg(client, &tmp, &fd) < 0 || >> + (tmp != IVSHMEM_PROTOCOL_VERSION) || fd != -1) { >> + debug_log(client, "cannot read from server\n"); >> + goto err_close; >> + } >> + debug_log(client, "our_id=%ld\n", client->local.id); > > This debug_log() is probably not intentional. local.id will always be > -1 here so the output is not useful. Yes, this is most likely a merge/rebase issue. Will remove this. > >> +static void ivshmem_check_version(void *opaque, const uint8_t * buf, int flags) >> +{ >> + IVShmemState *s = opaque; >> + PCIDevice *dev = PCI_DEVICE(s); >> + int tmp; >> + long version; >> + >> + memcpy(&version, buf, sizeof(long)); >> + tmp = qemu_chr_fe_get_msgfd(s->server_chr); >> + if (tmp != -1 || version != IVSHMEM_PROTOCOL_VERSION) { >> + fprintf(stderr, "incompatible version, you are connecting to a ivhsmem-" Hum, typo: ivhs -> ivsh. >> + "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 ? Thanks. -- David Marchand