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

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

On 8/15/2016 10:26 PM, Chris Wilson wrote:
> 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@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).
>
It would have been ideal if log packets can be dumped without any 
interpretation.

We copy the payload without any interpretation, only some bits of header 
we parse.

We also have to interpret the header (in subsequent patch) to copy only 
the updated payload data, for better performance.

>>>> +			}
>>>> +
>>>> +			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.
Actually if buffer_size has been validated & corrected, then both 
src_data_ptr + buffer_size and dst_data_ptr + buffer_size should be 
valid. Both buffers have been allocated by Driver.

Will avoid reading the buffer_size from the header and just move the 
pointer & do the copy as per the expected buffer size, which cannot go 
wrong (out of bounds) as Driver only allocated the log buffer.

Best regards
Akash

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