Re: [Qemu-devel] [PATCH 3/8] xen/9pfs: introduce Xen 9pfs backend

[Stefano Stabellini]

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.per...@citrix.com
> > > CC: jgr...@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 000..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':
> 
>   LINKcris-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 

Re: [Qemu-devel] [PATCH 3/8] xen/9pfs: introduce Xen 9pfs backend

[Greg Kurz]

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.per...@citrix.com
> > CC: jgr...@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 000..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 

Hmm... there's still a problem actually. This patch breaks build for some
targets with '--enable-xen --enable-virtfs':

  LINKcris-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'

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 

Re: [Qemu-devel] [PATCH 3/8] xen/9pfs: introduce Xen 9pfs backend

[Greg Kurz]

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.per...@citrix.com
> CC: jgr...@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 000..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 


pgpYOgOFh7kYZ.pgp
Description: OpenPGP digital signature

[Qemu-devel] [PATCH 3/8] xen/9pfs: introduce Xen 9pfs backend

[Stefano Stabellini]

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.per...@citrix.com
CC: jgr...@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 000..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 */
+};
+
+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),
+.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,
+};
-- 
1.9.1