| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <87tum6vb58.fsf@linux.ibm.com>
Date: Wed, 09 Jun 2021 21:03:31 -0300
From: Fabiano Rosas <farosas@...ux.ibm.com>
To: Pratik Sampat <psampat@...ux.ibm.com>, mpe@...erman.id.au,
benh@...nel.crashing.org, paulus@...ba.org,
linuxppc-dev@...ts.ozlabs.org, kvm-ppc@...r.kernel.org,
linux-kernel@...r.kernel.org, pratik.r.sampat@...il.com
Subject: Re: [RFC] powerpc/pseries: Interface to represent PAPR firmware
attributes
Pratik Sampat <psampat@...ux.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. “flags”:
> a. Bit 0: singleAttribute
> If set to 1, only return the single attribute matching firstAttributeId.
> b. Bits 1-63: Reserved
> 2. “firstAttributeId”: The first attribute to retrieve
> 3. “bufferAddress”: The logical real address of the start of the output buffer
> 4. “bufferSize”: The size in bytes of the output buffer
>
>
> From the document, the format of the output buffer is as follows:
>
> Table 1 --> output buffer
> ================================================================================
> | Field Name | Byte | Length | Description
> | | Offset | in Bytes |
> ================================================================================
> | NumberOf | | | Number of Attributes in Buffer
> | 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 changes
> | | | | will not impact the Array of
> | | | | attributes.
> | | | | Current version = 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
> ================================================================================
> | Field Name | Byte | Length | Description
> | | Offset | in Bytes |
> ================================================================================
> | 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 AttributeValue.
> --------------------------------------------------------------------------------
> | .... | | |
>
>
>>
>>> 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 convention.
> 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 interface
> to housing the sensor information.
>
> One alternative to add more structure to this format could be to introduce:
> 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 exists
> 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 but
> 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 information.
> The alternative I suggested a few sentences above to include ID based attribute
> 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@...ux.ibm.com>
>
> Thanks
> Pratik
Powered by blists - more mailing lists