[Intel-gfx] [PATCH 06/18] drm/i915: Handle log buffer flush interrupt event from GuC

[Chris Wilson]

On Mon, Aug 15, 2016 at 10:16:56PM +0530, Goel, Akash wrote:
> 
> 
> On 8/15/2016 9:36 PM, Chris Wilson wrote:
> >On Mon, Aug 15, 2016 at 08:19:47PM +0530, akash.goel at intel.com wrote:
> >>+static void guc_read_update_log_buffer(struct intel_guc *guc)
> >>+{
> >>+	struct guc_log_buffer_state *log_buffer_state, *log_buffer_snapshot_state;
> >>+	struct guc_log_buffer_state log_buffer_state_local;
> >>+	void *src_data_ptr, *dst_data_ptr;
> >>+	unsigned int buffer_size, expected_size;
> >>+	enum guc_log_buffer_type type;
> >>+
> >>+	if (WARN_ON(!guc->log.buf_addr))
> >>+		return;
> >>+
> >>+	/* Get the pointer to shared GuC log buffer */
> >>+	log_buffer_state = src_data_ptr = guc->log.buf_addr;
> >>+
> >>+	/* Get the pointer to local buffer to store the logs */
> >>+	dst_data_ptr = log_buffer_snapshot_state = guc_get_write_buffer(guc);
> >>+
> >>+	/* Actual logs are present from the 2nd page */
> >>+	src_data_ptr += PAGE_SIZE;
> >>+	dst_data_ptr += PAGE_SIZE;
> >>+
> >>+	for (type = GUC_ISR_LOG_BUFFER; type < GUC_MAX_LOG_BUFFER; type++) {
> >>+		/* Make a copy of the state structure in GuC log buffer (which
> >>+		 * is uncached mapped) on the stack to avoid reading from it
> >>+		 * multiple times.
> >>+		 */
> >>+		memcpy(&log_buffer_state_local, log_buffer_state,
> >>+		       sizeof(struct guc_log_buffer_state));
> >>+		buffer_size = log_buffer_state_local.size;
> >>+
> >>+		if (log_buffer_snapshot_state) {
> >>+			/* First copy the state structure in snapshot buffer */
> >>+			memcpy(log_buffer_snapshot_state, &log_buffer_state_local,
> >>+			       sizeof(struct guc_log_buffer_state));
> >>+
> >>+			/* The write pointer could have been updated by the GuC
> >>+			 * firmware, after sending the flush interrupt to Host,
> >>+			 * for consistency set the write pointer value to same
> >>+			 * value of sampled_write_ptr in the snapshot buffer.
> >>+			 */
> >>+			log_buffer_snapshot_state->write_ptr =
> >>+				log_buffer_snapshot_state->sampled_write_ptr;
> >>+
> >>+			log_buffer_snapshot_state++;
> >>+
> >>+			/* Now copy the actual logs, but before that validate
> >>+			 * the buffer size value retrieved from state structure.
> >>+			 */
> >>+			if (type == GUC_ISR_LOG_BUFFER)
> >>+				expected_size = (GUC_LOG_ISR_PAGES+1)*PAGE_SIZE;
> >>+			else if (type == GUC_DPC_LOG_BUFFER)
> >>+				expected_size = (GUC_LOG_DPC_PAGES+1)*PAGE_SIZE;
> >>+			else
> >>+				expected_size = (GUC_LOG_CRASH_PAGES+1)*PAGE_SIZE;
> >>+
> >>+			if (unlikely(buffer_size != expected_size)) {
> >>+				DRM_ERROR("unexpected log buffer size\n");
> >>+				/* Continue with further copying, already state
> >>+				 * structure has been copied which is enough to
> >>+				 * let Userspace know about the anomaly.
> >>+				 */
> >>+				buffer_size = expected_size;
> >
> >Urm, no.
> >
> >You tell userspace one thing and then do another. This code should just
> >be a conduit and not apply its own outdated interpretation.
> >
> Userspace parser would get to know from the state structure about
> the anomalous buffer size.

It will, but it won't be told what the kernel did. So if believes the
GuC (as it should since it is a packet that should be unadulterated) the
entire stream is then corrupt.

> Please suggest that what should be done here ideally.
> 
> Should the further copying (for this snapshot) be skipped ?

The kernel should be avoiding interpretting the log packets as much as
possible - I would prefer it if we just moved the byte stream without
trying to interpret it as datagrams. But there is probably some merit to
at least using the log packets (datagrams).

> >>+			}
> >>+
> >>+			memcpy(dst_data_ptr, src_data_ptr, buffer_size);
> >
> >Where do you validate that buffer_size is sane before copying?
> Sorry didn't get you, the check for buffer_size is being done right
> before this memcpy.

There is no explicit check for valid src_data_ptr + buffer_size or
dst_data_ptr + buffer_size, and a quick glance at the code suggested no
reason to believe they must be valid.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre