[PATCH v4 0/4] new ring macros, 9pfs and pvcalls headers

[PATCH v4 0/4] new ring macros, 9pfs and pvcalls headers

[Stefano Stabellini]

Hi all,

this patch series introduces a set of new ring macros to support rings
in the formats specified by the Xen 9pfs transport and PV Calls
protocol. It also introduces the Xen 9pfs and PV Calls protocols
headers.


Changes in v4:
- include ../grant_table.h in ring.h
- add a comment about required declarations on top of ring.h
- add a patch to introduce a C99 headers check
- add -include string.h to the existing C++ headers check
- add 9pfs and pvcalls to the C99 headers check

Changes in v3:
- fix commit message
- add newlines after read/write_packet functions
- reorder DEFINE_XEN_FLEX_RING_AND_INTF and DEFINE_XEN_FLEX_RING

Changes in v2:
- replace __attribute__((packed)) with #pragma pack
- remove XEN_9PFS_RING_ORDER, the 9pfs ring order should be dynamic
- add editor configuration blocks
- add links to the specs


Stefano Stabellini (4):
      ring.h: introduce macros to handle monodirectional rings with multiple req sizes
      xen: introduce a C99 headers check
      Introduce the Xen 9pfs transport header
      Introduce the pvcalls header

 xen/include/Makefile            |  19 +++--
 xen/include/public/io/9pfs.h    |  57 +++++++++++++++
 xen/include/public/io/pvcalls.h | 152 ++++++++++++++++++++++++++++++++++++++++
 xen/include/public/io/ring.h    | 148 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 371 insertions(+), 5 deletions(-)
 create mode 100644 xen/include/public/io/9pfs.h
 create mode 100644 xen/include/public/io/pvcalls.h

Cheers,

Stefano

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

[PATCH v4 1/4] ring.h: introduce macros to handle monodirectional rings with multiple req sizes

[Stefano Stabellini]

This patch introduces macros, structs and functions to handle rings in
the format described by docs/misc/pvcalls.markdown and
docs/misc/9pfs.markdown. The index page (struct __name##_data_intf)
contains the indexes and the grant refs to setup two rings.

               Indexes page
               +----------------------+
               |@0 $NAME_data_intf:   |
               |@76: ring_order = 1   |
               |@80: ref[0]+          |
               |@84: ref[1]+          |
               |           |          |
               |           |          |
               +----------------------+
                           |
                           v (data ring)
                   +-------+-----------+
                   |  @0->4095: in     |
                   |  ref[0]           |
                   |-------------------|
                   |  @4096->8191: out |
                   |  ref[1]           |
                   +-------------------+

$NAME_read_packet and $NAME_write_packet are provided to read or write
any data struct from/to the ring. In pvcalls, they are unused. In xen
9pfs, they are used to read or write the 9pfs header. In other protocols
they could be used to read/write the whole request structure. See
docs/misc/9pfs.markdown:Ring Usage to learn how to check how much data
is on the ring, and how to handle notifications.

There is a ring_size parameter to most functions so that protocols using
these macros don't have to have a statically defined ring order at build
time. In pvcalls for example, each new ring could have a different
order.

These macros don't help you share the indexes page or the event channels
needed for notifications. You can do that with other out of band
mechanisms, such as xenstore or another ring.

It is not possible to use a macro to define another macro with a
variable name. For this reason, this patch introduces static inline
functions instead, that are not C89 compliant. Additionally, the macro
defines a struct with a variable sized array, which is also not C89
compliant.

Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
CC: konrad.wilk@oracle.com
CC: JBeulich@suse.com
---

The version number on this patch has been reset sometimes after it was
added to this series. Previously it was a standalone patch.

---
 xen/include/public/io/ring.h | 148 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 148 insertions(+)

diff --git a/xen/include/public/io/ring.h b/xen/include/public/io/ring.h
index 801c0da..83fd721 100644
--- a/xen/include/public/io/ring.h
+++ b/xen/include/public/io/ring.h
@@ -27,7 +27,18 @@
 #ifndef __XEN_PUBLIC_IO_RING_H__
 #define __XEN_PUBLIC_IO_RING_H__
 
+/* 
+ * When #include'ing this header, you need to provide the following
+ * declarations upfront:
+ * - standard integers types (uint8_t, uint16_t, etc)
+ * - size_t
+ * - memcpy
+ * These declarations are provided by stdint.h and string.h of the
+ * standard headers.
+ */
+
 #include "../xen-compat.h"
+#include "../grant_table.h"
 
 #if __XEN_INTERFACE_VERSION__ < 0x00030208
 #define xen_mb()  mb()
@@ -313,6 +324,143 @@ typedef struct __name##_back_ring __name##_back_ring_t
     (_work_to_do) = RING_HAS_UNCONSUMED_RESPONSES(_r);                  \
 } while (0)
 
+
+/*
+ * DEFINE_XEN_FLEX_RING_AND_INTF defines two monodirectional rings and
+ * functions to check if there is data on the ring, and to read and
+ * write to them.
+ *
+ * DEFINE_XEN_FLEX_RING is similar to DEFINE_XEN_FLEX_RING_AND_INTF, but
+ * does not define the indexes page. As different protocols can have
+ * extensions to the basic format, this macro allow them to define their
+ * own struct.
+ *
+ * XEN_FLEX_RING_SIZE
+ *   Convenience macro to calculate the size of one of the two rings
+ *   from the overall order.
+ *
+ * $NAME_mask
+ *   Function to apply the size mask to an index, to reduce the index
+ *   within the range [0-size].
+ *
+ * $NAME_read_packet
+ *   Function to read data from the ring. The amount of data to read is
+ *   specified by the "size" argument.
+ *
+ * $NAME_write_packet
+ *   Function to write data to the ring. The amount of data to write is
+ *   specified by the "size" argument.
+ *
+ * $NAME_get_ring_ptr
+ *   Convenience function that returns a pointer to read/write to the
+ *   ring at the right location.
+ *
+ * $NAME_data_intf
+ *   Indexes page, shared between frontend and backend. It also
+ *   contains the array of grant refs.
+ *
+ * $NAME_queued
+ *   Function to calculate how many bytes are currently on the ring,
+ *   ready to be read. It can also be used to calculate how much free
+ *   space is currently on the ring (ring_size - $NAME_queued()).
+ */
+#ifndef PAGE_SHIFT
+#define PAGE_SHIFT 12
+#endif
+
+#define XEN_FLEX_RING_SIZE(order)                                             \
+    (1UL << (order + PAGE_SHIFT - 1))
+
+#define DEFINE_XEN_FLEX_RING(name)                                            \
+static inline RING_IDX name##_mask(RING_IDX idx, RING_IDX ring_size)          \
+{                                                                             \
+    return (idx & (ring_size - 1));                                           \
+}                                                                             \
+                                                                              \
+static inline RING_IDX name##_mask_order(RING_IDX idx, RING_IDX ring_order)   \
+{                                                                             \
+    return (idx & (XEN_FLEX_RING_SIZE(ring_order) - 1));                      \
+}                                                                             \
+                                                                              \
+static inline unsigned char* name##_get_ring_ptr(unsigned char *buf,          \
+                                                 RING_IDX idx,                \
+                                                 RING_IDX ring_order)         \
+{                                                                             \
+    return buf + name##_mask_order(idx, ring_order);                          \
+}                                                                             \
+                                                                              \
+static inline void name##_read_packet(const unsigned char *buf,               \
+        RING_IDX masked_prod, RING_IDX *masked_cons,                          \
+        RING_IDX ring_size, void *opaque, size_t size)                        \
+{                                                                             \
+    if (*masked_cons < masked_prod ||                                         \
+            size <= ring_size - *masked_cons) {                               \
+        memcpy(opaque, buf + *masked_cons, size);                             \
+    } else {                                                                  \
+        memcpy(opaque, buf + *masked_cons, ring_size - *masked_cons);         \
+        memcpy((unsigned char *)opaque + ring_size - *masked_cons, buf,       \
+                size - (ring_size - *masked_cons));                           \
+    }                                                                         \
+    *masked_cons = name##_mask(*masked_cons + size, ring_size);               \
+}                                                                             \
+                                                                              \
+static inline void name##_write_packet(unsigned char *buf,                    \
+        RING_IDX *masked_prod, RING_IDX masked_cons,                          \
+        RING_IDX ring_size, const void *opaque, size_t size)                  \
+{                                                                             \
+    if (*masked_prod < masked_cons ||                                         \
+        size <= ring_size - *masked_prod) {                                   \
+        memcpy(buf + *masked_prod, opaque, size);                             \
+    } else {                                                                  \
+        memcpy(buf + *masked_prod, opaque, ring_size - *masked_prod);         \
+        memcpy(buf, (unsigned char *)opaque + (ring_size - *masked_prod),     \
+                size - (ring_size - *masked_prod));                           \
+    }                                                                         \
+    *masked_prod = name##_mask(*masked_prod + size, ring_size);               \
+}                                                                             \
+                                                                              \
+struct name##_data {                                                          \
+    unsigned char *in; /* half of the allocation */                           \
+    unsigned char *out; /* half of the allocation */                          \
+};                                                                            \
+                                                                              \
+                                                                              \
+static inline RING_IDX name##_queued(RING_IDX prod,                           \
+        RING_IDX cons, RING_IDX ring_size)                                    \
+{                                                                             \
+    RING_IDX size;                                                            \
+                                                                              \
+    if (prod == cons)                                                         \
+        return 0;                                                             \
+                                                                              \
+    prod = name##_mask(prod, ring_size);                                      \
+    cons = name##_mask(cons, ring_size);                                      \
+                                                                              \
+    if (prod == cons)                                                         \
+        return ring_size;                                                     \
+                                                                              \
+    if (prod > cons)                                                          \
+        size = prod - cons;                                                   \
+    else                                                                      \
+        size = ring_size - (cons - prod);                                     \
+    return size;                                                              \
+};
+
+#define DEFINE_XEN_FLEX_RING_AND_INTF(name)                                   \
+struct name##_data_intf {                                                     \
+    RING_IDX in_cons, in_prod;                                                \
+                                                                              \
+    uint8_t pad1[56];                                                         \
+                                                                              \
+    RING_IDX out_cons, out_prod;                                              \
+                                                                              \
+    uint8_t pad2[56];                                                         \
+                                                                              \
+    RING_IDX ring_order;                                                      \
+    grant_ref_t ref[];                                                        \
+};                                                                            \
+DEFINE_XEN_FLEX_RING(name);
+
 #endif /* __XEN_PUBLIC_IO_RING_H__ */
 
 /*
-- 
1.9.1


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

[PATCH v4 2/4] xen: introduce a C99 headers check

[Stefano Stabellini]

Introduce a C99 headers check, for non-ANSI compliant headers. No
headers are added to the check yet.

In addition to the usual -include stdint.h, also add -include string.h
to the C99 check to get the declaration of memcpy and size_t.

For the same reasons, also add -include string.h to the C++ check.

Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
CC: JBeulich@suse.com
CC: konrad.wilk@oracle.com
---

It is actually string.h, not stddef.h, that provides the declaration of
memcpy.

---
 xen/include/Makefile | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/xen/include/Makefile b/xen/include/Makefile
index aca7f20..a46485b 100644
--- a/xen/include/Makefile
+++ b/xen/include/Makefile
@@ -90,11 +90,12 @@ compat/xlat.h: $(addprefix compat/.xlat/,$(xlat-y)) Makefile
 
 ifeq ($(XEN_TARGET_ARCH),$(XEN_COMPILE_ARCH))
 
-all: headers.chk headers++.chk
+all: headers.chk headers99.chk headers++.chk
 
 PUBLIC_HEADERS := $(filter-out public/arch-% public/dom0_ops.h, $(wildcard public/*.h public/*/*.h) $(public-y))
 
-PUBLIC_ANSI_HEADERS := $(filter-out public/%ctl.h public/xsm/% public/%hvm/save.h, $(PUBLIC_HEADERS))
+PUBLIC_C99_HEADERS :=
+PUBLIC_ANSI_HEADERS := $(filter-out public/%ctl.h public/xsm/% public/%hvm/save.h $(PUBLIC_C99_HEADERS), $(PUBLIC_HEADERS))
 
 headers.chk: $(PUBLIC_ANSI_HEADERS) Makefile
 	for i in $(filter %.h,$^); do \
@@ -104,18 +105,26 @@ headers.chk: $(PUBLIC_ANSI_HEADERS) Makefile
 	done >$@.new
 	mv $@.new $@
 
+headers99.chk: $(PUBLIC_C99_HEADERS) Makefile
+	for i in $(filter %.h,$^); do \
+	    $(CC) -x c -std=c99 -Wall -Werror -include stdint.h \
+	          -include string.h -S -o /dev/null $$i || exit 1; \
+	    echo $$i; \
+	done >$@.new
+	mv $@.new $@
+
 headers++.chk: $(PUBLIC_HEADERS) Makefile
 	if $(CXX) -v >/dev/null 2>&1; then \
 	    for i in $(filter %.h,$^); do \
 	        echo '#include "'$$i'"' \
 	        | $(CXX) -x c++ -std=gnu++98 -Wall -Werror -D__XEN_TOOLS__ \
-	          -include stdint.h -include public/xen.h -S -o /dev/null - \
+	          -include stdint.h -include string.h -include public/xen.h \
+	          -S -o /dev/null - \
 	        || exit 1; \
 	        echo $$i; \
 	    done ; \
 	fi >$@.new
 	mv $@.new $@
-
 endif
 
 ifeq ($(XEN_TARGET_ARCH),x86_64)
@@ -128,5 +137,5 @@ all: $(BASEDIR)/include/asm-x86/cpuid-autogen.h
 endif
 
 clean::
-	rm -rf compat headers.chk headers++.chk
+	rm -rf compat headers.chk headers99.chk headers++.chk
 	rm -f $(BASEDIR)/include/asm-x86/cpuid-autogen.h
-- 
1.9.1


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

[PATCH v4 3/4] Introduce the Xen 9pfs transport header

[Stefano Stabellini]

Define the ring according to the protocol specification, using the new
DEFINE_XEN_FLEX_RING_AND_INTF macro.

Add the header to the C99 check.

Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
CC: JBeulich@suse.com
CC: konrad.wilk@oracle.com
---
 xen/include/Makefile         |  2 +-
 xen/include/public/io/9pfs.h | 57 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+), 1 deletion(-)
 create mode 100644 xen/include/public/io/9pfs.h

diff --git a/xen/include/Makefile b/xen/include/Makefile
index a46485b..611576f 100644
--- a/xen/include/Makefile
+++ b/xen/include/Makefile
@@ -94,7 +94,7 @@ all: headers.chk headers99.chk headers++.chk
 
 PUBLIC_HEADERS := $(filter-out public/arch-% public/dom0_ops.h, $(wildcard public/*.h public/*/*.h) $(public-y))
 
-PUBLIC_C99_HEADERS :=
+PUBLIC_C99_HEADERS := public/io/9pfs.h
 PUBLIC_ANSI_HEADERS := $(filter-out public/%ctl.h public/xsm/% public/%hvm/save.h $(PUBLIC_C99_HEADERS), $(PUBLIC_HEADERS))
 
 headers.chk: $(PUBLIC_ANSI_HEADERS) Makefile
diff --git a/xen/include/public/io/9pfs.h b/xen/include/public/io/9pfs.h
new file mode 100644
index 0000000..bff9ad8
--- /dev/null
+++ b/xen/include/public/io/9pfs.h
@@ -0,0 +1,57 @@
+/*
+ * 9pfs.h -- Xen 9PFS transport
+ *
+ * Refer to docs/misc/9pfs.markdown for the specification
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
+ * deal in the Software without restriction, including without limitation the
+ * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ *
+ * Copyright (C) 2017 Stefano Stabellini <stefano@aporeto.com>
+ */
+
+#ifndef __XEN_PUBLIC_IO_9PFS_H__
+#define __XEN_PUBLIC_IO_9PFS_H__
+
+#include "ring.h"
+
+/*
+ * See docs/misc/9pfs.markdown in xen.git for the full specification:
+ * https://xenbits.xen.org/docs/unstable/misc/9pfs.html
+ */
+#pragma pack(push)
+#pragma pack(1)
+struct xen_9pfs_header {
+	uint32_t size;
+	uint8_t id;
+	uint16_t tag;
+};
+#pragma pack(pop)
+
+DEFINE_XEN_FLEX_RING_AND_INTF(xen_9pfs);
+
+#endif
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
1.9.1


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

[PATCH v4 4/4] Introduce the pvcalls header

[Stefano Stabellini]

Define the ring and request and response structs according to the
specification. Use the new DEFINE_XEN_FLEX_RING macro.

Add the header to the C99 check.

Signed-off-by: Stefano Stabellini <stefano@aporeto.com>
CC: JBeulich@suse.com
CC: konrad.wilk@oracle.com
---
 xen/include/Makefile            |   2 +-
 xen/include/public/io/pvcalls.h | 152 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 153 insertions(+), 1 deletion(-)
 create mode 100644 xen/include/public/io/pvcalls.h

diff --git a/xen/include/Makefile b/xen/include/Makefile
index 611576f..c3fc99c 100644
--- a/xen/include/Makefile
+++ b/xen/include/Makefile
@@ -94,7 +94,7 @@ all: headers.chk headers99.chk headers++.chk
 
 PUBLIC_HEADERS := $(filter-out public/arch-% public/dom0_ops.h, $(wildcard public/*.h public/*/*.h) $(public-y))
 
-PUBLIC_C99_HEADERS := public/io/9pfs.h
+PUBLIC_C99_HEADERS := public/io/9pfs.h public/io/pvcalls.h
 PUBLIC_ANSI_HEADERS := $(filter-out public/%ctl.h public/xsm/% public/%hvm/save.h $(PUBLIC_C99_HEADERS), $(PUBLIC_HEADERS))
 
 headers.chk: $(PUBLIC_ANSI_HEADERS) Makefile
diff --git a/xen/include/public/io/pvcalls.h b/xen/include/public/io/pvcalls.h
new file mode 100644
index 0000000..4123528
--- /dev/null
+++ b/xen/include/public/io/pvcalls.h
@@ -0,0 +1,152 @@
+/*
+ * pvcalls.h -- Xen PV Calls Protocol
+ *
+ * Refer to docs/misc/pvcalls.markdown for the specification
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
+ * deal in the Software without restriction, including without limitation the
+ * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ *
+ * Copyright (C) 2017 Stefano Stabellini <stefano@aporeto.com>
+ */
+
+#ifndef __XEN_PUBLIC_IO_PVCALLS_H__
+#define __XEN_PUBLIC_IO_PVCALLS_H__
+
+#include "ring.h"
+
+/*
+ * See docs/misc/pvcalls.markdown in xen.git for the full specification:
+ * https://xenbits.xen.org/docs/unstable/misc/pvcalls.html
+ */
+struct pvcalls_data_intf {
+    RING_IDX in_cons, in_prod, in_error;
+
+    uint8_t pad1[52];
+
+    RING_IDX out_cons, out_prod, out_error;
+
+    uint8_t pad2[52];
+
+    RING_IDX ring_order;
+    grant_ref_t ref[];
+};
+DEFINE_XEN_FLEX_RING(pvcalls);
+
+#define PVCALLS_SOCKET         0
+#define PVCALLS_CONNECT        1
+#define PVCALLS_RELEASE        2
+#define PVCALLS_BIND           3
+#define PVCALLS_LISTEN         4
+#define PVCALLS_ACCEPT         5
+#define PVCALLS_POLL           6
+
+struct xen_pvcalls_request {
+    uint32_t req_id; /* private to guest, echoed in response */
+    uint32_t cmd;    /* command to execute */
+    union {
+        struct xen_pvcalls_socket {
+            uint64_t id;
+            uint32_t domain;
+            uint32_t type;
+            uint32_t protocol;
+        } socket;
+        struct xen_pvcalls_connect {
+            uint64_t id;
+            uint8_t addr[28];
+            uint32_t len;
+            uint32_t flags;
+            grant_ref_t ref;
+            uint32_t evtchn;
+        } connect;
+        struct xen_pvcalls_release {
+            uint64_t id;
+            uint8_t reuse;
+        } release;
+        struct xen_pvcalls_bind {
+            uint64_t id;
+            uint8_t addr[28];
+            uint32_t len;
+        } bind;
+        struct xen_pvcalls_listen {
+            uint64_t id;
+            uint32_t backlog;
+        } listen;
+        struct xen_pvcalls_accept {
+            uint64_t id;
+            uint64_t id_new;
+            grant_ref_t ref;
+            uint32_t evtchn;
+        } accept;
+        struct xen_pvcalls_poll {
+            uint64_t id;
+        } poll;
+        /* dummy member to force sizeof(struct xen_pvcalls_request)
+         * to match across archs */
+        struct xen_pvcalls_dummy {
+            uint8_t dummy[56];
+        } dummy;
+    } u;
+};
+
+struct xen_pvcalls_response {
+    uint32_t req_id;
+    uint32_t cmd;
+    int32_t ret;
+    uint32_t pad;
+    union {
+        struct _xen_pvcalls_socket {
+            uint64_t id;
+        } socket;
+        struct _xen_pvcalls_connect {
+            uint64_t id;
+        } connect;
+        struct _xen_pvcalls_release {
+            uint64_t id;
+        } release;
+        struct _xen_pvcalls_bind {
+            uint64_t id;
+        } bind;
+        struct _xen_pvcalls_listen {
+            uint64_t id;
+        } listen;
+        struct _xen_pvcalls_accept {
+            uint64_t id;
+        } accept;
+        struct _xen_pvcalls_poll {
+            uint64_t id;
+        } poll;
+        struct _xen_pvcalls_dummy {
+            uint8_t dummy[8];
+        } dummy;
+    } u;
+};
+
+DEFINE_RING_TYPES(xen_pvcalls, struct xen_pvcalls_request,
+                  struct xen_pvcalls_response);
+
+#endif
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
1.9.1


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

Re: [PATCH v4 3/4] Introduce the Xen 9pfs transport header

[Jan Beulich]

>>> On 24.03.17 at 19:31, <sstabellini@kernel.org> wrote:
> --- /dev/null
> +++ b/xen/include/public/io/9pfs.h
> @@ -0,0 +1,57 @@
> +/*
> + * 9pfs.h -- Xen 9PFS transport
> + *
> + * Refer to docs/misc/9pfs.markdown for the specification
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to
> + * deal in the Software without restriction, including without limitation the
> + * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
> + * sell copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + *
> + * Copyright (C) 2017 Stefano Stabellini <stefano@aporeto.com>
> + */
> +
> +#ifndef __XEN_PUBLIC_IO_9PFS_H__
> +#define __XEN_PUBLIC_IO_9PFS_H__
> +
> +#include "ring.h"
> +
> +/*
> + * See docs/misc/9pfs.markdown in xen.git for the full specification:
> + * https://xenbits.xen.org/docs/unstable/misc/9pfs.html 
> + */
> +#pragma pack(push)
> +#pragma pack(1)
> +struct xen_9pfs_header {
> +	uint32_t size;
> +	uint8_t id;
> +	uint16_t tag;
> +};
> +#pragma pack(pop)

There's no precedent to using pragmas in the public headers, and
these aren't C99-compliant.

Jan


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

Re: [PATCH v4 1/4] ring.h: introduce macros to handle monodirectional rings with multiple req sizes

[Jan Beulich]

>>> On 24.03.17 at 19:31, <sstabellini@kernel.org> wrote:
> --- a/xen/include/public/io/ring.h
> +++ b/xen/include/public/io/ring.h
> @@ -27,7 +27,18 @@
>  #ifndef __XEN_PUBLIC_IO_RING_H__
>  #define __XEN_PUBLIC_IO_RING_H__
>  
> +/* 
> + * When #include'ing this header, you need to provide the following
> + * declarations upfront:
> + * - standard integers types (uint8_t, uint16_t, etc)
> + * - size_t
> + * - memcpy
> + * These declarations are provided by stdint.h and string.h of the
> + * standard headers.
> + */
> +
>  #include "../xen-compat.h"
> +#include "../grant_table.h"

I'd prefer this to be added to the prereqs, as the header itself will
- afaict - compile fine without the #include above. Also the list of
prereqs only applies to people who mean to use the new macros,
doesn't it? In that case the comment should say so.

> +#ifndef PAGE_SHIFT
> +#define PAGE_SHIFT 12
> +#endif

??? (I guess this should be another prereq?)

> +#define XEN_FLEX_RING_SIZE(order)                                             \
> +    (1UL << (order + PAGE_SHIFT - 1))

Please parenthesize uses of macro parameters.

> +static inline unsigned char* name##_get_ring_ptr(unsigned char *buf,          \

Misordered * and space (return type).

Jan


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

Re: [PATCH v4 2/4] xen: introduce a C99 headers check

[Jan Beulich]

>>> On 24.03.17 at 19:31, <sstabellini@kernel.org> wrote:
> Introduce a C99 headers check, for non-ANSI compliant headers. No
> headers are added to the check yet.
> 
> In addition to the usual -include stdint.h, also add -include string.h
> to the C99 check to get the declaration of memcpy and size_t.

Is this really needed for the check to succeed, without there being
any user of the macros?

> For the same reasons, also add -include string.h to the C++ check.

Same here then.

> @@ -104,18 +105,26 @@ headers.chk: $(PUBLIC_ANSI_HEADERS) Makefile
>  	done >$@.new
>  	mv $@.new $@
>  
> +headers99.chk: $(PUBLIC_C99_HEADERS) Makefile
> +	for i in $(filter %.h,$^); do \
> +	    $(CC) -x c -std=c99 -Wall -Werror -include stdint.h \
> +	          -include string.h -S -o /dev/null $$i || exit 1; \
> +	    echo $$i; \
> +	done >$@.new
> +	mv $@.new $@
> +
>  headers++.chk: $(PUBLIC_HEADERS) Makefile
>  	if $(CXX) -v >/dev/null 2>&1; then \
>  	    for i in $(filter %.h,$^); do \
>  	        echo '#include "'$$i'"' \
>  	        | $(CXX) -x c++ -std=gnu++98 -Wall -Werror -D__XEN_TOOLS__ \
> -	          -include stdint.h -include public/xen.h -S -o /dev/null - \
> +	          -include stdint.h -include string.h -include public/xen.h \
> +	          -S -o /dev/null - \
>  	        || exit 1; \
>  	        echo $$i; \
>  	    done ; \
>  	fi >$@.new
>  	mv $@.new $@
> -
>  endif

Please don't remove blank lines like this.

> @@ -128,5 +137,5 @@ all: $(BASEDIR)/include/asm-x86/cpuid-autogen.h
>  endif
>  
>  clean::
> -	rm -rf compat headers.chk headers++.chk
> +	rm -rf compat headers.chk headers99.chk headers++.chk

	rm -rf compat headers*.chk

? Plus a respective ./.gitignore adjustment.

Jan


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

Re: [PATCH v4 2/4] xen: introduce a C99 headers check

[Stefano Stabellini]

On Mon, 27 Mar 2017, Jan Beulich wrote:
> >>> On 24.03.17 at 19:31, <sstabellini@kernel.org> wrote:
> > Introduce a C99 headers check, for non-ANSI compliant headers. No
> > headers are added to the check yet.
> > 
> > In addition to the usual -include stdint.h, also add -include string.h
> > to the C99 check to get the declaration of memcpy and size_t.
> 
> Is this really needed for the check to succeed, without there being
> any user of the macros?

The check is unneeded, but the two following patches are going to add
two new users of these macros. The check will be needed then. I'll move
the patch to the end of the series.


> > For the same reasons, also add -include string.h to the C++ check.
> 
> Same here then.
>
> > @@ -104,18 +105,26 @@ headers.chk: $(PUBLIC_ANSI_HEADERS) Makefile
> >  	done >$@.new
> >  	mv $@.new $@
> >  
> > +headers99.chk: $(PUBLIC_C99_HEADERS) Makefile
> > +	for i in $(filter %.h,$^); do \
> > +	    $(CC) -x c -std=c99 -Wall -Werror -include stdint.h \
> > +	          -include string.h -S -o /dev/null $$i || exit 1; \
> > +	    echo $$i; \
> > +	done >$@.new
> > +	mv $@.new $@
> > +
> >  headers++.chk: $(PUBLIC_HEADERS) Makefile
> >  	if $(CXX) -v >/dev/null 2>&1; then \
> >  	    for i in $(filter %.h,$^); do \
> >  	        echo '#include "'$$i'"' \
> >  	        | $(CXX) -x c++ -std=gnu++98 -Wall -Werror -D__XEN_TOOLS__ \
> > -	          -include stdint.h -include public/xen.h -S -o /dev/null - \
> > +	          -include stdint.h -include string.h -include public/xen.h \
> > +	          -S -o /dev/null - \
> >  	        || exit 1; \
> >  	        echo $$i; \
> >  	    done ; \
> >  	fi >$@.new
> >  	mv $@.new $@
> > -
> >  endif
> 
> Please don't remove blank lines like this.

Ops, sorry


> > @@ -128,5 +137,5 @@ all: $(BASEDIR)/include/asm-x86/cpuid-autogen.h
> >  endif
> >  
> >  clean::
> > -	rm -rf compat headers.chk headers++.chk
> > +	rm -rf compat headers.chk headers99.chk headers++.chk
> 
> 	rm -rf compat headers*.chk
> 
> ? Plus a respective ./.gitignore adjustment.

I'll do

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

Re: [PATCH v4 1/4] ring.h: introduce macros to handle monodirectional rings with multiple req sizes

[Stefano Stabellini]

On Mon, 27 Mar 2017, Jan Beulich wrote:
> >>> On 24.03.17 at 19:31, <sstabellini@kernel.org> wrote:
> > --- a/xen/include/public/io/ring.h
> > +++ b/xen/include/public/io/ring.h
> > @@ -27,7 +27,18 @@
> >  #ifndef __XEN_PUBLIC_IO_RING_H__
> >  #define __XEN_PUBLIC_IO_RING_H__
> >  
> > +/* 
> > + * When #include'ing this header, you need to provide the following
> > + * declarations upfront:
> > + * - standard integers types (uint8_t, uint16_t, etc)
> > + * - size_t
> > + * - memcpy
> > + * These declarations are provided by stdint.h and string.h of the
> > + * standard headers.
> > + */
> > +
> >  #include "../xen-compat.h"
> > +#include "../grant_table.h"
> 
> I'd prefer this to be added to the prereqs, as the header itself will
> - afaict - compile fine without the #include above.

It does not, see "grant_ref_t ref[]" in struct name##_data_intf.


> Also the list of
> prereqs only applies to people who mean to use the new macros,
> doesn't it? In that case the comment should say so.

Standard integers types are needed also for the old macros. I'll
clarify what's needed for what.


> > +#ifndef PAGE_SHIFT
> > +#define PAGE_SHIFT 12
> > +#endif
> 
> ??? (I guess this should be another prereq?)

Yes, the PAGE_SHIFT definition could be a prereq, but I thought that
these 3 lines are small enough that they would be harmless. I am happy
to make it a prereq though, I'll remove it.


> > +#define XEN_FLEX_RING_SIZE(order)                                             \
> > +    (1UL << (order + PAGE_SHIFT - 1))
> 
> Please parenthesize uses of macro parameters.

OK


> > +static inline unsigned char* name##_get_ring_ptr(unsigned char *buf,          \
> 
> Misordered * and space (return type).

I'll fix.

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

Re: [PATCH v4 3/4] Introduce the Xen 9pfs transport header

[Stefano Stabellini]

On Mon, 27 Mar 2017, Jan Beulich wrote:
> >>> On 24.03.17 at 19:31, <sstabellini@kernel.org> wrote:
> > --- /dev/null
> > +++ b/xen/include/public/io/9pfs.h
> > @@ -0,0 +1,57 @@
> > +/*
> > + * 9pfs.h -- Xen 9PFS transport
> > + *
> > + * Refer to docs/misc/9pfs.markdown for the specification
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a copy
> > + * of this software and associated documentation files (the "Software"), to
> > + * deal in the Software without restriction, including without limitation the
> > + * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
> > + * sell copies of the Software, and to permit persons to whom the Software is
> > + * furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice shall be included in
> > + * all copies or substantial portions of the Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
> > + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> > + * DEALINGS IN THE SOFTWARE.
> > + *
> > + * Copyright (C) 2017 Stefano Stabellini <stefano@aporeto.com>
> > + */
> > +
> > +#ifndef __XEN_PUBLIC_IO_9PFS_H__
> > +#define __XEN_PUBLIC_IO_9PFS_H__
> > +
> > +#include "ring.h"
> > +
> > +/*
> > + * See docs/misc/9pfs.markdown in xen.git for the full specification:
> > + * https://xenbits.xen.org/docs/unstable/misc/9pfs.html 
> > + */
> > +#pragma pack(push)
> > +#pragma pack(1)
> > +struct xen_9pfs_header {
> > +	uint32_t size;
> > +	uint8_t id;
> > +	uint16_t tag;
> > +};
> > +#pragma pack(pop)
> 
> There's no precedent to using pragmas in the public headers, and
> these aren't C99-compliant.

I'll remove pragma, together with the definition of struct
xen_9pfs_header: this structure is already defined as part of the 9p
protocol, and it is already mentioned in the Xen 9pfs transport spec as
well. In fact, both QEMU and Linux already have it defined. I don't
think we need it here.

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

Re: [PATCH v4 1/4] ring.h: introduce macros to handle monodirectional rings with multiple req sizes

[Stefano Stabellini]

On Mon, 27 Mar 2017, Stefano Stabellini wrote:
> > > +#ifndef PAGE_SHIFT
> > > +#define PAGE_SHIFT 12
> > > +#endif
> > 
> > ??? (I guess this should be another prereq?)
> 
> Yes, the PAGE_SHIFT definition could be a prereq, but I thought that
> these 3 lines are small enough that they would be harmless. I am happy
> to make it a prereq though, I'll remove it.

Thinking more about this, PAGE_SHIFT should come from Xen, rather than
Linux or any standard headers, because on systems that support multiple
page sizes, the page size of the Xen protocols should always be the same
(4096). In other words, on an arm64 server Xen will use 4K pages, Dom0
could use 64K pages, but the 9pfs protocol will still use 4K pages for
compatilibity with other guests that might or might not support 64K.
This is how 64K pages are implemented in regards to Xen PV protocols
today: we assume that 4K is baked into the protocols' ABI.

Thus, I think I'll have to keep the PAGE_SHIFT definition here, but
maybe I'll rename it to XEN_PAGE_SHIFT instead, to avoid clashes with
the system's PAGE_SHIFT that could differ.

Something like:

#define XEN_PAGE_SHIFT 12
#define XEN_FLEX_RING_SIZE(order)                                             \
    (1UL << ((order) + XEN_PAGE_SHIFT - 1))


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

Re: [PATCH v4 1/4] ring.h: introduce macros to handle monodirectional rings with multiple req sizes

[Jan Beulich]

>>> Stefano Stabellini <sstabellini@kernel.org> 03/27/17 10:53 PM >>>
>On Mon, 27 Mar 2017, Jan Beulich wrote:
>> >>> On 24.03.17 at 19:31, <sstabellini@kernel.org> wrote:
>> > --- a/xen/include/public/io/ring.h
>> > +++ b/xen/include/public/io/ring.h
>> > @@ -27,7 +27,18 @@
>> >  #ifndef __XEN_PUBLIC_IO_RING_H__
>> >  #define __XEN_PUBLIC_IO_RING_H__
>> >  
>> > +/* 
>> > + * When #include'ing this header, you need to provide the following
>> > + * declarations upfront:
>> > + * - standard integers types (uint8_t, uint16_t, etc)
>> > + * - size_t
>> > + * - memcpy
>> > + * These declarations are provided by stdint.h and string.h of the
>> > + * standard headers.
>> > + */
>> > +
>> >  #include "../xen-compat.h"
>> > +#include "../grant_table.h"
>> 
>> I'd prefer this to be added to the prereqs, as the header itself will
>> - afaict - compile fine without the #include above.
>
>It does not, see "grant_ref_t ref[]" in struct name##_data_intf.

The use is in a macro _definition_, and iirc the macro is not being _used_.
(Almost) any garbage can be put in unused macro definitions without causing
compile breakage.

>> > +#ifndef PAGE_SHIFT
>> > +#define PAGE_SHIFT 12
>> > +#endif
>> 
>> ??? (I guess this should be another prereq?)
>
>Yes, the PAGE_SHIFT definition could be a prereq, but I thought that
>these 3 lines are small enough that they would be harmless. I am happy
>to make it a prereq though, I'll remove it.

The problem isn't size or anything, but the risk to clash with a _later_ definition
(due to a subsequent #include). The name, after all, isn't in the XEN_ name space.

Jan


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

Re: [PATCH v4 2/4] xen: introduce a C99 headers check

[Jan Beulich]

>>> Stefano Stabellini <sstabellini@kernel.org> 03/27/17 10:50 PM >>>
>On Mon, 27 Mar 2017, Jan Beulich wrote:
>> >>> On 24.03.17 at 19:31, <sstabellini@kernel.org> wrote:
>> > Introduce a C99 headers check, for non-ANSI compliant headers. No
>> > headers are added to the check yet.
>> > 
>> > In addition to the usual -include stdint.h, also add -include string.h
>> > to the C99 check to get the declaration of memcpy and size_t.
>> 
>> Is this really needed for the check to succeed, without there being
>> any user of the macros?
>
>The check is unneeded, but the two following patches are going to add
>two new users of these macros. The check will be needed then. I'll move
>the patch to the end of the series.

We shouldn't enforce this extra inclusion on all headers, i.e. I think you'll
need to introduce per-header prereqs to be enforced via -include.

> > For the same reasons, also add -include string.h to the C++ check.
> 
> Same here then.

One more thing here - if at all, this perhaps would need to be cstring instead
of string.h.

Jan


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

Re: [PATCH v4 3/4] Introduce the Xen 9pfs transport header

[Jan Beulich]

>>> Stefano Stabellini <sstabellini@kernel.org> 03/27/17 10:54 PM >>>
>On Mon, 27 Mar 2017, Jan Beulich wrote:
>> >>> On 24.03.17 at 19:31, <sstabellini@kernel.org> wrote:
>> > +/*
>> > + * See docs/misc/9pfs.markdown in xen.git for the full specification:
>> > + * https://xenbits.xen.org/docs/unstable/misc/9pfs.html 
>> > + */
>> > +#pragma pack(push)
>> > +#pragma pack(1)
>> > +struct xen_9pfs_header {
>> > +	uint32_t size;
>> > +	uint8_t id;
>> > +	uint16_t tag;
>> > +};
>> > +#pragma pack(pop)
>> 
>> There's no precedent to using pragmas in the public headers, and
>> these aren't C99-compliant.
>
>I'll remove pragma, together with the definition of struct
>xen_9pfs_header: this structure is already defined as part of the 9p
>protocol, and it is already mentioned in the Xen 9pfs transport spec as
>well. In fact, both QEMU and Linux already have it defined. I don't
>think we need it here.

That'll deal with the immediate issue here, but not with the more general
implied one: Why would you want to have misaligned fields in a protocol
definition?

Jan


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

Re: [PATCH v4 3/4] Introduce the Xen 9pfs transport header

[Stefano Stabellini]

On Tue, 28 Mar 2017, Jan Beulich wrote:
> >>> Stefano Stabellini <sstabellini@kernel.org> 03/27/17 10:54 PM >>>
> >On Mon, 27 Mar 2017, Jan Beulich wrote:
> >> >>> On 24.03.17 at 19:31, <sstabellini@kernel.org> wrote:
> >> > +/*
> >> > + * See docs/misc/9pfs.markdown in xen.git for the full specification:
> >> > + * https://xenbits.xen.org/docs/unstable/misc/9pfs.html 
> >> > + */
> >> > +#pragma pack(push)
> >> > +#pragma pack(1)
> >> > +struct xen_9pfs_header {
> >> > +	uint32_t size;
> >> > +	uint8_t id;
> >> > +	uint16_t tag;
> >> > +};
> >> > +#pragma pack(pop)
> >> 
> >> There's no precedent to using pragmas in the public headers, and
> >> these aren't C99-compliant.
> >
> >I'll remove pragma, together with the definition of struct
> >xen_9pfs_header: this structure is already defined as part of the 9p
> >protocol, and it is already mentioned in the Xen 9pfs transport spec as
> >well. In fact, both QEMU and Linux already have it defined. I don't
> >think we need it here.
> 
> That'll deal with the immediate issue here, but not with the more general
> implied one: Why would you want to have misaligned fields in a protocol
> definition?

Because this header is not actually part of the Xen trasport protocol,
it is defined by the 9pfs specification. That's why QEMU already had it.
I cannot do anything about that. I was only redefining it here for
convenience, because reading the header is required to figure out how
big is a request (or a response).

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

Re: [PATCH v4 1/4] ring.h: introduce macros to handle monodirectional rings with multiple req sizes

[Stefano Stabellini]

On Tue, 28 Mar 2017, Jan Beulich wrote:
> >>> Stefano Stabellini <sstabellini@kernel.org> 03/27/17 10:53 PM >>>
> >On Mon, 27 Mar 2017, Jan Beulich wrote:
> >> >>> On 24.03.17 at 19:31, <sstabellini@kernel.org> wrote:
> >> > --- a/xen/include/public/io/ring.h
> >> > +++ b/xen/include/public/io/ring.h
> >> > @@ -27,7 +27,18 @@
> >> >  #ifndef __XEN_PUBLIC_IO_RING_H__
> >> >  #define __XEN_PUBLIC_IO_RING_H__
> >> >  
> >> > +/* 
> >> > + * When #include'ing this header, you need to provide the following
> >> > + * declarations upfront:
> >> > + * - standard integers types (uint8_t, uint16_t, etc)
> >> > + * - size_t
> >> > + * - memcpy
> >> > + * These declarations are provided by stdint.h and string.h of the
> >> > + * standard headers.
> >> > + */
> >> > +
> >> >  #include "../xen-compat.h"
> >> > +#include "../grant_table.h"
> >> 
> >> I'd prefer this to be added to the prereqs, as the header itself will
> >> - afaict - compile fine without the #include above.
> >
> >It does not, see "grant_ref_t ref[]" in struct name##_data_intf.
> 
> The use is in a macro _definition_, and iirc the macro is not being _used_.
> (Almost) any garbage can be put in unused macro definitions without causing
> compile breakage.

I think I understand: you prefer to remove the inclusion from ring.h,
add it to the prereqs, and add the #include to 9pfs.h and pvcalls.h.
I'll do that.


> >> > +#ifndef PAGE_SHIFT
> >> > +#define PAGE_SHIFT 12
> >> > +#endif
> >> 
> >> ??? (I guess this should be another prereq?)
> >
> >Yes, the PAGE_SHIFT definition could be a prereq, but I thought that
> >these 3 lines are small enough that they would be harmless. I am happy
> >to make it a prereq though, I'll remove it.
> 
> The problem isn't size or anything, but the risk to clash with a _later_ definition
> (due to a subsequent #include). The name, after all, isn't in the XEN_ name space.
 
Yes, you are right, and as I replied afterward we cannot rely on the
PAGE_SHIFT set by the system anyway. If that's OK I'll define
XEN_PAGE_SHIFT instead.

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

Re: [PATCH v4 2/4] xen: introduce a C99 headers check

[Stefano Stabellini]

On Tue, 28 Mar 2017, Jan Beulich wrote:
> >>> Stefano Stabellini <sstabellini@kernel.org> 03/27/17 10:50 PM >>>
> >On Mon, 27 Mar 2017, Jan Beulich wrote:
> >> >>> On 24.03.17 at 19:31, <sstabellini@kernel.org> wrote:
> >> > Introduce a C99 headers check, for non-ANSI compliant headers. No
> >> > headers are added to the check yet.
> >> > 
> >> > In addition to the usual -include stdint.h, also add -include string.h
> >> > to the C99 check to get the declaration of memcpy and size_t.
> >> 
> >> Is this really needed for the check to succeed, without there being
> >> any user of the macros?
> >
> >The check is unneeded, but the two following patches are going to add
> >two new users of these macros. The check will be needed then. I'll move
> >the patch to the end of the series.
> 
> We shouldn't enforce this extra inclusion on all headers, i.e. I think you'll
> need to introduce per-header prereqs to be enforced via -include.

I understand what you mean now, but all C99 headers have the same
prereqs and none of the ansi headers have any prereqs. The only
overlapping case is the c++ tests.

I would probably assume that c99 headers have an extra prereq, and
maybe add a check only in the c++ test to add prereqs when necessary.

I'll send a new version of the series today, maybe we can discuss that.


> > > For the same reasons, also add -include string.h to the C++ check.
> > 
> > Same here then.
> 
> One more thing here - if at all, this perhaps would need to be cstring instead
> of string.h.

OK

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

Re: [PATCH v4 1/4] ring.h: introduce macros to handle monodirectional rings with multiple req sizes

[Jan Beulich]

>>> On 28.03.17 at 23:07, <sstabellini@kernel.org> wrote:
> On Tue, 28 Mar 2017, Jan Beulich wrote:
>> >>> Stefano Stabellini <sstabellini@kernel.org> 03/27/17 10:53 PM >>>
>> >On Mon, 27 Mar 2017, Jan Beulich wrote:
>> >> >>> On 24.03.17 at 19:31, <sstabellini@kernel.org> wrote:
>> >> > +#ifndef PAGE_SHIFT
>> >> > +#define PAGE_SHIFT 12
>> >> > +#endif
>> >> 
>> >> ??? (I guess this should be another prereq?)
>> >
>> >Yes, the PAGE_SHIFT definition could be a prereq, but I thought that
>> >these 3 lines are small enough that they would be harmless. I am happy
>> >to make it a prereq though, I'll remove it.
>> 
>> The problem isn't size or anything, but the risk to clash with a _later_ definition
>> (due to a subsequent #include). The name, after all, isn't in the XEN_ name space.
>  
> Yes, you are right, and as I replied afterward we cannot rely on the
> PAGE_SHIFT set by the system anyway. If that's OK I'll define
> XEN_PAGE_SHIFT instead.

That should be fine, I think.

Jan


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

Re: [PATCH v4 3/4] Introduce the Xen 9pfs transport header

[Jan Beulich]

>>> On 28.03.17 at 23:04, <sstabellini@kernel.org> wrote:
> On Tue, 28 Mar 2017, Jan Beulich wrote:
>> >>> Stefano Stabellini <sstabellini@kernel.org> 03/27/17 10:54 PM >>>
>> >On Mon, 27 Mar 2017, Jan Beulich wrote:
>> >> >>> On 24.03.17 at 19:31, <sstabellini@kernel.org> wrote:
>> >> > +/*
>> >> > + * See docs/misc/9pfs.markdown in xen.git for the full specification:
>> >> > + * https://xenbits.xen.org/docs/unstable/misc/9pfs.html 
>> >> > + */
>> >> > +#pragma pack(push)
>> >> > +#pragma pack(1)
>> >> > +struct xen_9pfs_header {
>> >> > +	uint32_t size;
>> >> > +	uint8_t id;
>> >> > +	uint16_t tag;
>> >> > +};
>> >> > +#pragma pack(pop)
>> >> 
>> >> There's no precedent to using pragmas in the public headers, and
>> >> these aren't C99-compliant.
>> >
>> >I'll remove pragma, together with the definition of struct
>> >xen_9pfs_header: this structure is already defined as part of the 9p
>> >protocol, and it is already mentioned in the Xen 9pfs transport spec as
>> >well. In fact, both QEMU and Linux already have it defined. I don't
>> >think we need it here.
>> 
>> That'll deal with the immediate issue here, but not with the more general
>> implied one: Why would you want to have misaligned fields in a protocol
>> definition?
> 
> Because this header is not actually part of the Xen trasport protocol,
> it is defined by the 9pfs specification. That's why QEMU already had it.
> I cannot do anything about that. I was only redefining it here for
> convenience, because reading the header is required to figure out how
> big is a request (or a response).

If the size is all you care about, perhaps

struct xen_9pfs_header {
	uint8_t size[4];
	uint8_t id[1];
	uint8_t tag[2];
};

would do? (This made me notice you use hard tabs, which you
shouldn't in the Xen tree.)

Jan


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

Re: [PATCH v4 3/4] Introduce the Xen 9pfs transport header

[Stefano Stabellini]

On Wed, 29 Mar 2017, Jan Beulich wrote:
> >>> On 28.03.17 at 23:04, <sstabellini@kernel.org> wrote:
> > On Tue, 28 Mar 2017, Jan Beulich wrote:
> >> >>> Stefano Stabellini <sstabellini@kernel.org> 03/27/17 10:54 PM >>>
> >> >On Mon, 27 Mar 2017, Jan Beulich wrote:
> >> >> >>> On 24.03.17 at 19:31, <sstabellini@kernel.org> wrote:
> >> >> > +/*
> >> >> > + * See docs/misc/9pfs.markdown in xen.git for the full specification:
> >> >> > + * https://xenbits.xen.org/docs/unstable/misc/9pfs.html 
> >> >> > + */
> >> >> > +#pragma pack(push)
> >> >> > +#pragma pack(1)
> >> >> > +struct xen_9pfs_header {
> >> >> > +	uint32_t size;
> >> >> > +	uint8_t id;
> >> >> > +	uint16_t tag;
> >> >> > +};
> >> >> > +#pragma pack(pop)
> >> >> 
> >> >> There's no precedent to using pragmas in the public headers, and
> >> >> these aren't C99-compliant.
> >> >
> >> >I'll remove pragma, together with the definition of struct
> >> >xen_9pfs_header: this structure is already defined as part of the 9p
> >> >protocol, and it is already mentioned in the Xen 9pfs transport spec as
> >> >well. In fact, both QEMU and Linux already have it defined. I don't
> >> >think we need it here.
> >> 
> >> That'll deal with the immediate issue here, but not with the more general
> >> implied one: Why would you want to have misaligned fields in a protocol
> >> definition?
> > 
> > Because this header is not actually part of the Xen trasport protocol,
> > it is defined by the 9pfs specification. That's why QEMU already had it.
> > I cannot do anything about that. I was only redefining it here for
> > convenience, because reading the header is required to figure out how
> > big is a request (or a response).
> 
> If the size is all you care about, perhaps
> 
> struct xen_9pfs_header {
> 	uint8_t size[4];
> 	uint8_t id[1];
> 	uint8_t tag[2];
> };
> 
> would do?

I think it risks causing confusion with the regular header definition,
it's best to drop it. After all, it is specified elsewhere and clearly
mentioned in docs/misc/9pfs.markdown. There won't be any surprises.


> (This made me notice you use hard tabs, which you
> shouldn't in the Xen tree.)

Sorry about the tabs, I checked and none of the other patches have them.

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