[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 3/9] xen: introduce the header file for the X
From: |
Stefano Stabellini |
Subject: |
Re: [Qemu-devel] [PATCH v2 3/9] xen: introduce the header file for the Xen 9pfs transport protocol |
Date: |
Wed, 15 Mar 2017 12:02:18 -0700 (PDT) |
User-agent: |
Alpine 2.10 (DEB 1266 2009-07-14) |
On Wed, 15 Mar 2017, Greg Kurz wrote:
> On Mon, 13 Mar 2017 16:55:54 -0700
> Stefano Stabellini <address@hidden> wrote:
>
> > It uses the new ring.h macros to declare rings and interfaces.
> >
> > Signed-off-by: Stefano Stabellini <address@hidden>
> > CC: address@hidden
> > CC: address@hidden
> > ---
> > 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&m=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 0000000..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 <xen/io/protocols.h>
> > +
> > +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
[Qemu-devel] [PATCH v2 4/9] xen/9pfs: introduce Xen 9pfs backend, Stefano Stabellini, 2017/03/13
[Qemu-devel] [PATCH v2 5/9] xen/9pfs: connect to the frontend, Stefano Stabellini, 2017/03/13