ipc_service: open-amp: Align VRINGs

This patchset is doing three things:

1. It is fixing the bogus algorithm to find the optimal number of
   descriptors for a given memory size.

2. It is changing values for VDEV_STATUS_SIZE and
   IPC_SERVICE_STATIC_VRINGS_ALIGNMENT to better align to a usual cache
   line size.

3. RX/TX VRINGs are now correctly aligned to MEM_ALIGNMENT (and cache
   line alignment).

Signed-off-by: Carlo Caione <[email protected]>

dts/bindings/ipc/zephyr,ipc-openamp-static-vrings.yaml

@@ -48,4 +48,5 @@ properties:
     type: int
     description: |
       The size of the buffer used to send data between host and remote. Default
-      value is RPMSG_BUFFER_SIZE. This property must be the same for host and remote.
+      value is RPMSG_BUFFER_SIZE. This property must be the same for host and
+      remote and preferably a multiple of the cache line size.

include/zephyr/ipc/ipc_static_vrings.h

@@ -25,8 +25,13 @@ extern "C" {
 /** Number of used VRING buffers. */
 #define VRING_COUNT	(2)
 
-/** VRING alignment. */
-#define VRING_ALIGNMENT	CONFIG_IPC_SERVICE_STATIC_VRINGS_ALIGNMENT
+/**
+ * Memory alignment.
+ *
+ * This should take into account the cache line if the cache is enabled, otherwise
+ * it should be naturally aligned to the machine word size.
+ */
+#define MEM_ALIGNMENT	CONFIG_IPC_SERVICE_STATIC_VRINGS_MEM_ALIGNMENT
 
 /**
  * @typedef ipc_notify_cb

subsys/ipc/ipc_service/backends/ipc_rpmsg_static_vrings.c

@@ -250,12 +250,34 @@ static int vr_shm_configure(struct ipc_static_vrings *vr, const struct backend_c
 		return -ENOMEM;
 	}
 
-	vr->shm_addr = conf->shm_addr + VDEV_STATUS_SIZE;
-	vr->shm_size = shm_size(num_desc, conf->buffer_size) - VDEV_STATUS_SIZE;
+	/*
+	 * conf->shm_addr  +--------------+  vr->status_reg_addr
+	 *		   |    STATUS    |
+	 *		   +--------------+  vr->shm_addr
+	 *		   |              |
+	 *		   |              |
+	 *		   |   RX BUFS    |
+	 *		   |              |
+	 *		   |              |
+	 *		   +--------------+
+	 *		   |              |
+	 *		   |              |
+	 *		   |   TX BUFS    |
+	 *		   |              |
+	 *		   |              |
+	 *		   +--------------+  vr->rx_addr (aligned)
+	 *		   |   RX VRING   |
+	 *		   +--------------+  vr->tx_addr (aligned)
+	 *		   |   TX VRING   |
+	 *		   +--------------+
+	 */
+
+	vr->shm_addr = ROUND_UP(conf->shm_addr + VDEV_STATUS_SIZE, MEM_ALIGNMENT);
+	vr->shm_size = shm_size(num_desc, conf->buffer_size);
 
 	vr->rx_addr = vr->shm_addr + VRING_COUNT * vq_ring_size(num_desc, conf->buffer_size);
-	vr->tx_addr = ROUND_UP(vr->rx_addr + vring_size(num_desc, VRING_ALIGNMENT),
-			       VRING_ALIGNMENT);
+	vr->tx_addr = ROUND_UP(vr->rx_addr + vring_size(num_desc, MEM_ALIGNMENT),
+			       MEM_ALIGNMENT);
 
 	vr->status_reg_addr = conf->shm_addr;
 

subsys/ipc/ipc_service/backends/ipc_rpmsg_static_vrings.h

@@ -130,7 +130,10 @@
  *
  */
 
-#define VDEV_STATUS_SIZE	(4) /* Size of status region */
+/*
+ * Size of the status region (possibly a multiple of the cache line size).
+ */
+#define VDEV_STATUS_SIZE	CONFIG_IPC_SERVICE_STATIC_VRINGS_MEM_ALIGNMENT
 
 #define VIRTQUEUE_ID_HOST	(0)
 #define VIRTQUEUE_ID_REMOTE	(1)
@@ -140,24 +143,25 @@
 
 static inline size_t vq_ring_size(unsigned int num, unsigned int buf_size)
 {
-	return (buf_size * num);
+	return ROUND_UP((buf_size * num), MEM_ALIGNMENT);
 }
 
 static inline size_t shm_size(unsigned int num, unsigned int buf_size)
 {
-	return (VDEV_STATUS_SIZE + (VRING_COUNT * vq_ring_size(num, buf_size)) +
-	       (VRING_COUNT * vring_size(num, VRING_ALIGNMENT)));
+	return (VRING_COUNT * (vq_ring_size(num, buf_size) +
+		ROUND_UP(vring_size(num, MEM_ALIGNMENT), MEM_ALIGNMENT)));
 }
 
-static inline unsigned int optimal_num_desc(size_t shm_size, unsigned int buf_size)
+static inline unsigned int optimal_num_desc(size_t mem_size, unsigned int buf_size)
 {
-	size_t available, single_alloc;
-	unsigned int num_desc;
+	size_t available;
+	unsigned int num_desc = 1;
 
-	available = shm_size - VDEV_STATUS_SIZE;
-	single_alloc = VRING_COUNT * (vq_ring_size(1, buf_size) + vring_size(1, VRING_ALIGNMENT));
+	available = mem_size - VDEV_STATUS_SIZE;
 
-	num_desc = (unsigned int) (available / single_alloc);
+	while (available > shm_size(num_desc, buf_size)) {
+		num_desc++;
+	}
 
 	return (1 << (find_msb_set(num_desc) - 1));
 }

subsys/ipc/ipc_service/lib/Kconfig

@@ -11,12 +11,13 @@ config IPC_SERVICE_STATIC_VRINGS
 	help
 	  "Static VRINGs library"
 
-config IPC_SERVICE_STATIC_VRINGS_ALIGNMENT
+config IPC_SERVICE_STATIC_VRINGS_MEM_ALIGNMENT
 	int "VRINGs alignment"
 	depends on IPC_SERVICE_STATIC_VRINGS
 	default 4
 	help
-	  Static VRINGs alignment
+	  Static VRINGs alignment. This should take into account the cache line
+	  alignment if the cache is enabled.
 
 menuconfig IPC_SERVICE_ICMSG
 	bool "icmsg IPC library"

subsys/ipc/ipc_service/lib/ipc_static_vrings.c

@@ -127,13 +127,13 @@ static int vq_setup(struct ipc_static_vrings *vr, unsigned int role)
 	vr->rvrings[RPMSG_VQ_0].io = vr->shm_io;
 	vr->rvrings[RPMSG_VQ_0].info.vaddr = (void *) vr->tx_addr;
 	vr->rvrings[RPMSG_VQ_0].info.num_descs = vr->vring_size;
-	vr->rvrings[RPMSG_VQ_0].info.align = VRING_ALIGNMENT;
+	vr->rvrings[RPMSG_VQ_0].info.align = MEM_ALIGNMENT;
 	vr->rvrings[RPMSG_VQ_0].vq = vr->vq[RPMSG_VQ_0];
 
 	vr->rvrings[RPMSG_VQ_1].io = vr->shm_io;
 	vr->rvrings[RPMSG_VQ_1].info.vaddr = (void *) vr->rx_addr;
 	vr->rvrings[RPMSG_VQ_1].info.num_descs = vr->vring_size;
-	vr->rvrings[RPMSG_VQ_1].info.align = VRING_ALIGNMENT;
+	vr->rvrings[RPMSG_VQ_1].info.align = MEM_ALIGNMENT;
 	vr->rvrings[RPMSG_VQ_1].vq = vr->vq[RPMSG_VQ_1];
 
 	vr->vdev.role = role;