[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v8 1/4] ring.h: introduce macros to handle monodirectional rings with multiple req sizes
On Tue, 4 Apr 2017, Konrad Rzeszutek Wilk wrote: > On Fri, Mar 31, 2017 at 12:15:16PM -0700, Stefano Stabellini wrote: > > 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@xxxxxxxxxxx> > > CC: konrad.wilk@xxxxxxxxxx > > CC: JBeulich@xxxxxxxx > > --- > > xen/include/public/io/ring.h | 151 > > +++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 151 insertions(+) > > > > diff --git a/xen/include/public/io/ring.h b/xen/include/public/io/ring.h > > index 801c0da..7b7794c 100644 > > --- a/xen/include/public/io/ring.h > > +++ b/xen/include/public/io/ring.h > > @@ -27,6 +27,21 @@ > > #ifndef __XEN_PUBLIC_IO_RING_H__ > > #define __XEN_PUBLIC_IO_RING_H__ > > > > +/* > > + * When #include'ing this header, you need to provide the following > > + * declaration upfront: > > + * - standard integers types (uint8_t, uint16_t, etc) > > + * They are provided by stdint.h of the standard headers. > > + * > > + * In addition, if you intend to use the FLEX macros, you also need to > > + * provide the following, before invoking the FLEX macros: > > + * - size_t > > + * - memcpy > > + * - grant_ref_t > > + * These declarations are provided by string.h of the standard headers, > > + * and grant_table.h from the Xen public headers. > > + */ > > + > > #include "../xen-compat.h" > > > > #if __XEN_INTERFACE_VERSION__ < 0x00030208 > > @@ -313,6 +328,142 @@ 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()). > > s/ring_size/XEN_FLEX_RING_SIZE/ ? OK, I'll write (XEN_FLEX_RING_SIZE() - $NAME_queued()). I think that also adding all the parameters would be too wordy. > > + */ > > + > > +#define XEN_PAGE_SHIFT 12 > > Perhaps a comment explaining why you have that? It looks very architecture > dependent? > > Or perhaps you could do with > > #ifndef XEN_PAGE_SHIFT > /* > * Hypercall interface only supports only 4Kb page unless the architecture > * overrides it. > */ I don't think there are any architecture overrides actually. On ARM is always defined as 4K, no matter the choice of page granularity made by the operating system. > #define XEN_PAGE_SHIFT 12 > #endif > > ? I'll add the #ifndef to avoid possible collisions and add this comment: The PAGE_SIZE (thus, also PAGE_SHIFT) for ring protocols and hypercall interfaces is always 4K, regardless of the architecture and page granularity chosen by operating systems. > > +#define XEN_FLEX_RING_SIZE(order) > > \ > > + (1UL << ((order) + XEN_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 unsigned char *name##_get_ring_ptr(unsigned char *buf, > > \ > > + RING_IDX idx, > > \ > > + RING_IDX ring_size) > > \ > > +{ > > \ > > + return buf + name##_mask(idx, ring_size); > > \ > > +} > > \ > > + > > \ > > +static inline void name##_read_packet(void *opaque, > > \ > > + const unsigned char *buf, > > \ > > + size_t size, > > \ > > + RING_IDX masked_prod, > > \ > > + RING_IDX *masked_cons, > > \ > > + RING_IDX ring_size) > > \ > > +{ > > \ > > + if (*masked_cons < masked_prod || > > \ > > + size <= ring_size - *masked_cons) { > > \ > > 'size <= ..' is not aligned here while it is in _write_packet? > > Besides that it looks fine to me. I'll align > > + 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, > > \ > > + const void *opaque, > > \ > > + size_t size, > > \ > > + RING_IDX *masked_prod, > > \ > > + RING_IDX masked_cons, > > \ > > + RING_IDX ring_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); > > \ > > +} > > \ > > + > > \ > > +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; > > \ > > +} > > \ > > + > > \ > > +struct name##_data { > > \ > > + unsigned char *in; /* half of the allocation */ > > \ > > + unsigned char *out; /* half of the allocation */ > > \ > > +} > > + > > +#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@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |