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

[Chris Wilson]

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.

> +			}
> +
> +			memcpy(dst_data_ptr, src_data_ptr, buffer_size);

Where do you validate that buffer_size is sane before copying?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre