Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Fabiano Rosas <farosas@linux.ibm.com>
To:     Pratik Sampat <psampat@linux.ibm.com>, mpe@ellerman.id.au,
        benh@kernel.crashing.org, paulus@samba.org,
        linuxppc-dev@lists.ozlabs.org, kvm-ppc@vger.kernel.org,
        linux-kernel@vger.kernel.org, pratik.r.sampat@gmail.com
Subject: Re: [RFC] powerpc/pseries: Interface to represent PAPR firmware
 attributes
In-Reply-To: <5c9cb57b-e9d8-0361-8be7-60dc9618db34@linux.ibm.com>
References: <20210604163501.51511-1-psampat@linux.ibm.com>
 <87wnr4uhs9.fsf@linux.ibm.com>
 <5c9cb57b-e9d8-0361-8be7-60dc9618db34@linux.ibm.com>
Date:   Wed, 09 Jun 2021 21:03:31 -0300
Message-ID: <87tum6vb58.fsf@linux.ibm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Pratik Sampat <psampat@linux.ibm.com> writes:

>>> 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
>> Is this new hypercall already present in the spec? These seem a bit
>> underspecified to me.
>
> Yes, it is present in the spec. I probably summarized a little more than =
needed
> here and I could expand upon below.
>
> The input buffer recives the following data:
>
> 1. =E2=80=9Cflags=E2=80=9D:
> 	a. Bit 0: singleAttribute
> 		If set to 1, only return the single attribute matching firstAttributeId.
> 	b. Bits 1-63: Reserved
> 2. =E2=80=9CfirstAttributeId=E2=80=9D: The first attribute to retrieve
> 3. =E2=80=9CbufferAddress=E2=80=9D: The logical real address of the start=
 of the output buffer
> 4. =E2=80=9CbufferSize=E2=80=9D: The size in bytes of the output buffer
>=20=09
>
>  From the document, the format of the output buffer is as follows:
>
> Table 1 --> output buffer
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D
> | Field Name           | Byte   | Length   |  Description
> |                      | Offset | in Bytes |
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D
> | NumberOf             |        |          | Number of Attributes in Buff=
er
> | AttributesInBuffer   | 0x000  | 0x08     |
> -------------------------------------------------------------------------=
-------
> | AttributeArrayOffset | 0x008  | 0x08     | Byte offset to start of Array
> |                      |        |          | of Attributes
> |                      |        |          |
> -------------------------------------------------------------------------=
-------
> | OutputBufferData     |        |          | Version of the Header.
> | HeaderVersion        | 0x010  | 0x01     | The header will be always
> | AttributesInBuffer   |        |          | backward compatible, and cha=
nges
> |                      |        |          | will not impact the Array of
> |                      |        |          | attributes.
> |                      |        |          | Current version =3D 0x01

This is not clear to me. In the event of a header version change, is the
total set of attributes guaranteed to remain the same? Or only the array
layout? We might not need to expose the version information after all.

> -------------------------------------------------------------------------=
-------
> | ArrayOfAttributes    |        |          | The array will contain
> |                      |        |          | "NumberOfAttributesInBuffer"
> |                      |        |          | array elements not to exceed
> |                      |        |          | the size of the buffer.
> |                      |        |          | Layout of the array is
> |                      |        |          | detailed in Table 2.
> -------------------------------------------------------------------------=
-------
>
>
> Table 2 --> Array of attributes
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D
> | Field Name           | Byte   | Length   |  Description
> |                      | Offset | in Bytes |
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D
> | 1st AttributeId      | 0x000  | 0x08     | The ID of the Attribute
> -------------------------------------------------------------------------=
-------
> | 1st AttributeValue   | 0x008  | 0x08     | The numerical value of
> |                      |        |          | the attribute
> -------------------------------------------------------------------------=
-------
> | 1st AttributeString  | 0x010  | 0x40     | The ASCII string
> | Description          |        |          | description of the
> |                      |        |          | attribute, up to 63
> |                      |        |          | characters plus a NULL
> |                      |        |          | terminator.

There is a slight disconnect in that this is called "description" by the
spec, which makes me think they could eventually have something more
verbose than what you'd expect from "name".

So they could give us either: "Frequency" or "The Frequency in GigaHertz".

> -------------------------------------------------------------------------=
-------
> | 1st AttributeValue   | 0x050  | 0x40     | The ASCII string
> | StringDescription    |        |          | description of the
> |                      |        |          | attribute value, up to 63
> |                      |        |          | characters plus a NULL
> |                      |        |          | terminator. If this
> |                      |        |          | contains only a NULL
> |                      |        |          | terminator, then there is
> |                      |        |          | no ASCII string
> |                      |        |          | associated with AttributeVal=
ue.
> -------------------------------------------------------------------------=
-------
> | ....                 |        |          |
>
>
>>
>>> The new H_CALL exports information in direct string value format, hence
>>> a new interface has been introduced in /sys/firmware/papr to export
>> Hm.. Maybe this should be something less generic than "papr"?
>
> The interface naming was inspired from /sys/firmware/opal's naming conven=
tion.
> We believed the name PAPR could serve as more generic name to be used by =
both
> Linux running on PHYP and linux on KVM.

Right, I agree with that rationale, but /opal has identifiable elements
in it whereas /papr would have the generic "attr_X_name", which does not
give much hint about what they are.

We also expect people to iterate the "attr_X_*" files, so if we decide
to add something else under /papr in the future, that would potentially
cause issues with any tool that just lists the content of the directory.

So maybe we should be proactive and put the hcall stuff inside a
subdirectory already. /papr/energy_scale_attrs comes to mind, but I
don't have a strong opinion on the particular name.

>
> If you have something more concrete in mind, please let me know. I'm open=
 to
> suggestions.
>
>>
>>> this information to userspace in an extensible pass-through format.
>>> The H_CALL returns the name, numeric value and string value. As string
>>> values are in human readable format, therefore if the string value
>>> exists then that is given precedence over the numeric value.
>> So the hypervisor could simply not send the string representation? How
>> will the userspace tell the difference since they are reading everything
>> from a file?
>>
>> Overall I'd say we should give the data in a more structured way and let
>> the user-facing tool do the formatting and presentation.
>
> That's a valid concern, the design for this was inspired from hwmon's int=
erface
> to housing the sensor information.
>
> One alternative to add more structure to this format could be to introduc=
e:
> attr_X_name, attr_X_num_val, attr_X_str_val
>
> However, in some cases like min/max frequency the string value is empty. =
In
> that case the file attr_X_str_val will also be empty.
> Is that an acceptable format of having empty files that in some cases will
> never be populated?

I'm thinking yes, but I'm not sure. Let's see if someone else has a say
in this.

> We also went ahead to confirm with the SPEC team that if a string value e=
xists
> in their buffer, that must be given precedence.

Huh.. That must be a recommendation only. The hypervisor has no control
over how people present the information in userspace.

>
> Another alternative format could to keep attr_X_name, attr_X_val intact b=
ut
> change what X means. Currently X is just an iteratively increasing number=
. But
> X can also serve as an ID which we get from H_CALL output buffer.

This seems like a good idea. It makes it easier to correlate the
attribute with what is in PAPR.

>
> In this case, we should also include some versioning so that the tool now=
 also
> has cognizance of contents of each file.
>
>>> The format of exposing the sysfs information is as follows:
>>> /sys/firmware/papr/
>>>    |-- attr_0_name
>>>    |-- attr_0_val
>>>    |-- attr_1_name
>>>    |-- attr_1_val
>>> ...
>> How do we keep a stable interface with userspace? Say the hypervisor
>> decides to add or remove attributes, change their order, string
>> representation, etc? It will inform us via the version field, but that
>> is lost when we output this to sysfs.
>>
>> I get that if the userspace just iterate over the contents of the
>> directory then nothing breaks, but there is not much else it could do it
>> seems.
>
> Fair point, having the version exposed to the sysfs does seem crucial.
>
> Currently in ppc-utils we iterate over all the information, however as you
> rightly pointed out there may be other tools needing just specific inform=
ation.
> The alternative I suggested a few sentences above to include ID based att=
ribute
> naming and versioning maybe a more elegant way of solving this problem.
>
> What are your thoughts on a design like this?
>

Based on all the new information you provided, I'd say present all the
data and group it under the ID:

/sys/firmware/papr/energy_scale_attrs/
   |-- <id>/
     |-- desc
     |-- value
     |-- value_desc
   |-- <id>/
     |-- desc
     |-- value
     |-- value_desc

Is that workable?

>>> 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@linux.ibm.com>
>
> Thanks
> Pratik