Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <3995ade6-385c-45cd-3ff4-052835337546@redhat.com>
Date:   Thu, 17 Nov 2022 14:33:34 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.1
Subject: Re: [PATCH 4/9] platform/x86/intel/sdsi: Add meter certificate
 support
Content-Language: en-US, nl
To:     "David E. Box" <david.e.box@linux.intel.com>, markgross@kernel.org,
        andriy.shevchenko@linux.intel.com, srinivas.pandruvada@intel.com
Cc:     platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20221101191023.4150315-1-david.e.box@linux.intel.com>
 <20221101191023.4150315-5-david.e.box@linux.intel.com>
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <20221101191023.4150315-5-david.e.box@linux.intel.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi,

On 11/1/22 20:10, David E. Box wrote:
> Add support for reading the meter certificate from Intel On Demand
> hardware.  The meter certificate [1] is used to access the utilization
> metrics of enabled features in support of the Intel On Demand consumption
> model. Similar to the state certificate, the meter certificate is read by
> mailbox command.
> 
> [1] https://github.com/intel-sandbox/debox1.intel_sdsi/blob/gnr-review/meter-certificate.rst
> 
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>

Besides Andy's remarks I also have 1 remark myself, see below.

> ---
>  .../ABI/testing/sysfs-driver-intel_sdsi       | 10 ++++
>  drivers/platform/x86/intel/sdsi.c             | 52 +++++++++++++++----
>  2 files changed, 52 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-driver-intel_sdsi b/Documentation/ABI/testing/sysfs-driver-intel_sdsi
> index 9d77f30d9b9a..f8afed127107 100644
> --- a/Documentation/ABI/testing/sysfs-driver-intel_sdsi
> +++ b/Documentation/ABI/testing/sysfs-driver-intel_sdsi
> @@ -69,6 +69,16 @@ Description:
>  		the CPU configuration is updated. A cold reboot is required to
>  		fully activate the feature. Mailbox command.
>  
> +What:		/sys/bus/auxiliary/devices/intel_vsec.sdsi.X/meter_certificate
> +Date:		Nov 2022
> +KernelVersion:	6.2
> +Contact:	"David E. Box" <david.e.box@linux.intel.com>
> +Description:
> +		(RO) Used to read back the current meter certificate for the CPU
> +		from Intel On Demand hardware. The meter certificate contains
> +		utilization metrics of On Demand enabled features. Mailbox
> +		command.
> +
>  What:		/sys/bus/auxiliary/devices/intel_vsec.sdsi.X/state_certificate
>  Date:		Feb 2022
>  KernelVersion:	5.18
> diff --git a/drivers/platform/x86/intel/sdsi.c b/drivers/platform/x86/intel/sdsi.c
> index ab1f65919fc5..1dee10822df7 100644
> --- a/drivers/platform/x86/intel/sdsi.c
> +++ b/drivers/platform/x86/intel/sdsi.c
> @@ -42,6 +42,7 @@
>  
>  #define SDSI_ENABLED_FEATURES_OFFSET	16
>  #define SDSI_FEATURE_SDSI		BIT(3)
> +#define SDSI_FEATURE_METERING		BIT(26)
>  
>  #define SDSI_SOCKET_ID_OFFSET		64
>  #define SDSI_SOCKET_ID			GENMASK(3, 0)
> @@ -80,9 +81,10 @@
>  #define SDSI_GUID_V2			0xF210D9EF
>  
>  enum sdsi_command {
> -	SDSI_CMD_PROVISION_AKC		= 0x04,
> -	SDSI_CMD_PROVISION_CAP		= 0x08,
> -	SDSI_CMD_READ_STATE		= 0x10,
> +	SDSI_CMD_PROVISION_AKC		= 0x0004,
> +	SDSI_CMD_PROVISION_CAP		= 0x0008,
> +	SDSI_CMD_READ_STATE		= 0x0010,
> +	SDSI_CMD_READ_METER		= 0x0014,
>  };
>  
>  struct sdsi_mbox_info {
> @@ -398,13 +400,10 @@ static ssize_t provision_cap_write(struct file *filp, struct kobject *kobj,
>  }
>  static BIN_ATTR_WO(provision_cap, SDSI_SIZE_WRITE_MSG);
>  
> -static long state_certificate_read(struct file *filp, struct kobject *kobj,
> -				   struct bin_attribute *attr, char *buf, loff_t off,
> -				   size_t count)
> +static ssize_t
> +certificate_read(u64 command, struct sdsi_priv *priv, char *buf, loff_t off,
> +		 size_t count)
>  {
> -	struct device *dev = kobj_to_dev(kobj);
> -	struct sdsi_priv *priv = dev_get_drvdata(dev);
> -	u64 command = SDSI_CMD_READ_STATE;
>  	struct sdsi_mbox_info info;
>  	size_t size;
>  	int ret;
> @@ -441,8 +440,31 @@ static long state_certificate_read(struct file *filp, struct kobject *kobj,
>  
>  	return size;
>  }
> +
> +static ssize_t
> +state_certificate_read(struct file *filp, struct kobject *kobj,
> +		       struct bin_attribute *attr, char *buf, loff_t off,
> +		       size_t count)
> +{
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct sdsi_priv *priv = dev_get_drvdata(dev);
> +
> +	return certificate_read(SDSI_CMD_READ_STATE, priv, buf, off, count);
> +}
>  static BIN_ATTR(state_certificate, 0400, state_certificate_read, NULL, SDSI_SIZE_READ_MSG);
>  
> +static ssize_t
> +meter_certificate_read(struct file *filp, struct kobject *kobj,
> +		       struct bin_attribute *attr, char *buf, loff_t off,
> +		       size_t count)
> +{
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct sdsi_priv *priv = dev_get_drvdata(dev);
> +
> +	return certificate_read(SDSI_CMD_READ_METER, priv, buf, off, count);
> +}
> +static BIN_ATTR(meter_certificate, 0400, meter_certificate_read, NULL, SDSI_SIZE_READ_MSG);
> +
>  static ssize_t registers_read(struct file *filp, struct kobject *kobj,
>  			      struct bin_attribute *attr, char *buf, loff_t off,
>  			      size_t count)
> @@ -472,6 +494,7 @@ static BIN_ATTR(registers, 0400, registers_read, NULL, SDSI_SIZE_REGS);
>  static struct bin_attribute *sdsi_bin_attrs[] = {
>  	&bin_attr_registers,
>  	&bin_attr_state_certificate,
> +	&bin_attr_meter_certificate,
>  	&bin_attr_provision_akc,
>  	&bin_attr_provision_cap,
>  	NULL
> @@ -491,7 +514,16 @@ sdsi_battr_is_visible(struct kobject *kobj, struct bin_attribute *attr, int n)
>  	if (!(priv->features & SDSI_FEATURE_SDSI))
>  		return 0;
>  
> -	return attr->attr.mode;
> +	if (attr == &bin_attr_state_certificate ||
> +	    attr == &bin_attr_provision_akc ||
> +	    attr == &bin_attr_provision_cap)
> +		return attr->attr.mode;

I would prefer for you to just drop this and then change:

> +
> +	if (attr == &bin_attr_meter_certificate &&
> +	    !!(priv->features & SDSI_FEATURE_METERING))
> +		return attr->attr.mode;

to:

	if (attr == &bin_attr_meter_certificate)
		return (priv->features & SDSI_FEATURE_METERING) ?
				attr->attr.mode : 0;

And then keep:

	return attr->attr.mode;

at the end of this function, because now you are hiding all
new attributes by default and then you have to add any new
attributes to the if above, leading to an ever growing lists
of attr to check in that if, so just having:

	return attr->attr.mode;

As default for any non-matches attr would be much better IMHO.

Regards,

Hans


> +
> +	return 0;
>  }
>  
>  static ssize_t guid_show(struct device *dev, struct device_attribute *attr, char *buf)