On Wed, 15 Mar 2017, Greg Kurz wrote: > On Mon, 13 Mar 2017 16:55:54 -0700 > Stefano Stabellini <sstabell...@kernel.org> wrote: > > > It uses the new ring.h macros to declare rings and interfaces. > > > > Signed-off-by: Stefano Stabellini <stef...@aporeto.com> > > 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&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
