Re: [PATCH v2 1/1] powerpc/pseries: Interface to represent PAPR firmware attributes
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/ |-- / |-- desc |-- value |-- value_desc (if exists) |-- / |-- 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 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.
Re: [PATCH v2 1/1] powerpc/pseries: Interface to represent PAPR firmware attributes
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/ >|-- / > |-- desc > |-- value > |-- value_desc (if exists) >|-- / > |-- 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_ATTRS10 > +#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 -- Thanks and Regards gautham.
[PATCH v2 1/1] powerpc/pseries: Interface to represent PAPR firmware attributes
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/ |-- / |-- desc |-- value |-- value_desc (if exists) |-- / |-- 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 --- .../sysfs-firmware-papr-energy-scale-info | 26 ++ arch/powerpc/include/asm/hvcall.h | 23 +- arch/powerpc/kvm/trace_hv.h | 1 + arch/powerpc/platforms/pseries/Makefile | 3 +- .../pseries/papr_platform_attributes.c| 320 ++ 5 files changed, 371 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 ..fd82f2bfafe5 --- /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 +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/ + /sys/firmware/papr/energy_scale_info//desc + /sys/firmware/papr/energy_scale_info//value + /sys/firmware/papr/energy_scale_info//value_desc +Date: June 2021 +Contact: Linux for PowerPC mailing list +Description: Energy, frequency attributes directory for POWERVM servers + + This directory provides energy, erequency, folding information. It + contains below sysfs attributes: + + - desc: String description of the attribute + + - value: Numeric value of attribute + + - value_desc: String value of attribute diff --git a/arch/powerpc/include/asm/hvcall.h b/arch/powerpc/include/asm/hvcall.h index e3b29eda8074..25a6b744d41e 100644 --- a/arch/powerpc/include/asm/hvcall.h +++ b/arch/powerpc/include/asm/hvcall.h @@ -316,7 +316,8 @@ #define H_SCM_PERFORMANCE_STATS 0x418 #define H_RPT_INVALIDATE 0x448 #define H_SCM_FLUSH0x44C -#define MAX_HCALL_OPCODE H_SCM_FLUSH +#define H_GET_ENERGY_SCALE_INFO0x450 +#define MAX_HCALL_OPCODE H_GET_ENERGY_SCALE_INFO /* Scope args for H_SCM_UNBIND_ALL */ #define H_UNBIND_SCOPE_ALL (0x1) @@ -631,6 +632,26 @@ struct hv_gpci_request_buffer { uint8_t bytes[HGPCI_MAX_DATA_BYTES]; } __packed; +#define MAX_ESI_ATTRS 10 +#defin