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

["Goel, Akash" <akash.goel@intel.com>]

On 8/15/2016 9:36 PM, Chris Wilson wrote:
> On Mon, Aug 15, 2016 at 08:19:47PM +0530, akash.goel@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.

Please suggest that what should be done here ideally.

Should the further copying (for this snapshot) be skipped ?

>> +			}
>> +
>> +			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.

Best regards
Akash
> -Chris
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx