Re: [PATCH v3 09/14 RESEND] xenpm: Print HWP parameters

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.05.2023 20:11, Jason Andryuk wrote:
> On Mon, May 8, 2023 at 6:43 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>> On 01.05.2023 21:30, Jason Andryuk wrote:
>>> --- a/tools/misc/xenpm.c
>>> +++ b/tools/misc/xenpm.c
>>> @@ -708,6 +708,44 @@ void start_gather_func(int argc, char *argv[])
>>>      pause();
>>>  }
>>>
>>> +static void calculate_hwp_activity_window(const xc_hwp_para_t *hwp,
>>> +                                          unsigned int *activity_window,
>>> +                                          const char **units)
>>> +{
>>> +    unsigned int mantissa = hwp->activity_window & 
>>> HWP_ACT_WINDOW_MANTISSA_MASK;
>>> +    unsigned int exponent =
>>> +        (hwp->activity_window >> HWP_ACT_WINDOW_EXPONENT_SHIFT) &
>>> +            HWP_ACT_WINDOW_EXPONENT_MASK;
>>
>> I wish we had MASK_EXTR() in common-macros.h. While really a comment on
>> patch 7 - HWP_ACT_WINDOW_EXPONENT_SHIFT is redundant information and
>> should imo be omitted from the public interface, in favor of just a
>> (suitably shifted) mask value. Also note how those constants all lack
>> proper XEN_ prefixes.
> 
> I'll add a patch adding MASK_EXTR() & MASK_INSR() to common-macros.h
> and use those - is there any reason not to do that?

I don't think there is, but I'm also not a maintainer of that code.

>>> +    unsigned int multiplier = 1;
>>> +    unsigned int i;
>>> +
>>> +    if ( hwp->activity_window == 0 )
>>> +    {
>>> +        *units = "hardware selected";
>>> +        *activity_window = 0;
>>> +
>>> +        return;
>>> +    }
>>
>> While in line with documentation, any mantissa of 0 results in a 0us
>> window, which I assume would then also mean "hardware selected".
> 
> I hadn't considered that.  The hardware seems to allow you to write a
> 0 mantissa, non-0 exponent.  From the SDM, it's unclear what that
> would mean.  The code as written would display "0 us", "0 ms", or "0
> s" - not "0 hardware selected".  Do you want more explicity printing
> for those cases?  I think it's fine to have a distinction between the
> output.  "0 hardware selected" is the known valid value that is
> working as expected.  The other ones being something different seems
> good to me since we don't really know what they mean.

Keeping things - apart from perhaps adding a respective comment - is
okay, as long as we don't know any better.

Jan