[Intel-gfx] [PATCH 1/3] drm/i915/guc: Limit scheduling properties to avoid overflow

[John Harrison]

On 3/2/2022 01:20, Tvrtko Ursulin wrote:
> On 01/03/2022 19:57, John Harrison wrote:
>> On 3/1/2022 02:50, Tvrtko Ursulin wrote:
>>> On 28/02/2022 18:32, John Harrison wrote:
>>>> On 2/28/2022 08:11, Tvrtko Ursulin wrote:
>>>>> On 25/02/2022 17:39, John Harrison wrote:
>>>>>> On 2/25/2022 09:06, Tvrtko Ursulin wrote:
>>>>>>>
>>>>>>> On 24/02/2022 19:19, John Harrison wrote:
>>>>>>>
>>>>>>> [snip]
>>>>>>>
>>>>>>>>>>>>> ./gt/uc/intel_guc_fwif.h: u32 execution_quantum;
>>>>>>>>>>>>>
>>>>>>>>>>>>> ./gt/uc/intel_guc_submission.c: desc->execution_quantum = 
>>>>>>>>>>>>> engine->props.timeslice_duration_ms * 1000;
>>>>>>>>>>>>>
>>>>>>>>>>>>> ./gt/intel_engine_types.h: unsigned long 
>>>>>>>>>>>>> timeslice_duration_ms;
>>>>>>>>>>>>>
>>>>>>>>>>>>> timeslice_store/preempt_timeout_store:
>>>>>>>>>>>>> err = kstrtoull(buf, 0, &duration);
>>>>>>>>>>>>>
>>>>>>>>>>>>> So both kconfig and sysfs can already overflow GuC, not 
>>>>>>>>>>>>> only because of tick conversion internally but because at 
>>>>>>>>>>>>> backend level nothing was done for assigning 64-bit into 
>>>>>>>>>>>>> 32-bit. Or I failed to find where it is handled.
>>>>>>>>>>>> That's why I'm adding this range check to make sure we 
>>>>>>>>>>>> don't allow overflows.
>>>>>>>>>>>
>>>>>>>>>>> Yes and no, this fixes it, but the first bug was not only 
>>>>>>>>>>> due GuC internal tick conversion. It was present ever since 
>>>>>>>>>>> the u64 from i915 was shoved into u32 sent to GuC. So even 
>>>>>>>>>>> if GuC used the value without additional multiplication, bug 
>>>>>>>>>>> was be there. My point being when GuC backend was added 
>>>>>>>>>>> timeout_ms values should have been limited/clamped to 
>>>>>>>>>>> U32_MAX. The tick discovery is additional limit on top.
>>>>>>>>>> I'm not disagreeing. I'm just saying that the truncation 
>>>>>>>>>> wasn't noticed until I actually tried using very long 
>>>>>>>>>> timeouts to debug a particular problem. Now that it is 
>>>>>>>>>> noticed, we need some method of range checking and this 
>>>>>>>>>> simple clamp solves all the truncation problems.
>>>>>>>>>
>>>>>>>>> Agreed in principle, just please mention in the commit message 
>>>>>>>>> all aspects of the problem.
>>>>>>>>>
>>>>>>>>> I think we can get away without a Fixes: tag since it requires 
>>>>>>>>> user fiddling to break things in unexpected ways.
>>>>>>>>>
>>>>>>>>> I would though put in a code a clamping which expresses both, 
>>>>>>>>> something like min(u32, ..GUC LIMIT..). So the full story is 
>>>>>>>>> documented forever. Or "if > u32 || > ..GUC LIMIT..) return 
>>>>>>>>> -EINVAL". Just in case GuC limit one day changes but u32 
>>>>>>>>> stays. Perhaps internal ticks go away or anything and we are 
>>>>>>>>> left with plain 1:1 millisecond relationship.
>>>>>>>> Can certainly add a comment along the lines of "GuC API only 
>>>>>>>> takes a 32bit field but that is further reduced to GUC_LIMIT 
>>>>>>>> due to internal calculations which would otherwise overflow".
>>>>>>>>
>>>>>>>> But if the GuC limit is > u32 then, by definition, that means 
>>>>>>>> the GuC API has changed to take a u64 instead of a u32. So 
>>>>>>>> there will no u32 truncation any more. So I'm not seeing a need 
>>>>>>>> to explicitly test the integer size when the value check covers 
>>>>>>>> that.
>>>>>>>
>>>>>>> Hmm I was thinking if the internal conversion in the GuC fw 
>>>>>>> changes so that GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS goes above 
>>>>>>> u32, then to be extra safe by documenting in code there is the 
>>>>>>> additional limit of the data structure field. Say the field was 
>>>>>>> changed to take some unit larger than a millisecond. Then the 
>>>>>>> check against the GuC MAX limit define would not be enough, 
>>>>>>> unless that would account both for internal implementation and 
>>>>>>> u32 in the protocol. Maybe that is overdefensive but I don't see 
>>>>>>> that it harms. 50-50, but it's do it once and forget so I'd do it.
>>>>>> Huh?
>>>>>>
>>>>>> How can the limit be greater than a u32 if the interface only 
>>>>>> takes a u32? By definition the limit would be clamped to u32 size.
>>>>>>
>>>>>> If you mean that the GuC policy is in different units and those 
>>>>>> units might not overflow but ms units do, then actually that is 
>>>>>> already the case. The GuC works in us not ms. That's part of why 
>>>>>> the wrap around is so low, we have to multiply by 1000 before 
>>>>>> sending to GuC. However, that is actually irrelevant because the 
>>>>>> comparison is being done on the i915 side in i915's units. We 
>>>>>> have to scale the GuC limit to match what i915 is using. And the 
>>>>>> i915 side is u64 so if the scaling to i915 numbers overflows a 
>>>>>> u32 then who cares because that comparison can be done at 64 bits 
>>>>>> wide.
>>>>>>
>>>>>> If the units change then that is a backwards breaking API change 
>>>>>> that will require a manual driver code update. You can't just 
>>>>>> recompile with a new header and magically get an ms to us or ms 
>>>>>> to s conversion in your a = b assignment. The code will need to 
>>>>>> be changed to do the new unit conversion (note we already convert 
>>>>>> from ms to us, the GuC API is all expressed in us). And that code 
>>>>>> change will mean having to revisit any and all scaling, type 
>>>>>> conversions, etc. I.e. any pre-existing checks will not 
>>>>>> necessarily be valid and will need to be re-visted anyway. But as 
>>>>>> above, any scaling to GuC units has to be incorporated into the 
>>>>>> limit already because otherwise the limit would not fit in the 
>>>>>> GuC's own API.
>>>>>
>>>>> Yes I get that, I was just worried that u32 field in the protocol 
>>>>> and GUC_POLICY_MAX_EXEC_QUANTUM_MS defines are separate in the 
>>>>> source code and then how to protect against forgetting to update 
>>>>> both in sync.
>>>>>
>>>>> Like if the protocol was changed to take nanoseconds, and firmware 
>>>>> implementation changed to support the full range, but define 
>>>>> left/forgotten at 100s. That would then overflow u32.
>>>> Huh? If the API was updated to 'support the full range' then how 
>>>> can you get overflow by forgetting to update the limit? You could 
>>>> get unnecessary clamping, which hopefully would be noticed by 
>>>> whoever is testing the new API and/or whoever requested the change. 
>>>> But you can't get u32 overflow errors if all the code has been 
>>>> updated to u64.
>>>
>>> 1)
>>> Change the protocol so that "u32 desc->execution_quantum" now takes 
>>> nano seconds.
>>>
>>> This now makes the maximum time 4.29.. seconds.
>> You seriously think this is likely to happen?
>>
>> That the GuC people would force an API change on us that is 
>> completely backwards from what we have been asking? And that such a 
>> massive backwards step would not get implemented correctly because 
>> someone didn't notice just how huge an impact it was?
>
> I don't know what we have been asking or what GuC people would do.
Despite the views of some in the community, the GuC team are not evil 
monsters out for world domination. We are their customers and their task 
is to provide a usable offload device that makes the Linux experience 
better not worse.

Just from this discussion alone, ignoring any internal forums, it has 
been made clear that the (long standing) request from the i915 team is 
to support 64bit policy values and (more recently) to officially 
document any and all limits involved in the policies. By definition, 
that also means that there would be significant push back and argument 
if the GuC team proposed making this interface worse.

>
>>> 2)
>>> Forget to update GUC_POLICY_MAX_EXEC_QUANTUM_MS from 100s, since for 
>>> instance that part at that point still not part of the interface 
>>> contract.
>> There is zero chance of the us -> ns change occurring in the 
>> foreseeable future whereas the expectation is to have the limits be 
>> part of the spec in the next firmware release. So this scenario is 
>> just not going to happen. And as above, it would be such a big change 
>> with such a huge amount of push back and discussion going on that it 
>> would be impossible for the limit update to be missed/forgotten.
>>
>>>
>>> 3)
>>> User passes in 5 seconds.
>>>
>>> Clamping check says all is good.
>>>
>>> "engine->props.timeslice_duration_ms > GUC_POLICY_MAX_EXEC_QUANTUM_MS"
>>>
>>> 4)
>>>
>>> Assignment was updated:
>>>
>>> gt/uc/intel_guc_submission.c:
>>>
>>>   desc->execution_quantum = engine->props.timeslice_duration_ms * 1e6;
>>>
>>> But someone did not realize field is u32.
>>>
>>>   desc->execution_quantum = engine->props.timeslice_duration_ms * 1e6;
>>>
>>> Defensive solution:
>>>
>>>   if (overflows_type(engine->props.timeslice_duration_ms * 1e6, 
>>> desc->execution_quantum))
>>>     drm_WARN_ON...
>>
>> All you are saying is that bugs can happen. The above is just one 
>> more place to have a bug.
>>
>> The purpose of the limit is to take into account all reasons for 
>> there being a limit. Having a bunch of different tests that are all 
>> testing the same thing is pointless.
>
> I am saying this:
>
> 1)
> The code I pointed out is a boundary layer between two components 
> which have independent design and development teams.
>
> 2)
> The limit in question is currently not explicitly defined by the 
> interface provider.
>
> 3)
> The limit in question is also implicitly defined by the hidden 
> internal firmware implementation details not relating to the units of 
> the interface.
>
> 4)
> The source code location of the clamping check is far away (different 
> file, different layer) from the assignment to the interface data 
> structure.
>
> From this it sounds plausible to me to have the check at the 
> assignment site and don't have to think about it further.
It also sounds plausible to use the concept of consolidation. Rather 
than scattering random different limit tests in random different places, 
it all goes into a single helper function that can be used at the top 
level and report any range issues before you get to the lower levels 
where errors might not be allowed. This was your own feedback (and is 
currently implemented in the v2 post).

John.

>
> Regards,
>
> Tvrtko