mach64: comment bus master / ring buffer behavior and security

3 files changed, 61 insertions, 1 deletions

diff --git a/shared-core/mach64_dma.c b/shared-core/mach64_dma.c
index 9aa3f768..411b98d5 100644
--- a/shared-core/mach64_dma.c
+++ b/shared-core/mach64_dma.c
@@ -562,6 +562,14 @@ void mach64_dump_ring_info(drm_mach64_private_t * dev_priv)
 /** \name DMA descriptor ring macros */
 /*@{*/
 
+/**
+ * Add the end mark to the ring's new tail position.
+ * 
+ * The bus master engine will keep processing the DMA buffers listed in the ring
+ * until it finds this mark, making it stop.
+ * 
+ * \sa mach64_clear_dma_eol
+ */ 
 static __inline__ void mach64_set_dma_eol(volatile u32 * addr)
 {
 #if defined(__i386__)
@@ -602,6 +610,17 @@ static __inline__ void mach64_set_dma_eol(volatile u32 * addr)
 #endif
 }
 
+/**
+ * Remove the end mark from the ring's old tail position.  
+ * 
+ * It should be called after calling mach64_set_dma_eol to mark the ring's new
+ * tail position.
+ * 
+ * We update the end marks while the bus master engine is in operation. Since  
+ * the bus master engine may potentially be reading from the same position
+ * that we write, we must change atomically to avoid having intermediary bad 
+ * data.
+ */
 static __inline__ void mach64_clear_dma_eol(volatile u32 * addr)
 {
 #if defined(__i386__)
@@ -691,7 +710,9 @@ do {									\
 	mach64_ring_tick( dev_priv, &(dev_priv)->ring );		\
 } while (0)
 
-
+/**
+ * Queue a DMA buffer of registers writes into the ring buffer.
+ */ 
 int mach64_add_buf_to_ring(drm_mach64_private_t *dev_priv,
                            drm_mach64_freelist_t *entry)
 {
@@ -734,6 +755,11 @@ int mach64_add_buf_to_ring(drm_mach64_private_t *dev_priv,
 	return 0;
 }
 
+/**
+ * Queue DMA buffer controlling host data tranfers (e.g., blit).
+ * 
+ * Almost identical to mach64_add_buf_to_ring.
+ */
 int mach64_add_hostdata_buf_to_ring(drm_mach64_private_t *dev_priv,
                                     drm_mach64_freelist_t *entry)
 {
diff --git a/shared-core/mach64_drv.h b/shared-core/mach64_drv.h
index 7bd40a68..1768a2a4 100644
--- a/shared-core/mach64_drv.h
+++ b/shared-core/mach64_drv.h
@@ -527,6 +527,9 @@ extern void mach64_driver_irq_uninstall(struct drm_device * dev);
 
 /* ================================================================
  * Ring operations
+ *
+ * Since the Mach64 bus master engine requires polling, these functions end
+ * up being called frequently, hence being inline.
  */
 
 static __inline__ void mach64_ring_start(drm_mach64_private_t * dev_priv)
@@ -591,6 +594,18 @@ static __inline__ void mach64_ring_resume(drm_mach64_private_t * dev_priv,
 	}
 }
 
+/**
+ * Poll the ring head and make sure the bus master is alive.
+ * 
+ * Mach64's bus master engine will stop if there are no more entries to process.
+ * This function polls the engine for the last processed entry and calls 
+ * mach64_ring_resume if there is an unprocessed entry.
+ * 
+ * Note also that, since we update the ring tail while the bus master engine is 
+ * in operation, it is possible that the last tail update was too late to be 
+ * processed, and the bus master engine stops at the previous tail position. 
+ * Therefore it is important to call this function frequently. 
+ */
 static __inline__ void mach64_ring_tick(drm_mach64_private_t * dev_priv,
 					drm_mach64_descriptor_ring_t * ring)
 {
@@ -676,6 +691,11 @@ mach64_update_ring_snapshot(drm_mach64_private_t * dev_priv)
 
 /* ================================================================
  * DMA macros
+ * 
+ * Mach64's ring buffer doesn't take register writes directly. These 
+ * have to be written indirectly in DMA buffers. These macros simplify 
+ * the task of setting up a buffer, writing commands to it, and 
+ * queuing the buffer in the ring. 
  */
 
 #define DMALOCALS				\
diff --git a/shared-core/mach64_state.c b/shared-core/mach64_state.c
index 6fcae948..88ff4843 100644
--- a/shared-core/mach64_state.c
+++ b/shared-core/mach64_state.c
@@ -575,6 +575,10 @@ static int mach64_dma_dispatch_vertex(struct drm_device * dev,
 		return -EAGAIN;
 	}
 
+	/* Mach64's vertex data is actually register writes. To avoid security
+	 * compromises these register writes have to be verified and copied from
+	 * user space into a private DMA buffer.
+	 */
 	verify_ret = copy_from_user_vertex(GETBUFPTR(copy_buf), buf, used);
 
 	if (verify_ret != 0) {
@@ -698,6 +702,16 @@ static int mach64_dma_dispatch_blit(struct drm_device * dev,
 		return -EAGAIN;
 	}
 
+	/* Copy the blit data from userspace.
+	 * 
+	 * XXX: This is overkill. The most efficient solution would be having 
+	 * two sets of buffers (one set private for vertex data, the other set 
+	 * client-writable for blits). However that would bring more complexity 
+	 * and would break backward compatability. The solution currently 
+	 * implemented is keeping all buffers private, allowing to secure the
+	 * driver, without increasing complexity at the expense of some speed 
+	 * transfering data.
+	 */
 	verify_ret = copy_from_user_blit(GETBUFPTR(copy_buf), blit->buf, used);
 
 	if (verify_ret != 0) {