From their introduction all xc_hypercall_bounce_pre() uses, when they
failed, would properly cause exit from the function including cleanup,
yet without informing the caller of the failure. Purge the unlock_1
label for being both pointless and mis-named.
An earlier attempt to switch to the usual split between return value and
errno wasn't quite complete.
HWP work made the cleanup of the "available governors" array
conditional, neglecting the fact that the condition used may not be the
condition that was used to allocate the buffer (as the structure field
is updated upon getting back EAGAIN). Throughout the function, use the
local variable being introduced to address that.
Fixes: 4513025a8790 ("libxc: convert sysctl interfaces over to hypercall buffers")
Amends: 73367cf3b4b4 ("libxc: Fix xc_pm API calls to return negative error and stash error in errno")
Fixes: 31e264c672bc ("pmstat&xenpm: Re-arrage for cpufreq union")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/tools/libs/ctrl/xc_pm.c
+++ b/tools/libs/ctrl/xc_pm.c
@@ -212,31 +212,32 @@ int xc_get_cpufreq_para(xc_interface *xc
DECLARE_NAMED_HYPERCALL_BOUNCE(scaling_available_governors,
user_para->scaling_available_governors,
user_para->gov_num * CPUFREQ_NAME_LEN * sizeof(char), XC_HYPERCALL_BUFFER_BOUNCE_BOTH);
-
- bool has_num = user_para->cpu_num &&
- user_para->freq_num &&
- user_para->gov_num;
+ unsigned int in_gov_num = user_para->gov_num;
+ bool has_num = user_para->cpu_num && user_para->freq_num;
if ( has_num )
{
if ( (!user_para->affected_cpus) ||
(!user_para->scaling_available_frequencies) ||
- (user_para->gov_num && !user_para->scaling_available_governors) )
+ (in_gov_num && !user_para->scaling_available_governors) )
{
errno = EINVAL;
return -1;
}
- if ( xc_hypercall_bounce_pre(xch, affected_cpus) )
- goto unlock_1;
- if ( xc_hypercall_bounce_pre(xch, scaling_available_frequencies) )
+ ret = xc_hypercall_bounce_pre(xch, affected_cpus);
+ if ( ret )
+ return ret;
+ ret = xc_hypercall_bounce_pre(xch, scaling_available_frequencies);
+ if ( ret )
goto unlock_2;
- if ( user_para->gov_num &&
- xc_hypercall_bounce_pre(xch, scaling_available_governors) )
+ if ( in_gov_num )
+ ret = xc_hypercall_bounce_pre(xch, scaling_available_governors);
+ if ( ret )
goto unlock_3;
set_xen_guest_handle(sys_para->affected_cpus, affected_cpus);
set_xen_guest_handle(sys_para->scaling_available_frequencies, scaling_available_frequencies);
- if ( user_para->gov_num )
+ if ( in_gov_num )
set_xen_guest_handle(sys_para->scaling_available_governors,
scaling_available_governors);
}
@@ -246,7 +247,7 @@ int xc_get_cpufreq_para(xc_interface *xc
sysctl.u.pm_op.cpuid = cpuid;
sys_para->cpu_num = user_para->cpu_num;
sys_para->freq_num = user_para->freq_num;
- sys_para->gov_num = user_para->gov_num;
+ sys_para->gov_num = in_gov_num;
ret = xc_sysctl(xch, &sysctl);
if ( ret )
@@ -256,12 +257,11 @@ int xc_get_cpufreq_para(xc_interface *xc
user_para->cpu_num = sys_para->cpu_num;
user_para->freq_num = sys_para->freq_num;
user_para->gov_num = sys_para->gov_num;
- ret = -errno;
}
if ( has_num )
goto unlock_4;
- goto unlock_1;
+ return ret;
}
else
{
@@ -298,13 +298,13 @@ int xc_get_cpufreq_para(xc_interface *xc
}
unlock_4:
- if ( user_para->gov_num )
+ if ( in_gov_num )
xc_hypercall_bounce_post(xch, scaling_available_governors);
unlock_3:
xc_hypercall_bounce_post(xch, scaling_available_frequencies);
unlock_2:
xc_hypercall_bounce_post(xch, affected_cpus);
-unlock_1:
+
return ret;
}On 2025-03-27 09:32, Jan Beulich wrote:
> From their introduction all xc_hypercall_bounce_pre() uses, when they
> failed, would properly cause exit from the function including cleanup,
> yet without informing the caller of the failure. Purge the unlock_1
> label for being both pointless and mis-named.
>
> An earlier attempt to switch to the usual split between return value and
> errno wasn't quite complete.
>
> HWP work made the cleanup of the "available governors" array
> conditional, neglecting the fact that the condition used may not be the
> condition that was used to allocate the buffer (as the structure field
> is updated upon getting back EAGAIN). Throughout the function, use the
> local variable being introduced to address that.
>
> Fixes: 4513025a8790 ("libxc: convert sysctl interfaces over to hypercall buffers")
> Amends: 73367cf3b4b4 ("libxc: Fix xc_pm API calls to return negative error and stash error in errno")
> Fixes: 31e264c672bc ("pmstat&xenpm: Re-arrage for cpufreq union")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
This fixes the currently broken logic with has_num, so I think it's a
good change.
I think the gov_num==0 no longer happens with HWP because it registers a
governor:
return cpufreq_register_governor(&cpufreq_gov_hwp);
Unfortunately, I no longer have an HWP-capable system to verify.
Regards,
Jason
On 27.03.2025 14:32, Jan Beulich wrote:
> From their introduction all xc_hypercall_bounce_pre() uses, when they
> failed, would properly cause exit from the function including cleanup,
> yet without informing the caller of the failure. Purge the unlock_1
> label for being both pointless and mis-named.
>
> An earlier attempt to switch to the usual split between return value and
> errno wasn't quite complete.
>
> HWP work made the cleanup of the "available governors" array
> conditional, neglecting the fact that the condition used may not be the
> condition that was used to allocate the buffer (as the structure field
> is updated upon getting back EAGAIN). Throughout the function, use the
> local variable being introduced to address that.
>
> Fixes: 4513025a8790 ("libxc: convert sysctl interfaces over to hypercall buffers")
> Amends: 73367cf3b4b4 ("libxc: Fix xc_pm API calls to return negative error and stash error in errno")
> Fixes: 31e264c672bc ("pmstat&xenpm: Re-arrage for cpufreq union")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
May I ask for an ack or comments towards what needs changing?
Thanks, Jan
> --- a/tools/libs/ctrl/xc_pm.c
> +++ b/tools/libs/ctrl/xc_pm.c
> @@ -212,31 +212,32 @@ int xc_get_cpufreq_para(xc_interface *xc
> DECLARE_NAMED_HYPERCALL_BOUNCE(scaling_available_governors,
> user_para->scaling_available_governors,
> user_para->gov_num * CPUFREQ_NAME_LEN * sizeof(char), XC_HYPERCALL_BUFFER_BOUNCE_BOTH);
> -
> - bool has_num = user_para->cpu_num &&
> - user_para->freq_num &&
> - user_para->gov_num;
> + unsigned int in_gov_num = user_para->gov_num;
> + bool has_num = user_para->cpu_num && user_para->freq_num;
>
> if ( has_num )
> {
> if ( (!user_para->affected_cpus) ||
> (!user_para->scaling_available_frequencies) ||
> - (user_para->gov_num && !user_para->scaling_available_governors) )
> + (in_gov_num && !user_para->scaling_available_governors) )
> {
> errno = EINVAL;
> return -1;
> }
> - if ( xc_hypercall_bounce_pre(xch, affected_cpus) )
> - goto unlock_1;
> - if ( xc_hypercall_bounce_pre(xch, scaling_available_frequencies) )
> + ret = xc_hypercall_bounce_pre(xch, affected_cpus);
> + if ( ret )
> + return ret;
> + ret = xc_hypercall_bounce_pre(xch, scaling_available_frequencies);
> + if ( ret )
> goto unlock_2;
> - if ( user_para->gov_num &&
> - xc_hypercall_bounce_pre(xch, scaling_available_governors) )
> + if ( in_gov_num )
> + ret = xc_hypercall_bounce_pre(xch, scaling_available_governors);
> + if ( ret )
> goto unlock_3;
>
> set_xen_guest_handle(sys_para->affected_cpus, affected_cpus);
> set_xen_guest_handle(sys_para->scaling_available_frequencies, scaling_available_frequencies);
> - if ( user_para->gov_num )
> + if ( in_gov_num )
> set_xen_guest_handle(sys_para->scaling_available_governors,
> scaling_available_governors);
> }
> @@ -246,7 +247,7 @@ int xc_get_cpufreq_para(xc_interface *xc
> sysctl.u.pm_op.cpuid = cpuid;
> sys_para->cpu_num = user_para->cpu_num;
> sys_para->freq_num = user_para->freq_num;
> - sys_para->gov_num = user_para->gov_num;
> + sys_para->gov_num = in_gov_num;
>
> ret = xc_sysctl(xch, &sysctl);
> if ( ret )
> @@ -256,12 +257,11 @@ int xc_get_cpufreq_para(xc_interface *xc
> user_para->cpu_num = sys_para->cpu_num;
> user_para->freq_num = sys_para->freq_num;
> user_para->gov_num = sys_para->gov_num;
> - ret = -errno;
> }
>
> if ( has_num )
> goto unlock_4;
> - goto unlock_1;
> + return ret;
> }
> else
> {
> @@ -298,13 +298,13 @@ int xc_get_cpufreq_para(xc_interface *xc
> }
>
> unlock_4:
> - if ( user_para->gov_num )
> + if ( in_gov_num )
> xc_hypercall_bounce_post(xch, scaling_available_governors);
> unlock_3:
> xc_hypercall_bounce_post(xch, scaling_available_frequencies);
> unlock_2:
> xc_hypercall_bounce_post(xch, affected_cpus);
> -unlock_1:
> +
> return ret;
> }
>
On Mon, Apr 07, 2025 at 01:38:24PM +0200, Jan Beulich wrote:
> On 27.03.2025 14:32, Jan Beulich wrote:
> > From their introduction all xc_hypercall_bounce_pre() uses, when they
> > failed, would properly cause exit from the function including cleanup,
> > yet without informing the caller of the failure. Purge the unlock_1
> > label for being both pointless and mis-named.
> >
> > An earlier attempt to switch to the usual split between return value and
> > errno wasn't quite complete.
> >
> > HWP work made the cleanup of the "available governors" array
> > conditional, neglecting the fact that the condition used may not be the
> > condition that was used to allocate the buffer (as the structure field
> > is updated upon getting back EAGAIN). Throughout the function, use the
> > local variable being introduced to address that.
> >
> > Fixes: 4513025a8790 ("libxc: convert sysctl interfaces over to hypercall buffers")
> > Amends: 73367cf3b4b4 ("libxc: Fix xc_pm API calls to return negative error and stash error in errno")
> > Fixes: 31e264c672bc ("pmstat&xenpm: Re-arrage for cpufreq union")
> > Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> May I ask for an ack or comments towards what needs changing?
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.
Cheers,
--
Anthony Perard | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
On 07.04.2025 14:45, Anthony PERARD wrote:
> On Mon, Apr 07, 2025 at 01:38:24PM +0200, Jan Beulich wrote:
>> On 27.03.2025 14:32, Jan Beulich wrote:
>>> From their introduction all xc_hypercall_bounce_pre() uses, when they
>>> failed, would properly cause exit from the function including cleanup,
>>> yet without informing the caller of the failure. Purge the unlock_1
>>> label for being both pointless and mis-named.
>>>
>>> An earlier attempt to switch to the usual split between return value and
>>> errno wasn't quite complete.
>>>
>>> HWP work made the cleanup of the "available governors" array
>>> conditional, neglecting the fact that the condition used may not be the
>>> condition that was used to allocate the buffer (as the structure field
>>> is updated upon getting back EAGAIN). Throughout the function, use the
>>> local variable being introduced to address that.
>>>
>>> Fixes: 4513025a8790 ("libxc: convert sysctl interfaces over to hypercall buffers")
>>> Amends: 73367cf3b4b4 ("libxc: Fix xc_pm API calls to return negative error and stash error in errno")
>>> Fixes: 31e264c672bc ("pmstat&xenpm: Re-arrage for cpufreq union")
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> May I ask for an ack or comments towards what needs changing?
>
> 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
of that bounce buffer is on the exit path, i.e. it being passed to
xc_hypercall_bounce_post(). The backing function xc__hypercall_bounce_post()
is dealing fine with the buffer being NULL. And that's what it would be left
at from DECLARE_NAMED_HYPERCALL_BOUNCE() if buffer allocation is skipped.
Jan
On Mon, Apr 07, 2025 at 03:23:48PM +0200, Jan Beulich wrote:
> On 07.04.2025 14:45, Anthony PERARD wrote:
> > On Mon, Apr 07, 2025 at 01:38:24PM +0200, Jan Beulich wrote:
> >> On 27.03.2025 14:32, Jan Beulich wrote:
> >>> From their introduction all xc_hypercall_bounce_pre() uses, when they
> >>> failed, would properly cause exit from the function including cleanup,
> >>> yet without informing the caller of the failure. Purge the unlock_1
> >>> label for being both pointless and mis-named.
> >>>
> >>> An earlier attempt to switch to the usual split between return value and
> >>> errno wasn't quite complete.
> >>>
> >>> HWP work made the cleanup of the "available governors" array
> >>> conditional, neglecting the fact that the condition used may not be the
> >>> condition that was used to allocate the buffer (as the structure field
> >>> is updated upon getting back EAGAIN). Throughout the function, use the
> >>> local variable being introduced to address that.
> >>>
> >>> Fixes: 4513025a8790 ("libxc: convert sysctl interfaces over to hypercall buffers")
> >>> Amends: 73367cf3b4b4 ("libxc: Fix xc_pm API calls to return negative error and stash error in errno")
> >>> Fixes: 31e264c672bc ("pmstat&xenpm: Re-arrage for cpufreq union")
> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >>
> >> May I ask for an ack or comments towards what needs changing?
> >
> > 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?
Is the proposed parameter (where only gov_num is set) a valid input? If
not, why is it not rejected before making the hypercall? (with this
patch)
> of that bounce buffer is on the exit path, i.e. it being passed to
> xc_hypercall_bounce_post(). The backing function xc__hypercall_bounce_post()
> is dealing fine with the buffer being NULL. And that's what it would be left
> at from DECLARE_NAMED_HYPERCALL_BOUNCE() if buffer allocation is skipped.
--
Anthony Perard | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
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:
>>> On Mon, Apr 07, 2025 at 01:38:24PM +0200, Jan Beulich wrote:
>>>> On 27.03.2025 14:32, Jan Beulich wrote:
>>>>> From their introduction all xc_hypercall_bounce_pre() uses, when they
>>>>> failed, would properly cause exit from the function including cleanup,
>>>>> yet without informing the caller of the failure. Purge the unlock_1
>>>>> label for being both pointless and mis-named.
>>>>>
>>>>> An earlier attempt to switch to the usual split between return value and
>>>>> errno wasn't quite complete.
>>>>>
>>>>> HWP work made the cleanup of the "available governors" array
>>>>> conditional, neglecting the fact that the condition used may not be the
>>>>> condition that was used to allocate the buffer (as the structure field
>>>>> is updated upon getting back EAGAIN). Throughout the function, use the
>>>>> local variable being introduced to address that.
>>>>>
>>>>> Fixes: 4513025a8790 ("libxc: convert sysctl interfaces over to hypercall buffers")
>>>>> Amends: 73367cf3b4b4 ("libxc: Fix xc_pm API calls to return negative error and stash error in errno")
>>>>> Fixes: 31e264c672bc ("pmstat&xenpm: Re-arrage for cpufreq union")
>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>
>>>> May I ask for an ack or comments towards what needs changing?
>>>
>>> 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.
> Is the proposed parameter (where only gov_num is set) a valid input? If
> not, why is it not rejected before making the hypercall? (with this
> patch)
Prior to the change adding the user_para->gov_num conditionals the interface
was all or nothing: You got actual data back only if you asked for all three
pieces. Said change then made obtaining the governors optional, yet only
quite. Once obtaining frequencies also becomes optional, I think we will be
in a better position to sanitize this function. Right now I'm only trying to
get some of the basic flaws sorted. The three modes we want/need to support
right now are
- caller wants just counts, but not actual data (would pass in all three
*_num as zero),
- caller wants all data (would pass in all three *_num as non-zero),
- the HWP special case of wanting CPU and frequency data, but not the
(meaningless there) governors.
Jan
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.perard@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
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.perard@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
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.perard@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
On Thu, Mar 27, 2025 at 02:32:04PM +0100, Jan Beulich wrote: > HWP work made the cleanup of the "available governors" array > conditional, neglecting the fact that the condition used may not be the I don't know why the cleanup was made conditional, as long as the bounce buffer is declared with DECLARE_NAMED_HYPERCALL_BOUNCE(), xc_hypercall_bounce_post() can be called without issue (even if ..bounce_pre isn't used). Maybe it's all those "unlock_*" labels that is misleading, a single "out" label which does the cleanup unconditionally would be more than enough. > condition that was used to allocate the buffer (as the structure field > is updated upon getting back EAGAIN). Throughout the function, use the > local variable being introduced to address that. > > --- a/tools/libs/ctrl/xc_pm.c > +++ b/tools/libs/ctrl/xc_pm.c > @@ -212,31 +212,32 @@ int xc_get_cpufreq_para(xc_interface *xc > DECLARE_NAMED_HYPERCALL_BOUNCE(scaling_available_governors, > user_para->scaling_available_governors, > user_para->gov_num * CPUFREQ_NAME_LEN * sizeof(char), XC_HYPERCALL_BUFFER_BOUNCE_BOTH); > - > - bool has_num = user_para->cpu_num && > - user_para->freq_num && > - user_para->gov_num; > + unsigned int in_gov_num = user_para->gov_num; > + bool has_num = user_para->cpu_num && user_para->freq_num; > > if ( has_num ) I think there's an issue here, this condition used to be true if `gov_num` was not 0, even if `cpu_num` and `freq_num` was 0. That's not the case anymore. Shouldn't `has_num` use also the value from `gov_num`? Do we actually need `has_num`? Thanks, -- Anthony Perard | Vates XCP-ng Developer XCP-ng & Xen Orchestra - Vates solutions web: https://vates.tech
On 28.03.2025 11:51, Anthony PERARD wrote: > On Thu, Mar 27, 2025 at 02:32:04PM +0100, Jan Beulich wrote: >> HWP work made the cleanup of the "available governors" array >> conditional, neglecting the fact that the condition used may not be the > > I don't know why the cleanup was made conditional, as long as the bounce > buffer is declared with DECLARE_NAMED_HYPERCALL_BOUNCE(), > xc_hypercall_bounce_post() can be called without issue (even if > ..bounce_pre isn't used). Maybe it's all those "unlock_*" labels that is > misleading, a single "out" label which does the cleanup > unconditionally would be more than enough. Oh, yet more cleanup to do there (independently of course). >> condition that was used to allocate the buffer (as the structure field >> is updated upon getting back EAGAIN). Throughout the function, use the >> local variable being introduced to address that. >> >> --- a/tools/libs/ctrl/xc_pm.c >> +++ b/tools/libs/ctrl/xc_pm.c >> @@ -212,31 +212,32 @@ int xc_get_cpufreq_para(xc_interface *xc >> DECLARE_NAMED_HYPERCALL_BOUNCE(scaling_available_governors, >> user_para->scaling_available_governors, >> user_para->gov_num * CPUFREQ_NAME_LEN * sizeof(char), XC_HYPERCALL_BUFFER_BOUNCE_BOTH); >> - >> - bool has_num = user_para->cpu_num && >> - user_para->freq_num && >> - user_para->gov_num; >> + unsigned int in_gov_num = user_para->gov_num; >> + bool has_num = user_para->cpu_num && user_para->freq_num; >> >> if ( has_num ) > > I think there's an issue here, this condition used to be true if > `gov_num` was not 0, even if `cpu_num` and `freq_num` was 0. That's not > the case anymore. Shouldn't `has_num` use also the value from > `gov_num`? Do we actually need `has_num`? Raising this question is where all of this started: https://lists.xen.org/archives/html/xen-devel/2025-03/msg01870.html. IOW with Penny's change I think the need for has_num will disappear, the latest. At this point, requesting the governors being optional, only ->gov_num shouldn't affect has_num. Once requesting frequencies becomes optional too, has_num would become a mere alias of ->cpu_num. Jan
© 2016 - 2025 Red Hat, Inc.