RE: [PATCH v3 2/3] tools: hv: Add vmbus_bufring

[Saurabh Singh Sengar]

> -----Original Message-----
> From: Michael Kelley (LINUX) <mikelley@xxxxxxxxxxxxx>
> Sent: Thursday, August 3, 2023 3:14 AM
> To: Saurabh Sengar <ssengar@xxxxxxxxxxxxxxxxxxx>; KY Srinivasan
> <kys@xxxxxxxxxxxxx>; Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>;
> wei.liu@xxxxxxxxxx; Dexuan Cui <decui@xxxxxxxxxxxxx>;
> gregkh@xxxxxxxxxxxxxxxxxxx; corbet@xxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> linux-hyperv@xxxxxxxxxxxxxxx; linux-doc@xxxxxxxxxxxxxxx
> Subject: [EXTERNAL] RE: [PATCH v3 2/3] tools: hv: Add vmbus_bufring
> 
> From: Saurabh Sengar <ssengar@xxxxxxxxxxxxxxxxxxx> Sent: Friday, July 14,
> 2023 3:26 AM
> >
> > Provide a userspace interface for userspace drivers or applications to
> > read/write a VMBus ringbuffer. A significant part of this code is
> > borrowed from DPDK[1]. Current library is supported exclusively for
> > the x86 architecture.
> >
> > To build this library:
> > make -C tools/hv libvmbus_bufring.a
> >
> > Applications using this library can include the vmbus_bufring.h header
> > file and libvmbus_bufring.a statically.
> >
> > [1]
> > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgith
> >
> ub.com%2FDPDK%2Fdpdk%2F&data=05%7C01%7Cssengar%40microsoft.com
> %7C7aa6d
> >
> 4dbbcb44895db5008db93a193c9%7C72f988bf86f141af91ab2d7cd011db47%7
> C1%7C0
> >
> %7C638266094508922046%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLj
> AwMDAiLCJQ
> >
> IjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata
> =O0cvl
> > EWlbNS51VoaBHo5l2wWDDjAFJVdfDeT3t%2FR36Y%3D&reserved=0
> >
> > Signed-off-by: Mary Hardy <maryhardy@xxxxxxxxxxxxx>
> > Signed-off-by: Saurabh Sengar <ssengar@xxxxxxxxxxxxxxxxxxx>
> > ---
> > [V3]
> > - Made ring buffer data offset depend on page size
> > - remove rte_smp_rwmb macro and reused rte_compiler_barrier instead
> > - Added legal counsel sign-off
> > - Removed "Link:" tag
> > - Improve commit messages
> > - new library compilation dependent on x86
> > - simplify mmap
> >
> > [V2]
> > - simpler sysfs path, less parsing
> >
> >  tools/hv/Build           |   1 +
> >  tools/hv/Makefile        |  13 +-
> >  tools/hv/vmbus_bufring.c | 297
> > +++++++++++++++++++++++++++++++++++++++
> >  tools/hv/vmbus_bufring.h | 154 ++++++++++++++++++++
> >  4 files changed, 464 insertions(+), 1 deletion(-)  create mode 100644
> > tools/hv/vmbus_bufring.c  create mode 100644 tools/hv/vmbus_bufring.h
> >
> > diff --git a/tools/hv/Build b/tools/hv/Build index
> > 6cf51fa4b306..2a667d3d94cb 100644
> > --- a/tools/hv/Build
> > +++ b/tools/hv/Build
> > @@ -1,3 +1,4 @@
> >  hv_kvp_daemon-y += hv_kvp_daemon.o
> >  hv_vss_daemon-y += hv_vss_daemon.o
> >  hv_fcopy_daemon-y += hv_fcopy_daemon.o
> > +vmbus_bufring-y += vmbus_bufring.o
> > diff --git a/tools/hv/Makefile b/tools/hv/Makefile index
> > fe770e679ae8..33cf488fd20f 100644
> > --- a/tools/hv/Makefile
> > +++ b/tools/hv/Makefile
> > @@ -11,14 +11,19 @@ srctree := $(patsubst %/,%,$(dir $(CURDIR)))
> > srctree := $(patsubst %/,%,$(dir $(srctree)))  endif
> >
> > +include $(srctree)/tools/scripts/Makefile.arch
> > +
> >  # Do not use make's built-in rules
> >  # (this improves performance and avoids hard-to-debug behaviour);
> > MAKEFLAGS += -r
> >
> >  override CFLAGS += -O2 -Wall -g -D_GNU_SOURCE -I$(OUTPUT)include
> >
> > +ifeq ($(SRCARCH),x86)
> > +ALL_LIBS := libvmbus_bufring.a
> > +endif
> >  ALL_TARGETS := hv_kvp_daemon hv_vss_daemon hv_fcopy_daemon
> > -ALL_PROGRAMS := $(patsubst %,$(OUTPUT)%,$(ALL_TARGETS))
> > +ALL_PROGRAMS := $(patsubst %,$(OUTPUT)%,$(ALL_TARGETS)) $(patsubst
> > %,$(OUTPUT)%,$(ALL_LIBS))
> >
> >  ALL_SCRIPTS := hv_get_dhcp_info.sh hv_get_dns_info.sh
> > hv_set_ifconfig.sh
> >
> > @@ -27,6 +32,12 @@ all: $(ALL_PROGRAMS)  export srctree OUTPUT CC LD
> > CFLAGS  include $(srctree)/tools/build/Makefile.include
> >
> > +HV_VMBUS_BUFRING_IN := $(OUTPUT)vmbus_bufring.o
> > +$(HV_VMBUS_BUFRING_IN): FORCE
> > +	$(Q)$(MAKE) $(build)=vmbus_bufring
> > +$(OUTPUT)libvmbus_bufring.a : vmbus_bufring.o
> > +	$(AR) rcs $@ $^
> > +
> >  HV_KVP_DAEMON_IN := $(OUTPUT)hv_kvp_daemon-in.o
> >  $(HV_KVP_DAEMON_IN): FORCE
> >  	$(Q)$(MAKE) $(build)=hv_kvp_daemon
> > diff --git a/tools/hv/vmbus_bufring.c b/tools/hv/vmbus_bufring.c new
> > file mode 100644 index 000000000000..fb1f0489c625
> > --- /dev/null
> > +++ b/tools/hv/vmbus_bufring.c
> > @@ -0,0 +1,297 @@
> > +// SPDX-License-Identifier: BSD-3-Clause
> > +/*
> > + * Copyright (c) 2009-2012,2016,2023 Microsoft Corp.
> > + * Copyright (c) 2012 NetApp Inc.
> > + * Copyright (c) 2012 Citrix Inc.
> > + * All rights reserved.
> > + */
> > +
> > +#include <errno.h>
> > +#include <emmintrin.h>
> > +#include <stdio.h>
> > +#include <string.h>
> > +#include <unistd.h>
> > +#include <sys/uio.h>
> > +#include "vmbus_bufring.h"
> > +
> > +#define	rte_compiler_barrier()	({ asm volatile ("" : : : "memory"); })
> > +#define RINGDATA_START_OFFSET	(getpagesize())
> > +#define VMBUS_RQST_ERROR	0xFFFFFFFFFFFFFFFF
> > +#define ALIGN(val, align)	((typeof(val))((val) & (~((typeof(val))((align) -
> 1)))))
> > +
> > +/* Increase bufring index by inc with wraparound */ static inline
> > +uint32_t vmbus_br_idxinc(uint32_t idx, uint32_t inc, uint32_t sz) {
> > +	idx += inc;
> > +	if (idx >= sz)
> > +		idx -= sz;
> > +
> > +	return idx;
> > +}
> > +
> > +void vmbus_br_setup(struct vmbus_br *br, void *buf, unsigned int
> > +blen) {
> > +	br->vbr = buf;
> > +	br->windex = br->vbr->windex;
> > +	br->dsize = blen - RINGDATA_START_OFFSET; }
> > +
> > +static inline __always_inline void
> > +rte_smp_mb(void)
> > +{
> > +	asm volatile("lock addl $0, -128(%%rsp); " ::: "memory"); }
> > +
> > +static inline int
> > +rte_atomic32_cmpset(volatile uint32_t *dst, uint32_t exp, uint32_t
> > +src) {
> > +	uint8_t res;
> > +
> > +	asm volatile("lock ; "
> > +		     "cmpxchgl %[src], %[dst];"
> > +		     "sete %[res];"
> > +		     : [res] "=a" (res),     /* output */
> > +		     [dst] "=m" (*dst)
> > +		     : [src] "r" (src),      /* input */
> > +		     "a" (exp),
> > +		     "m" (*dst)
> > +		     : "memory");            /* no-clobber list */
> > +	return res;
> > +}
> > +
> > +static inline uint32_t
> > +vmbus_txbr_copyto(const struct vmbus_br *tbr, uint32_t windex,
> > +		  const void *src0, uint32_t cplen) {
> > +	uint8_t *br_data = (uint8_t *)tbr->vbr + RINGDATA_START_OFFSET;
> > +	uint32_t br_dsize = tbr->dsize;
> > +	const uint8_t *src = src0;
> > +
> > +	if (cplen > br_dsize - windex) {
> > +		uint32_t fraglen = br_dsize - windex;
> > +
> > +		/* Wrap-around detected */
> > +		memcpy(br_data + windex, src, fraglen);
> > +		memcpy(br_data, src + fraglen, cplen - fraglen);
> > +	} else {
> > +		memcpy(br_data + windex, src, cplen);
> > +	}
> > +
> > +	return vmbus_br_idxinc(windex, cplen, br_dsize); }
> > +
> > +/*
> > + * Write scattered channel packet to TX bufring.
> > + *
> > + * The offset of this channel packet is written as a 64bits value
> > + * immediately after this channel packet.
> > + *
> > + * The write goes through three stages:
> > + *  1. Reserve space in ring buffer for the new data.
> > + *     Writer atomically moves priv_write_index.
> > + *  2. Copy the new data into the ring.
> > + *  3. Update the tail of the ring (visible to host) that indicates
> > + *     next read location. Writer updates write_index
> > + */
> > +static int
> > +vmbus_txbr_write(struct vmbus_br *tbr, const struct iovec iov[], int iovlen,
> > +		 bool *need_sig)
> > +{
> > +	struct vmbus_bufring *vbr = tbr->vbr;
> > +	uint32_t ring_size = tbr->dsize;
> > +	uint32_t old_windex, next_windex, windex, total;
> > +	uint64_t save_windex;
> > +	int i;
> > +
> > +	total = 0;
> > +	for (i = 0; i < iovlen; i++)
> > +		total += iov[i].iov_len;
> > +	total += sizeof(save_windex);
> > +
> > +	/* Reserve space in ring */
> > +	do {
> > +		uint32_t avail;
> > +
> > +		/* Get current free location */
> > +		old_windex = tbr->windex;
> > +
> > +		/* Prevent compiler reordering this with calculation */
> > +		rte_compiler_barrier();
> > +
> > +		avail = vmbus_br_availwrite(tbr, old_windex);
> > +
> > +		/* If not enough space in ring, then tell caller. */
> > +		if (avail <= total)
> > +			return -EAGAIN;
> > +
> > +		next_windex = vmbus_br_idxinc(old_windex, total, ring_size);
> > +
> > +		/* Atomic update of next write_index for other threads */
> > +	} while (!rte_atomic32_cmpset(&tbr->windex, old_windex,
> > +next_windex));
> > +
> > +	/* Space from old..new is now reserved */
> > +	windex = old_windex;
> > +	for (i = 0; i < iovlen; i++)
> > +		windex = vmbus_txbr_copyto(tbr, windex, iov[i].iov_base,
> > +iov[i].iov_len);
> > +
> > +	/* Set the offset of the current channel packet. */
> > +	save_windex = ((uint64_t)old_windex) << 32;
> > +	windex = vmbus_txbr_copyto(tbr, windex, &save_windex,
> > +				   sizeof(save_windex));
> > +
> > +	/* The region reserved should match region used */
> > +	if (windex != next_windex)
> > +		return -EINVAL;
> > +
> > +	/* Ensure that data is available before updating host index */
> > +	rte_compiler_barrier();
> > +
> > +	/* Checkin for our reservation. wait for our turn to update host */
> > +	while (!rte_atomic32_cmpset(&vbr->windex, old_windex,
> next_windex))
> > +		_mm_pause();
> > +
> > +	return 0;
> > +}
> > +
> > +int rte_vmbus_chan_send(struct vmbus_br *txbr, uint16_t type, void
> *data,
> > +			uint32_t dlen, uint32_t flags)
> > +{
> > +	struct vmbus_chanpkt pkt;
> > +	unsigned int pktlen, pad_pktlen;
> > +	const uint32_t hlen = sizeof(pkt);
> > +	bool send_evt = false;
> > +	uint64_t pad = 0;
> > +	struct iovec iov[3];
> > +	int error;
> > +
> > +	pktlen = hlen + dlen;
> > +	pad_pktlen = ALIGN(pktlen, sizeof(uint64_t));
> 
> This ALIGN function rounds down.  So pad_pktlen could be less than pktlen.

Thanks for pointing this, will fix.

> 
> > +
> > +	pkt.hdr.type = type;
> > +	pkt.hdr.flags = flags;
> > +	pkt.hdr.hlen = hlen >> VMBUS_CHANPKT_SIZE_SHIFT;
> > +	pkt.hdr.tlen = pad_pktlen >> VMBUS_CHANPKT_SIZE_SHIFT;
> > +	pkt.hdr.xactid = VMBUS_RQST_ERROR; /* doesn't support multiple
> > +requests at same time */
> > +
> > +	iov[0].iov_base = &pkt;
> > +	iov[0].iov_len = hlen;
> > +	iov[1].iov_base = data;
> > +	iov[1].iov_len = dlen;
> > +	iov[2].iov_base = &pad;
> > +	iov[2].iov_len = pad_pktlen - pktlen;
> 
> Given the way your ALIGN function works, the above could produce a
> negative value for iov[2].iov_len.  Then bad things will happen. :-(

Got it.

> 
> > +
> > +	error = vmbus_txbr_write(txbr, iov, 3, &send_evt);
> > +
> > +	return error;
> > +}
> > +

<snip>

> > +	 * we can request that the receiver interrupt the sender
> > +	 * when the ring transitions from being full to being able
> > +	 * to handle a message of size "pending_send_sz".
> > +	 *
> > +	 * Add necessary state for this enhancement.
> > +	 */
> > +	volatile uint32_t pending_send;
> > +	uint32_t reserved1[12];
> > +
> > +	union {
> > +		struct {
> > +			uint32_t feat_pending_send_sz:1;
> > +		};
> > +		uint32_t value;
> > +	} feature_bits;
> > +
> > +	/*
> > +	 * Ring data starts here + RingDataStartOffset
> 
> This mention of RingDataStartOffset looks stale.  I could not find it defined
> anywhere.

Will correct it to:
Ring data starts after PAGE_SIZE offset from the start of this struct (RINGDATA_START_OFFSET).

- Saurabh