On Tue, Apr 08, 2025 at 12:47:58PM +0200, Jan Beulich wrote:
> 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)?
Yes, sounds good, thanks.
--
Anthony Perard | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
