Message-ID: <YVMfonwjmbgL/ZCX@kroah.com>
Date:   Tue, 28 Sep 2021 15:58:58 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Pratik Sampat <psampat@...ux.ibm.com>
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 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.

> > > +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,

greg k-h

Powered by blists - more mailing lists