From patchwork Mon Mar 13 23:26:28 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Stefano Stabellini X-Patchwork-Id: 738474 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3vhvCZ2Ph8z9s75 for ; Tue, 14 Mar 2017 10:27:05 +1100 (AEDT) Received: from localhost ([::1]:54841 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cnZMk-0003Dn-9F for incoming@patchwork.ozlabs.org; Mon, 13 Mar 2017 19:27:02 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38050) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cnZMK-0003Dh-Pr for qemu-devel@nongnu.org; Mon, 13 Mar 2017 19:26:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1cnZMH-0003rW-K5 for qemu-devel@nongnu.org; Mon, 13 Mar 2017 19:26:36 -0400 Received: from mail.kernel.org ([198.145.29.136]:43216) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1cnZMH-0003py-AY for qemu-devel@nongnu.org; Mon, 13 Mar 2017 19:26:33 -0400 Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 8708A2021F; Mon, 13 Mar 2017 23:26:30 +0000 (UTC) Received: from [10.1.10.194] (96-82-76-110-static.hfc.comcastbusiness.net [96.82.76.110]) (using TLSv1 with cipher ECDHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id BA27220165; Mon, 13 Mar 2017 23:26:28 +0000 (UTC) Date: Mon, 13 Mar 2017 16:26:28 -0700 (PDT) From: Stefano Stabellini X-X-Sender: sstabellini@sstabellini-ThinkPad-X260 To: Greg Kurz In-Reply-To: <20170310104828.78604daf@bahia> Message-ID: References: <1488852768-8935-1-git-send-email-sstabellini@kernel.org> <1488852768-8935-3-git-send-email-sstabellini@kernel.org> <20170309185426.3506a17b@bahia> <20170310104828.78604daf@bahia> User-Agent: Alpine 2.10 (DEB 1266 2009-07-14) MIME-Version: 1.0 X-Virus-Scanned: ClamAV using ClamSMTP X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x [fuzzy] X-Received-From: 198.145.29.136 Subject: Re: [Qemu-devel] [PATCH 3/8] xen/9pfs: introduce Xen 9pfs backend X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: jgross@suse.com, Stefano Stabellini , qemu-devel@nongnu.org, Stefano Stabellini , "Aneesh Kumar K.V" , anthony.perard@citrix.com, xen-devel@lists.xenproject.org Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: "Qemu-devel" On Fri, 10 Mar 2017, Greg Kurz wrote: > On Thu, 9 Mar 2017 18:54:26 +0100 > Greg Kurz wrote: > > > On Mon, 6 Mar 2017 18:12:43 -0800 > > Stefano Stabellini wrote: > > > > > Introduce the Xen 9pfs backend: add struct XenDevOps to register as a > > > Xen backend and add struct V9fsTransport to register as v9fs transport. > > > > > > All functions are empty stubs for now. > > > > > > Signed-off-by: Stefano Stabellini > > > CC: anthony.perard@citrix.com > > > CC: jgross@suse.com > > > CC: Aneesh Kumar K.V > > > CC: Greg Kurz > > > --- > > > hw/9pfs/xen-9p-backend.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 96 insertions(+) > > > create mode 100644 hw/9pfs/xen-9p-backend.c > > > > > > diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c > > > new file mode 100644 > > > index 0000000..924fb64 > > > --- /dev/null > > > +++ b/hw/9pfs/xen-9p-backend.c > > > @@ -0,0 +1,96 @@ > > > +/* > > > + * Xen 9p backend > > > + * > > > + * Copyright Aporeto 2017 > > > + * > > > + * Authors: > > > + * Stefano Stabellini > > > + * > > > + */ > > > + > > > +#include "qemu/osdep.h" > > > + > > > +#include "hw/hw.h" > > > +#include "hw/9pfs/9p.h" > > > +#include "hw/xen/xen_backend.h" > > > +#include "xen_9pfs.h" > > > +#include "qemu/config-file.h" > > > +#include "fsdev/qemu-fsdev.h" > > > + > > > +struct Xen9pfsDev { > > > + struct XenDevice xendev; /* must be first */ > > > +}; > > > > According to HACKING, this should be: > > > > typedef struct Xen9pfsDev { > > struct XenDevice xendev; /* must be first */ > > } Xen9pfsDev; > > > > and... > > > > > + > > > +static ssize_t xen_9pfs_pdu_vmarshal(V9fsPDU *pdu, > > > + size_t offset, > > > + const char *fmt, > > > + va_list ap) > > > +{ > > > + return 0; > > > +} > > > + > > > +static ssize_t xen_9pfs_pdu_vunmarshal(V9fsPDU *pdu, > > > + size_t offset, > > > + const char *fmt, > > > + va_list ap) > > > +{ > > > + return 0; > > > +} > > > + > > > +static void xen_9pfs_init_out_iov_from_pdu(V9fsPDU *pdu, > > > + struct iovec **piov, > > > + unsigned int *pniov) > > > +{ > > > +} > > > + > > > +static void xen_9pfs_init_in_iov_from_pdu(V9fsPDU *pdu, > > > + struct iovec **piov, > > > + unsigned int *pniov, > > > + size_t size) > > > +{ > > > +} > > > + > > > +static void xen_9pfs_push_and_notify(V9fsPDU *pdu) > > > +{ > > > +} > > > + > > > +static const struct V9fsTransport xen_9p_transport = { > > > + .pdu_vmarshal = xen_9pfs_pdu_vmarshal, > > > + .pdu_vunmarshal = xen_9pfs_pdu_vunmarshal, > > > + .init_in_iov_from_pdu = xen_9pfs_init_in_iov_from_pdu, > > > + .init_out_iov_from_pdu = xen_9pfs_init_out_iov_from_pdu, > > > + .push_and_notify = xen_9pfs_push_and_notify, > > > +}; > > > + > > > +static int xen_9pfs_init(struct XenDevice *xendev) > > > +{ > > > + return 0; > > > +} > > > + > > > +static int xen_9pfs_free(struct XenDevice *xendev) > > > +{ > > > + return -1; > > > +} > > > + > > > +static int xen_9pfs_connect(struct XenDevice *xendev) > > > +{ > > > + return 0; > > > +} > > > + > > > +static void xen_9pfs_alloc(struct XenDevice *xendev) > > > +{ > > > +} > > > + > > > +static void xen_9pfs_disconnect(struct XenDevice *xendev) > > > +{ > > > +} > > > + > > > +struct XenDevOps xen_9pfs_ops = { > > > + .size = sizeof(struct Xen9pfsDev), > > > > ... s/struct // > > > > > + .flags = DEVOPS_FLAG_NEED_GNTDEV, > > > + .alloc = xen_9pfs_alloc, > > > + .init = xen_9pfs_init, > > > + .initialise = xen_9pfs_connect, > > > + .disconnect = xen_9pfs_disconnect, > > > + .free = xen_9pfs_free, > > > +}; > > > > With the above comments addressed: > > > > Reviewed-by: Greg Kurz Thank you for the quick review and useful comments. I'll address them in the next version of the series. > Hmm... there's still a problem actually. This patch breaks build for some > targets with '--enable-xen --enable-virtfs': > > LINK cris-softmmu/qemu-system-cris > ../hw/xen/xen_backend.o: In function `xen_be_register_common': > /home/greg/Work/qemu/qemu-9p/hw/xen/xen_backend.c:588: undefined reference to `xen_9pfs_ops' I could reproduce the problem > This happens because only targets that support virtio and PCI pull the > 9pfs/fsdev object files. This is an effort to have smaller QEMU binaries, > based on the historical dependency of 9pfs on virtio. This patch breaks > this assumption if CONFIG_XEN_BACKEND is defined but the target doesn't > define CONFIG_VIRTIO and CONFIG_PCI. > > Since 9pfs can now be used with another transport, I guess that the > dependency on virtio and PCI isn't right anymore. The new condition > for targets to support 9pfs should rather be something like: > > CONFIG_VIRTFS == y && CONFIG_SOFTMMU == y && (CONFIG_VIRTIO == y || CONFIG_XEN_BACKEND == y) > > This would cause all targets to pull the 9pfs/fsdev object files though. > > So I have a question: do all targets need to support the Xen backend, really ? Not really: Xen is only supported on x86 and ARM platforms today. Typically, Xen is only enabled in QEMU for i386 or x86_64 softmmu targets, even when used in an ARM context (long story short: the Xen PV machine is arch-agnostic, but the QEMU build doesn't support that, so we tied it to x86 just because we had to pick one). In fact, I think the issue is that CONFIG_XEN_BACKEND is set wrongly by the configure script. It's not an host property, but a target property like CONFIG_XEN. We need: With this change, and the other change you suggested: common-obj-$(CONFIG_XEN_BACKEND) += xen-9p-backend.o I couldn't see any build issues. diff --git a/configure b/configure index 6c21975..6d8f752 100755 --- a/configure +++ b/configure @@ -5442,7 +5442,6 @@ if test "$virglrenderer" = "yes" ; then echo "VIRGL_LIBS=$virgl_libs" >> $config_host_mak fi if test "$xen" = "yes" ; then - echo "CONFIG_XEN_BACKEND=y" >> $config_host_mak echo "CONFIG_XEN_CTRL_INTERFACE_VERSION=$xen_ctrl_version" >> $config_host_mak if test "$xen_pv_domain_build" = "yes" ; then echo "CONFIG_XEN_PV_DOMAIN_BUILD=y" >> $config_host_mak @@ -6028,6 +6027,7 @@ case "$target_name" in i386|x86_64) if test "$xen" = "yes" -a "$target_softmmu" = "yes" ; then echo "CONFIG_XEN=y" >> $config_target_mak + echo "CONFIG_XEN_BACKEND=y" >> $config_target_mak if test "$xen_pci_passthrough" = yes; then echo "CONFIG_XEN_PCI_PASSTHROUGH=y" >> "$config_target_mak" fi