From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.5 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7A89EC07E96 for ; Thu, 8 Jul 2021 10:36:14 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 541BF61447 for ; Thu, 8 Jul 2021 10:36:14 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231550AbhGHKiy (ORCPT ); Thu, 8 Jul 2021 06:38:54 -0400 Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:49790 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S231332AbhGHKix (ORCPT ); Thu, 8 Jul 2021 06:38:53 -0400 Received: from pps.filterd (m0098413.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.0.43/8.16.0.43) with SMTP id 168AYAGd046938; Thu, 8 Jul 2021 06:35:50 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ibm.com; h=date : from : to : cc : subject : message-id : reply-to : references : mime-version : content-type : in-reply-to; s=pp1; bh=EKNhUAjpoR2zsfBpOiCvG61sy+hto3HdXvo++O0la20=; b=qG9TTP+XxvFYKK6n4r0c+FqLERZVZ9B4IfsxsAGKFCyyQqK1abPpIY/x41A3v1iXTHgb 5o+TUuNRyUHh2I7HUbc2JeQd9IHtLUwn6nJNzuVb7QwQHb/aqZvwpmbP6+t+URGnYpi8 FPLx0I5qZtp7uJbzD9PnZL/dXh5tC7djpsT2cPZEXDieL8WZWyswkUQ2DmQOOx3OF0Ps VOhG+C4jEm8Z71TvSGtUQC6AGMC3tY8QAsqrsKXAxPuOtbg3J2FzKfzqStjUgXpOAieE +3gSz8P6VfcF2xLNbMQ6EEUOYgQsXDS81NM9/yM03AqbgeHTD8Pdp5xxvuALdLSXPpgk rw== Received: from pps.reinject (localhost [127.0.0.1]) by mx0b-001b2d01.pphosted.com with ESMTP id 39nrfh2wd3-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 08 Jul 2021 06:35:50 -0400 Received: from m0098413.ppops.net (m0098413.ppops.net [127.0.0.1]) by pps.reinject (8.16.0.43/8.16.0.43) with SMTP id 168AZfd7054180; Thu, 8 Jul 2021 06:35:50 -0400 Received: from ppma02wdc.us.ibm.com (aa.5b.37a9.ip4.static.sl-reverse.com [169.55.91.170]) by mx0b-001b2d01.pphosted.com with ESMTP id 39nrfh2wcp-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 08 Jul 2021 06:35:50 -0400 Received: from pps.filterd (ppma02wdc.us.ibm.com [127.0.0.1]) by ppma02wdc.us.ibm.com (8.16.1.2/8.16.1.2) with SMTP id 168AU5uo013075; Thu, 8 Jul 2021 10:35:49 GMT Received: from b03cxnp08027.gho.boulder.ibm.com (b03cxnp08027.gho.boulder.ibm.com [9.17.130.19]) by ppma02wdc.us.ibm.com with ESMTP id 39jfhcm5uv-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 08 Jul 2021 10:35:49 +0000 Received: from b03ledav002.gho.boulder.ibm.com (b03ledav002.gho.boulder.ibm.com [9.17.130.233]) by b03cxnp08027.gho.boulder.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 168AZmCY4456968 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Thu, 8 Jul 2021 10:35:48 GMT Received: from b03ledav002.gho.boulder.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 55C1E13604F; Thu, 8 Jul 2021 10:35:48 +0000 (GMT) Received: from b03ledav002.gho.boulder.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id AA0BB136051; Thu, 8 Jul 2021 10:35:47 +0000 (GMT) Received: from sofia.ibm.com (unknown [9.102.17.152]) by b03ledav002.gho.boulder.ibm.com (Postfix) with ESMTP; Thu, 8 Jul 2021 10:35:47 +0000 (GMT) Received: by sofia.ibm.com (Postfix, from userid 1000) id E82F12E3B06; Thu, 8 Jul 2021 16:05:43 +0530 (IST) Date: Thu, 8 Jul 2021 16:05:43 +0530 From: Gautham R Shenoy To: "Pratik R. Sampat" Cc: 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: [PATCH v2 1/1] powerpc/pseries: Interface to represent PAPR firmware attributes Message-ID: <20210708103543.GA20804@in.ibm.com> Reply-To: ego@linux.vnet.ibm.com References: <20210706082400.36996-1-psampat@linux.ibm.com> <20210706082400.36996-2-psampat@linux.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210706082400.36996-2-psampat@linux.ibm.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-TM-AS-GCONF: 00 X-Proofpoint-GUID: LeqWVR8_rrO6euybY3olKXeVbuUI5BYr X-Proofpoint-ORIG-GUID: dghLorA6Xbf-5xYQ9ghIUOEHPeqI-4ES X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.391,18.0.790 definitions=2021-07-08_04:2021-07-06,2021-07-08 signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 clxscore=1015 phishscore=0 lowpriorityscore=0 suspectscore=0 malwarescore=0 mlxlogscore=999 spamscore=0 adultscore=0 impostorscore=0 priorityscore=1501 mlxscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2104190000 definitions=main-2107080057 Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 and Regards gautham. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.3 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 42F01C07E96 for ; Thu, 8 Jul 2021 10:36:30 +0000 (UTC) Received: from lists.ozlabs.org (lists.ozlabs.org [112.213.38.117]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 9692361435 for ; Thu, 8 Jul 2021 10:36:29 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 9692361435 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linux.vnet.ibm.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=linuxppc-dev-bounces+linuxppc-dev=archiver.kernel.org@lists.ozlabs.org Received: from boromir.ozlabs.org (localhost [IPv6:::1]) by lists.ozlabs.org (Postfix) with ESMTP id 4GLCQ40xyBz3bXY for ; Thu, 8 Jul 2021 20:36:28 +1000 (AEST) Authentication-Results: lists.ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=ibm.com header.i=@ibm.com header.a=rsa-sha256 header.s=pp1 header.b=qG9TTP+X; dkim-atps=neutral Authentication-Results: lists.ozlabs.org; spf=none (no SPF record) smtp.mailfrom=linux.vnet.ibm.com (client-ip=148.163.158.5; helo=mx0a-001b2d01.pphosted.com; envelope-from=ego@linux.vnet.ibm.com; receiver=) Authentication-Results: lists.ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=ibm.com header.i=@ibm.com header.a=rsa-sha256 header.s=pp1 header.b=qG9TTP+X; dkim-atps=neutral Received: from mx0a-001b2d01.pphosted.com (mx0b-001b2d01.pphosted.com [148.163.158.5]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 4GLCPW2RtHz2yXs for ; Thu, 8 Jul 2021 20:35:58 +1000 (AEST) Received: from pps.filterd (m0098413.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.0.43/8.16.0.43) with SMTP id 168AYAGd046938; Thu, 8 Jul 2021 06:35:50 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ibm.com; h=date : from : to : cc : subject : message-id : reply-to : references : mime-version : content-type : in-reply-to; s=pp1; bh=EKNhUAjpoR2zsfBpOiCvG61sy+hto3HdXvo++O0la20=; b=qG9TTP+XxvFYKK6n4r0c+FqLERZVZ9B4IfsxsAGKFCyyQqK1abPpIY/x41A3v1iXTHgb 5o+TUuNRyUHh2I7HUbc2JeQd9IHtLUwn6nJNzuVb7QwQHb/aqZvwpmbP6+t+URGnYpi8 FPLx0I5qZtp7uJbzD9PnZL/dXh5tC7djpsT2cPZEXDieL8WZWyswkUQ2DmQOOx3OF0Ps VOhG+C4jEm8Z71TvSGtUQC6AGMC3tY8QAsqrsKXAxPuOtbg3J2FzKfzqStjUgXpOAieE +3gSz8P6VfcF2xLNbMQ6EEUOYgQsXDS81NM9/yM03AqbgeHTD8Pdp5xxvuALdLSXPpgk rw== Received: from pps.reinject (localhost [127.0.0.1]) by mx0b-001b2d01.pphosted.com with ESMTP id 39nrfh2wd3-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 08 Jul 2021 06:35:50 -0400 Received: from m0098413.ppops.net (m0098413.ppops.net [127.0.0.1]) by pps.reinject (8.16.0.43/8.16.0.43) with SMTP id 168AZfd7054180; Thu, 8 Jul 2021 06:35:50 -0400 Received: from ppma02wdc.us.ibm.com (aa.5b.37a9.ip4.static.sl-reverse.com [169.55.91.170]) by mx0b-001b2d01.pphosted.com with ESMTP id 39nrfh2wcp-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 08 Jul 2021 06:35:50 -0400 Received: from pps.filterd (ppma02wdc.us.ibm.com [127.0.0.1]) by ppma02wdc.us.ibm.com (8.16.1.2/8.16.1.2) with SMTP id 168AU5uo013075; Thu, 8 Jul 2021 10:35:49 GMT Received: from b03cxnp08027.gho.boulder.ibm.com (b03cxnp08027.gho.boulder.ibm.com [9.17.130.19]) by ppma02wdc.us.ibm.com with ESMTP id 39jfhcm5uv-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 08 Jul 2021 10:35:49 +0000 Received: from b03ledav002.gho.boulder.ibm.com (b03ledav002.gho.boulder.ibm.com [9.17.130.233]) by b03cxnp08027.gho.boulder.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 168AZmCY4456968 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Thu, 8 Jul 2021 10:35:48 GMT Received: from b03ledav002.gho.boulder.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 55C1E13604F; Thu, 8 Jul 2021 10:35:48 +0000 (GMT) Received: from b03ledav002.gho.boulder.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id AA0BB136051; Thu, 8 Jul 2021 10:35:47 +0000 (GMT) Received: from sofia.ibm.com (unknown [9.102.17.152]) by b03ledav002.gho.boulder.ibm.com (Postfix) with ESMTP; Thu, 8 Jul 2021 10:35:47 +0000 (GMT) Received: by sofia.ibm.com (Postfix, from userid 1000) id E82F12E3B06; Thu, 8 Jul 2021 16:05:43 +0530 (IST) Date: Thu, 8 Jul 2021 16:05:43 +0530 From: Gautham R Shenoy To: "Pratik R. Sampat" Subject: Re: [PATCH v2 1/1] powerpc/pseries: Interface to represent PAPR firmware attributes Message-ID: <20210708103543.GA20804@in.ibm.com> References: <20210706082400.36996-1-psampat@linux.ibm.com> <20210706082400.36996-2-psampat@linux.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210706082400.36996-2-psampat@linux.ibm.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-TM-AS-GCONF: 00 X-Proofpoint-GUID: LeqWVR8_rrO6euybY3olKXeVbuUI5BYr X-Proofpoint-ORIG-GUID: dghLorA6Xbf-5xYQ9ghIUOEHPeqI-4ES X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.391, 18.0.790 definitions=2021-07-08_04:2021-07-06, 2021-07-08 signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 clxscore=1015 phishscore=0 lowpriorityscore=0 suspectscore=0 malwarescore=0 mlxlogscore=999 spamscore=0 adultscore=0 impostorscore=0 priorityscore=1501 mlxscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2104190000 definitions=main-2107080057 X-BeenThere: linuxppc-dev@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: ego@linux.vnet.ibm.com Cc: pratik.r.sampat@gmail.com, linux-kernel@vger.kernel.org, kvm-ppc@vger.kernel.org, paulus@samba.org, linuxppc-dev@lists.ozlabs.org Errors-To: linuxppc-dev-bounces+linuxppc-dev=archiver.kernel.org@lists.ozlabs.org Sender: "Linuxppc-dev" 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 and Regards gautham. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gautham R Shenoy Date: Thu, 08 Jul 2021 10:47:43 +0000 Subject: Re: [PATCH v2 1/1] powerpc/pseries: Interface to represent PAPR firmware attributes Message-Id: <20210708103543.GA20804@in.ibm.com> List-Id: References: <20210706082400.36996-1-psampat@linux.ibm.com> <20210706082400.36996-2-psampat@linux.ibm.com> In-Reply-To: <20210706082400.36996-2-psampat@linux.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: "Pratik R. Sampat" Cc: 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 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 and Regards gautham.