Message-ID: <1d8e82ab-fab7-d014-c812-2c086dd7a63f@linux.ibm.com>
Date:   Tue, 28 Sep 2021 19:41:44 +0530
From:   Pratik Sampat <psampat@...ux.ibm.com>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     mpe@...erman.id.au, benh@...nel.crashing.org, paulus@...ba.org,
        shuah@...nel.org, farosas@...ux.ibm.com, kjain@...ux.ibm.com,
        linuxppc-dev@...ts.ozlabs.org, kvm-ppc@...r.kernel.org,
        linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org,
        pratik.r.sampat@...il.com
Subject: Re: [PATCH v8 1/2] powerpc/pseries: Interface to represent PAPR
 firmware attributes



On 28/09/21 7:28 pm, Greg KH wrote:
> On Tue, Sep 28, 2021 at 06:13:18PM +0530, Pratik Sampat wrote:
>> Hello Greg,
>>
>> Thank you for your review.
>>
>> On 28/09/21 5:38 pm, Greg KH wrote:
>>> On Tue, Sep 28, 2021 at 05:21:01PM +0530, Pratik R. Sampat wrote:
>>>> Adds a generic interface to represent the energy and frequency related
>>>> PAPR attributes on the system using the new H_CALL
>>>> "H_GET_ENERGY_SCALE_INFO".
>>>>
>>>> H_GET_EM_PARMS H_CALL was previously responsible for exporting this
>>>> information in the lparcfg, however the H_GET_EM_PARMS H_CALL
>>>> will be deprecated P10 onwards.
>>>>
>>>> The H_GET_ENERGY_SCALE_INFO H_CALL is of the following call format:
>>>> hcall(
>>>>     uint64 H_GET_ENERGY_SCALE_INFO,  // Get energy scale info
>>>>     uint64 flags,           // Per the flag request
>>>>     uint64 firstAttributeId,// The attribute id
>>>>     uint64 bufferAddress,   // Guest physical address of the output buffer
>>>>     uint64 bufferSize       // The size in bytes of the output buffer
>>>> );
>>>>
>>>> This H_CALL can query either all the attributes at once with
>>>> firstAttributeId = 0, flags = 0 as well as query only one attribute
>>>> at a time with firstAttributeId = id, flags = 1.
>>>>
>>>> The output buffer consists of the following
>>>> 1. number of attributes              - 8 bytes
>>>> 2. array offset to the data location - 8 bytes
>>>> 3. version info                      - 1 byte
>>>> 4. A data array of size num attributes, which contains the following:
>>>>     a. attribute ID              - 8 bytes
>>>>     b. attribute value in number - 8 bytes
>>>>     c. attribute name in string  - 64 bytes
>>>>     d. attribute value in string - 64 bytes
>>>>
>>>> The new H_CALL exports information in direct string value format, hence
>>>> a new interface has been introduced in
>>>> /sys/firmware/papr/energy_scale_info to export this information to
>>>> userspace in an extensible pass-through format.
>>>>
>>>> The H_CALL returns the name, numeric value and string value (if exists)
>>>>
>>>> The format of exposing the sysfs information is as follows:
>>>> /sys/firmware/papr/energy_scale_info/
>>>>      |-- <id>/
>>>>        |-- desc
>>>>        |-- value
>>>>        |-- value_desc (if exists)
>>>>      |-- <id>/
>>>>        |-- desc
>>>>        |-- value
>>>>        |-- value_desc (if exists)
>>>> ...
>>>>
>>>> The energy information that is exported is useful for userspace tools
>>>> such as powerpc-utils. Currently these tools infer the
>>>> "power_mode_data" value in the lparcfg, which in turn is obtained from
>>>> the to be deprecated H_GET_EM_PARMS H_CALL.
>>>> On future platforms, such userspace utilities will have to look at the
>>>> data returned from the new H_CALL being populated in this new sysfs
>>>> interface and report this information directly without the need of
>>>> interpretation.
>>>>
>>>> Signed-off-by: Pratik R. Sampat <psampat@...ux.ibm.com>
>>>> Reviewed-by: Gautham R. Shenoy <ego@...ux.vnet.ibm.com>
>>>> Reviewed-by: Fabiano Rosas <farosas@...ux.ibm.com>
>>>> Reviewed-by: Kajol Jain <kjain@...ux.ibm.com>
>>>> ---
>>>>    .../sysfs-firmware-papr-energy-scale-info     |  26 ++
>>>>    arch/powerpc/include/asm/hvcall.h             |  24 +-
>>>>    arch/powerpc/kvm/trace_hv.h                   |   1 +
>>>>    arch/powerpc/platforms/pseries/Makefile       |   3 +-
>>>>    .../pseries/papr_platform_attributes.c        | 312 ++++++++++++++++++
>>>>    5 files changed, 364 insertions(+), 2 deletions(-)
>>>>    create mode 100644 Documentation/ABI/testing/sysfs-firmware-papr-energy-scale-info
>>>>    create mode 100644 arch/powerpc/platforms/pseries/papr_platform_attributes.c
>>>>
>>>> diff --git a/Documentation/ABI/testing/sysfs-firmware-papr-energy-scale-info b/Documentation/ABI/testing/sysfs-firmware-papr-energy-scale-info
>>>> new file mode 100644
>>>> index 000000000000..139a576c7c9d
>>>> --- /dev/null
>>>> +++ b/Documentation/ABI/testing/sysfs-firmware-papr-energy-scale-info
>>>> @@ -0,0 +1,26 @@
>>>> +What:		/sys/firmware/papr/energy_scale_info
>>>> +Date:		June 2021
>>>> +Contact:	Linux for PowerPC mailing list <linuxppc-dev@...abs.org>
>>>> +Description:	Directory hosting a set of platform attributes like
>>>> +		energy/frequency on Linux running as a PAPR guest.
>>>> +
>>>> +		Each file in a directory contains a platform
>>>> +		attribute hierarchy pertaining to performance/
>>>> +		energy-savings mode and processor frequency.
>>>> +
>>>> +What:		/sys/firmware/papr/energy_scale_info/<id>
>>>> +		/sys/firmware/papr/energy_scale_info/<id>/desc
>>>> +		/sys/firmware/papr/energy_scale_info/<id>/value
>>>> +		/sys/firmware/papr/energy_scale_info/<id>/value_desc
>>>> +Date:		June 2021
>>>> +Contact:	Linux for PowerPC mailing list <linuxppc-dev@...abs.org>
>>>> +Description:	Energy, frequency attributes directory for POWERVM servers
>>>> +
>>>> +		This directory provides energy, frequency, folding information. It
>>>> +		contains below sysfs attributes:
>>>> +
>>>> +		- desc: String description of the attribute <id>
>>>> +
>>>> +		- value: Numeric value of attribute <id>
>>>> +
>>>> +		- value_desc: String value of attribute <id>
>>> Can you just make 4 different entries in this file, making it easier to
>>> parse and extend over time?
>> Do you mean I only create one file per attribute and populate it with 4
>> different entries as follows?
>>
>> # cat /sys/firmware/papr/energy_scale_info/<id>
>> id:
>> desc:
>> value:
>> value_desc:
> No, I mean in this documentation file, have 4 different "What:" entries,
> don't lump 4 of them together into one larger Description for no reason
> like you did here.
>
> The sysfs files themselves are fine.

Ah okay, I understand what you're saying. I just need to make 4 different
entries in the documentation.
Thanks for that clarification.

>>>> +struct papr_attr {
>>>> +	u64 id;
>>>> +	struct kobj_attribute kobj_attr;
>>> Why does an attribute have to be part of this structure?
>> I bundled both an attribute as well as its ID in a structure because each
>> attributes value could only be queried from the firmware with the corresponding
>> ID.
>> It seemed to be logically connected and that's why I had them in the structure.
>> Are you suggesting we maintain them separately and don't need the coupling?
> The id is connected to the kobject, not the attribute, right?
> Attributes do not have uniqueness like this normally.
>
>
>>>> +static struct papr_ops_info {
>>>> +	const char *attr_name;
>>>> +	ssize_t (*show)(struct kobject *kobj, struct kobj_attribute *kobj_attr,
>>>> +			char *buf);
>>>> +} ops_info[MAX_ATTRS] = {
>>>> +	{ "desc", papr_show_desc },
>>>> +	{ "value", papr_show_value },
>>>> +	{ "value_desc", papr_show_value_desc },
>>> What is wrong with just using the __ATTR_RO() macro and then having an
>>> array of attributes in a single group?  That should be a lot simpler
>>> overall, right?
>> If I understand this correctly, you mean I can have a array of attributes in a
>> flat single group?
> Yes.
>
>> I suppose that would be a simpler, given your earlier suggestion to wrap
>> attribute values up in a single file per attribute.
>>
>> However, the intent of grouping and keeping files separate was that each sysfs
>> file has only one value to display.
> That is correct, and not a problem here at all.
>
>> I can change it to using an array of attributes in a single group too if you
>> believe that is right way to go instead.
> You have 3 variables for your attributes:
>
> static struct kobj_attribute papr_desc = __ATTR_RO(desc);
> static struct kobj_attribute papr_value = __ATTR_RO(value);
> static struct kobj_attribute papr_value_desc = __ATTR_RO(value_desc);
>
> and then your attribute group:
> static struct attribute papr_attrs[] = {
> 	&papr_desc.attr,
> 	&papr_value.attr,
> 	&papr_value_desc.attr,
> 	NULL,
> };
>
> ATTRIBUTE_GROUPS(papr);
>
> Then take that papr_groups and register that with the kobject when
> needed.
>
> But, you seem to only be having a whole kobject for a subdirectory,
> right?  No need for that, just name your attribute group, so instead of
>
> ATTRIBUTE_GROUPS(papr);
>
> do:
> static const struct attribute_group papr_group = {
> 	.name = "Your Subdirectory Name here",
> 	.attrs = papr_attrs,
> };
>
> Hope this helps,

Yes, this does!
I understand now that a whole kobject for a sub-directory is futile.
The approach you suggested for having papr_groups register with the kobject
whenever needed is more cleaner.

Thanks for the help, I'll rework my current logic according to that.

Pratik

> greg k-h

Powered by blists - more mailing lists