From 7d08b816b7af3cd415bebf65f44313415fea091a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jos=C3=A9=20Fonseca?= <jrfonseca@tungstengraphics.com>
Date: Sat, 8 Dec 2007 19:21:27 +0000
Subject: mach64: comment bus master / ring buffer behavior and security
---
shared-core/mach64_dma.c | 28 +++++++++++++++++++++++++++-
shared-core/mach64_drv.h | 20 ++++++++++++++++++++
shared-core/mach64_state.c | 14 ++++++++++++++
3 files changed, 61 insertions(+), 1 deletion(-)
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) {
--
cgit v1.2.3