Messages in this thread | | | Subject | Re: [PATCH v2 1/1] powerpc/pseries: Interface to represent PAPR firmware attributes | From | Pratik Sampat <> | Date | Mon, 12 Jul 2021 13:13:02 +0530 |
| |
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.
| |