Re: [PATCH v4 12/15] xen: Add SET_CPUFREQ_HWP xen_sysctl_pm_op

[Jan Beulich <jbeulich@xxxxxxxx>]

(re-adding xen-devel@)

On 21.06.2023 16:16, Jason Andryuk wrote:
> On Mon, Jun 19, 2023 at 10:47 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>> On 14.06.2023 20:02, Jason Andryuk wrote:
>>> +    if ( !(set_cppc->set_params & XEN_SYSCTL_CPPC_SET_ACT_WINDOW) &&
>>> +         set_cppc->activity_window )
>>> +        return -EINVAL;
> 
> There were a few aspects intended to be checked, but I have failed to
> implement them all properly and consistently.  The 32bit fields of the
> CPPC interface are larger than the 8 bit HWP fields (10 bits for
> activity window).  So the first aspect was supposed to ensure all
> those out-of-range bits are 0.
> 
> The second aspect, which wasn't implemented properly, was that fields
> would be 0 unless the corresponding bit was set in set_params.
> 
> The third aspect was to fail if a field was specified but hardware
> support isn't available.  That is now only activity window.
> 
> Do aspects #1 and #2 sound appropriate?  We can discuss #3 below.

Personally I'd prefer if inapplicable fields weren't checked, unless we
expect re-use of those fields with a different way of indicating that
the field holds an applicable value. But my primary desire is for
checking to be as consistent as possible.

>> Feels like I have wondered before what good this check does. I'm
>> inclined to suggest to ...
> 
> This check was supposed to enforce #2.
> 
>>> +    if ( set_cppc->activity_window & ~XEN_SYSCTL_CPPC_ACT_WINDOW_MASK )
>>> +        return -EINVAL;
>>
>> ... fold the two relevant checks, omitting the middle one:
>>
>>     if ( (set_cppc->set_params & XEN_SYSCTL_CPPC_SET_ACT_WINDOW) &&
>>          (!feature_hwp_activity_window ||
>>           (set_cppc->activity_window & ~XEN_SYSCTL_CPPC_ACT_WINDOW_MASK))
>>         return -EINVAL;
>>
>> Yet I'm also a little worried about the feature check, requiring the
>> caller to first figure out whether that feature is available. Would
>> it be an alternative to make such "best effort", preferably with
>> some indication that this aspect of the request was not carried out?
> 
> Yes, it would be nice to try and apply on a "best effort" basis as
> it's only activity window which may not be supported.
> 
> The SDM says, "Processors may support a subset of IA32_HWP_REQUEST
> fields as indicated by CPUID. Reads of non-supported fields will
> return 0. Writes to non-supported fields are ignored."
> 
> I'll have to test this, but potentially we just let the writes go
> through?  If the user checks xenpm, they will see that the activity
> window isn't supported?  Hmmm, I don't have a machine without activity
> window support, so I can't test it.  Skylake introduced HWP, but my
> skylake test system supports activity window.
> 
> Or do you want to make xen_set_cppc_para have an in/out and return the
> applied features?

Yes, that was what I meant with "indication of some sort". You could
e.g. simply clear the respective control bit in the request (and then
arrange for it to be copied back).

Jan