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). So the patch is fine:
Reviewed-by: Anthony PERARD <anthony.per...@vates.tech>
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...).
Thanks,
--
Anthony Perard | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
