On 08.04.2025 11:59, Anthony PERARD wrote:
> On Mon, Apr 07, 2025 at 05:38:57PM +0200, Jan Beulich wrote:
>> On 07.04.2025 17:23, Anthony PERARD wrote:
>>> On Mon, Apr 07, 2025 at 03:23:48PM +0200, Jan Beulich wrote:
>>>> On 07.04.2025 14:45, Anthony PERARD wrote:
>>>>> Calling xc_get_cpufreq_para with:
>>>>>
>>>>> user_para = {
>>>>> .cpu_num = 0,
>>>>> .freq_num = 0,
>>>>> .gov_num = 9,
>>>>> };
>>>>>
>>>>> seems broken. It's looks like the `scaling_available_governors` bounce
>>>>> buffer is going to be used without been allocated properly handled, with
>>>>> this patch.
>>>>
>>>> The local variable "in_gov_num" controls its allocation and use, together
>>>> with
>>>> has_num. "Use" as in passing to set_xen_guest_handle(). The only further
>>>> use
>>>
>>> When has_num == 0, `in_gov_num` is only used to set `sys_para->gov_num`.
>>> It also make a conditional call to xc_hypercall_bounce_post() but
>>> there's nothing to do.
>>>
>>> Why user_para.gov_num seems to control the size of a buffer, but then
>>> that buffer is just discarded under some condition with this patch?
>>
>> That's nothing this patch changes. Previously has_num would also have been
>> false in the example you give.
>
> Right, sorry. I was persuaded that `has_num` meant "any" of the buffers
> are allocated, instead of the written "all" of them are allocated in C.
> The logic in this function is really hard to follow because it doesn't
> make sense, especially the conditional on `has_num`.
>
> Your patch does make requesting governors actually optional now (and now
> that I realise the calculation of `has_num` doesn't really reflect the
> name). The introduced `in_gov_num` local variable isn't very useful as
> the only real need is in the cleaning path (and we discussed earlier
> that cleaning can be done unconditionally).
Hmm, yes. See below.
> So the patch is fine:
>
> Reviewed-by: Anthony PERARD <anthony.per...@vates.tech>
Thanks.
> Oh, one more thing, it's funny that a lot of faff is done toward making
> the cleaning optional, with all the "unlock_*" label, but then cleaning
> code path can be executed when e.g. cpu_num=0,freq_num=4 (unless the
> hypercall return an error in such case, but the code shouldn't rely on
> that...).
Yeah, perhaps I could have dropped the conditional there, rather than updating
it. Are you happy for me to do so, dropping in_gov_num again (adjusting the
description some, of course)?
Jan
