Re: [Xen-devel] [PATCH v2 3/9] xen: introduce the header file for the Xen 9pfs transport protocol

[Stefano Stabellini]

On Wed, 15 Mar 2017, Greg Kurz wrote:
> On Mon, 13 Mar 2017 16:55:54 -0700
> Stefano Stabellini  wrote:
> 
> > It uses the new ring.h macros to declare rings and interfaces.
> > 
> > Signed-off-by: Stefano Stabellini 
> > CC: anthony.per...@citrix.com
> > CC: jgr...@suse.com
> > ---
> >  hw/9pfs/xen_9pfs.h | 20 
> >  1 file changed, 20 insertions(+)
> >  create mode 100644 hw/9pfs/xen_9pfs.h
> > 
> 
> This header file has only one user: please move its content to
> hw/9pfs/xen-9p-backend.c (except the 9P header structure, see
> below).

OK, I can do that. There is going to be a very similar header in the Xen
tree under xen/include/public/io
(http://marc.info/?l=xen-devel=148952997709142), but from QEMU point
of view, it makes sense to fold it in xen-9p-backend.c.


> > diff --git a/hw/9pfs/xen_9pfs.h b/hw/9pfs/xen_9pfs.h
> > new file mode 100644
> > index 000..c4e8901
> > --- /dev/null
> > +++ b/hw/9pfs/xen_9pfs.h
> > @@ -0,0 +1,20 @@
> > +#ifndef XEN_9PFS_H
> > +#define XEN_9PFS_H
> > +
> > +#include "hw/xen/io/ring.h"
> > +#include 
> > +
> > +struct xen_9pfs_header {
> > +   uint32_t size;
> > +   uint8_t id;
> > +   uint16_t tag;
> > +
> > +   /* uint8_t sdata[]; */
> 
> This doesn't seem useful.

I'll remove


> > +} __attribute__((packed));
> > +
> 
> A few remarks:
> - this is a 9P message header actually, which is also used with virtio,
> - QEMU coding style requires a typedef in CamelCase,
> - the 9P protocol explicitely uses little-endian ordering. Since we
>   don't have endian-specific types, it makes sense to indicate that
>   when naming the fields.
> - we have a QEMU_PACKED macro which seems to be used a lot more than
>   the gcc syntax
> 
> Please define a new type in hw/9pfs/9p.h and use it in both transports.
> Something like:
> 
> typedef struct {
> uint32_t size_le;
> uint8_t id;
> uint16_t tag_le;
> } QEMU_PACKED P9MsgHeader;

OK


> > +#define PAGE_SHIFT XC_PAGE_SHIFT
> 
> I don't see any user for this in hw/9pfs/xen-9p-backend.c

PAGE_SHIFT is used by the macros below, but the original Xen headers
don't have the PAGE_SHIFT definition, so, for the sake of keeping the
two in sync, I didn't add it there.


> > +#define XEN_9PFS_RING_ORDER 6
> > +#define XEN_9PFS_RING_SIZE  XEN_FLEX_RING_SIZE(XEN_9PFS_RING_ORDER)
> > +DEFINE_XEN_FLEX_RING_AND_INTF(xen_9pfs);
> > +
> > +#endif

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 3/9] xen: introduce the header file for the Xen 9pfs transport protocol

[Greg Kurz]

On Mon, 13 Mar 2017 16:55:54 -0700
Stefano Stabellini  wrote:

> It uses the new ring.h macros to declare rings and interfaces.
> 
> Signed-off-by: Stefano Stabellini 
> CC: anthony.per...@citrix.com
> CC: jgr...@suse.com
> ---
>  hw/9pfs/xen_9pfs.h | 20 
>  1 file changed, 20 insertions(+)
>  create mode 100644 hw/9pfs/xen_9pfs.h
> 

This header file has only one user: please move its content to
hw/9pfs/xen-9p-backend.c (except the 9P header structure, see
below).

> diff --git a/hw/9pfs/xen_9pfs.h b/hw/9pfs/xen_9pfs.h
> new file mode 100644
> index 000..c4e8901
> --- /dev/null
> +++ b/hw/9pfs/xen_9pfs.h
> @@ -0,0 +1,20 @@
> +#ifndef XEN_9PFS_H
> +#define XEN_9PFS_H
> +
> +#include "hw/xen/io/ring.h"
> +#include 
> +
> +struct xen_9pfs_header {
> + uint32_t size;
> + uint8_t id;
> + uint16_t tag;
> +
> + /* uint8_t sdata[]; */

This doesn't seem useful.

> +} __attribute__((packed));
> +

A few remarks:
- this is a 9P message header actually, which is also used with virtio,
- QEMU coding style requires a typedef in CamelCase,
- the 9P protocol explicitely uses little-endian ordering. Since we
  don't have endian-specific types, it makes sense to indicate that
  when naming the fields.
- we have a QEMU_PACKED macro which seems to be used a lot more than
  the gcc syntax

Please define a new type in hw/9pfs/9p.h and use it in both transports.
Something like:

typedef struct {
uint32_t size_le;
uint8_t id;
uint16_t tag_le;
} QEMU_PACKED P9MsgHeader;

> +#define PAGE_SHIFT XC_PAGE_SHIFT

I don't see any user for this in hw/9pfs/xen-9p-backend.c

> +#define XEN_9PFS_RING_ORDER 6
> +#define XEN_9PFS_RING_SIZE  XEN_FLEX_RING_SIZE(XEN_9PFS_RING_ORDER)
> +DEFINE_XEN_FLEX_RING_AND_INTF(xen_9pfs);
> +
> +#endif



pgp4plURek8V4.pgp
Description: OpenPGP digital signature
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [PATCH v2 3/9] xen: introduce the header file for the Xen 9pfs transport protocol

[Stefano Stabellini]

It uses the new ring.h macros to declare rings and interfaces.

Signed-off-by: Stefano Stabellini 
CC: anthony.per...@citrix.com
CC: jgr...@suse.com
---
 hw/9pfs/xen_9pfs.h | 20 
 1 file changed, 20 insertions(+)
 create mode 100644 hw/9pfs/xen_9pfs.h

diff --git a/hw/9pfs/xen_9pfs.h b/hw/9pfs/xen_9pfs.h
new file mode 100644
index 000..c4e8901
--- /dev/null
+++ b/hw/9pfs/xen_9pfs.h
@@ -0,0 +1,20 @@
+#ifndef XEN_9PFS_H
+#define XEN_9PFS_H
+
+#include "hw/xen/io/ring.h"
+#include 
+
+struct xen_9pfs_header {
+   uint32_t size;
+   uint8_t id;
+   uint16_t tag;
+
+   /* uint8_t sdata[]; */
+} __attribute__((packed));
+
+#define PAGE_SHIFT XC_PAGE_SHIFT
+#define XEN_9PFS_RING_ORDER 6
+#define XEN_9PFS_RING_SIZE  XEN_FLEX_RING_SIZE(XEN_9PFS_RING_ORDER)
+DEFINE_XEN_FLEX_RING_AND_INTF(xen_9pfs);
+
+#endif
-- 
1.9.1


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel