Re: [PATCH v8 1/2] powerpc/pseries: Interface to represent PAPR firmware attributes
From: Pratik Sampat
Date: Tue Sep 28 2021 - 10:12:25 EST
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@xxxxxxxxxxxxx>
Reviewed-by: Gautham R. Shenoy <ego@xxxxxxxxxxxxxxxxxx>
Reviewed-by: Fabiano Rosas <farosas@xxxxxxxxxxxxx>
Reviewed-by: Kajol Jain <kjain@xxxxxxxxxxxxx>
---
.../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@xxxxxxxxxx>
+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@xxxxxxxxxx>
+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