lkml.org 
[lkml]   [2021]   [Jul]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 1/1] powerpc/pseries: Interface to represent PAPR firmware attributes
From
Date


On 08/07/21 4:05 pm, Gautham R Shenoy wrote:
> Hello Pratik,
>
> On Tue, Jul 06, 2021 at 01:54:00PM +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)
>> ...
>
> I like this implementation. Only one comment w.r.t versioning below:
>
> [..snip..]
>
>> @@ -631,6 +632,26 @@ struct hv_gpci_request_buffer {
>> uint8_t bytes[HGPCI_MAX_DATA_BYTES];
>> } __packed;
>>
>> +#define MAX_ESI_ATTRS 10
>> +#define MAX_BUF_SZ (sizeof(struct h_energy_scale_info_hdr) + \
>> + (sizeof(struct energy_scale_attribute) * MAX_ESI_ATTRS))
>> +
>> +struct energy_scale_attribute {
>> + __be64 id;
>> + __be64 value;
>> + unsigned char desc[64];
>> + unsigned char value_desc[64];
>> +} __packed;
>> +
>> +struct h_energy_scale_info_hdr {
>> + __be64 num_attrs;
>> + __be64 array_offset;
>> + __u8 data_header_version;
>> +} __packed;
>> +
>
> [..snip..]
>
>> +static int __init papr_init(void)
>> +{
>> + uint64_t num_attrs;
>> + int ret, idx, i;
>> + char *esi_buf;
>> +
>> + if (!firmware_has_feature(FW_FEATURE_LPAR))
>> + return -ENXIO;
>> +
>> + esi_buf = kmalloc(MAX_BUF_SZ, GFP_KERNEL);
>> + if (esi_buf == NULL)
>> + return -ENOMEM;
>> + /*
>> + * 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
>> + */
>> + ret = plpar_hcall_norets(H_GET_ENERGY_SCALE_INFO, ESI_FLAGS_ALL, 0,
>> + virt_to_phys(esi_buf), MAX_BUF_SZ);
>
> It is good that you are passing a character buffer here and
> interpreting it later. This helps in the cases where the header has
> more fields than what we are aware of changed. I think even a future
> platform adds newer fields to the header, those fields will be
> appended after the existing fields, so we should still be able to
> interpret the first three fields of the header that we are currently
> aware of.
>
>
>> + if (ret != H_SUCCESS) {
>> + pr_warn("hcall failed: H_GET_ENERGY_SCALE_INFO");
>> + goto out;
>> + }
>> +
>> + esi_hdr = (struct h_energy_scale_info_hdr *) esi_buf;
>> + num_attrs = be64_to_cpu(esi_hdr->num_attrs);
>
> Shouldn't we check for the esi_hdr->data_header_version here?
> Currently we are only aware of the version 1. If we happen to run this
> kernel code on a future platform which supports a different version,
> wouldn't it be safer to bail out here ?
>
> Otherwise this patch looks good to me.
>
> Reviewed-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>

Thanks for the review Gautham.
Sure I'll make use of the header version to bail out.
That just seems like the safest approach.

I'll add that and spin a new version here

Thanks
Pratik

> --
> Thanks and Regards
> gautham.

\
 
 \ /
  Last update: 2021-07-12 10:35    [W:0.051 / U:0.144 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site