linux-kernel - Re: [PATCH] Export Idlemask values based on the APU

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Tue, 14 Sep 2021 11:53:55 -0500
From:   "Limonciello, Mario" <mario.limonciello@....com>
To:     Sanket Goswami <Sanket.Goswami@....com>, Shyam-sundar.S-k@....com,
        hdegoede@...hat.com, mgross@...ux.intel.com
Cc:     platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Export Idlemask values based on the APU

Sanket,

Thanks for this!  I think it will be quite useful.

On 9/14/2021 08:14, Sanket Goswami wrote:
> IdleMask is the metric used by the PM firmware to know the status of each
> of the Hardware IP blocks monitored by the PM firmware.

Although reading runtime Idle mask from debugfs is useful for 
determining runtime power consumption blockers, I think it may also be 
useful to check before OS_HINT is sent as a debugging tactic.
The value in the mask can then be stored in an integer and/or:

1. Read out with debugfs later (maybe as output during part of reading 
smu_fw_info or even a second line of amd_pmc_idlemask along the lines of 
"last s0i3 mask").
2. Output with dev_dbg to allow turning it on with dynamic debugging and 
catching in kernel logs.

I think the second flow is actually the most valuable.  Users who have 
rare failures could be asked to turn on dynamic debugging for that 
single line and then compare the IdleMask between a good and bad suspend.

> 
> Knowing this value is key to get the information of s2idle suspend/resume
> status. This value is mapped to PMC scratch registers, retrieve them
> accordingly based on the CPU family and the underlying firmware support.
> 
> Co-developed-by: Shyam Sundar S K <Shyam-sundar.S-k@....com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@....com>
> Signed-off-by: Sanket Goswami <Sanket.Goswami@....com>
> ---
>   drivers/platform/x86/amd-pmc.c | 57 ++++++++++++++++++++++++++++++++++
>   1 file changed, 57 insertions(+)
> 
> diff --git a/drivers/platform/x86/amd-pmc.c b/drivers/platform/x86/amd-pmc.c
> index 3481479a2942..e2e55e5bea22 100644
> --- a/drivers/platform/x86/amd-pmc.c
> +++ b/drivers/platform/x86/amd-pmc.c
> @@ -29,6 +29,10 @@
>   #define AMD_PMC_REGISTER_RESPONSE	0x980
>   #define AMD_PMC_REGISTER_ARGUMENT	0x9BC
>   
> +/* PMC Scratch Registers */
> +#define AMD_PMC_SCRATCH_REG_CZN		0x94
> +#define AMD_PMC_SCRATCH_REG_YC		0xD14
> +
>   /* Base address of SMU for mapping physical address to virtual address */
>   #define AMD_PMC_SMU_INDEX_ADDRESS	0xB8
>   #define AMD_PMC_SMU_INDEX_DATA		0xBC
> @@ -110,6 +114,10 @@ struct amd_pmc_dev {
>   	u32 base_addr;
>   	u32 cpu_id;
>   	u32 active_ips;
> +/* SMU version information */
> +	u16 major;
> +	u16 minor;
> +	u16 rev;
>   	struct device *dev;
>   	struct mutex lock; /* generic mutex lock */
>   #if IS_ENABLED(CONFIG_DEBUG_FS)
> @@ -201,6 +209,53 @@ static int s0ix_stats_show(struct seq_file *s, void *unused)
>   }
>   DEFINE_SHOW_ATTRIBUTE(s0ix_stats);
>   
> +static int amd_pmc_get_smu_version(struct amd_pmc_dev *dev)
> +{
> +	int rc;
> +	u32 val;
> +
> +	rc = amd_pmc_send_cmd(dev, 0, &val, SMU_MSG_GETSMUVERSION, 1);
> +	if (rc)
> +		return rc;
> +
> +	dev->major = (val >> 16) & GENMASK(15, 0);
> +	dev->minor = (val >> 8) & GENMASK(7, 0);
> +	dev->rev = (val >> 0) & GENMASK(7, 0);
> +
> +	dev_dbg(dev->dev, "SMU version is %u.%u.%u\n", dev->major, dev->minor, dev->rev);
> +
> +	return 0;
> +}
> +
> +static int amd_pmc_idlemask_show(struct seq_file *s, void *unused)

If you follow my suggestion in the commit message I would think you 
should make this a new function that is amd_pmc_idlemask_read that is 
called both when reading from the debugfs attribute as well as during 
s0i3 entry.  When called from s0i3 entry the value can be stored to 
something like dev->last_idle_mask.

> +{
> +	struct amd_pmc_dev *dev = s->private;
> +	u32 val;
> +
> +	amd_pmc_get_smu_version(dev);

The SMU version doesn't change during runtime and furthermore the values 
are cached into dev->major, dev->minor, dev->rev.

Wouldn't it make more sense to read the SMU version during 
`amd_pmc_probe` rather than every time the idle mask is read?

> +
> +	if (dev->major > 56 || (dev->major >= 55 && dev->minor >= 37)) {
> +		switch (dev->cpu_id) {
> +		case AMD_CPU_ID_CZN:
> +			val = amd_pmc_reg_read(dev, AMD_PMC_SCRATCH_REG_CZN);
> +			seq_printf(s, "SMU idlemask: 0x%x\n", val);
> +			break;
> +		case AMD_CPU_ID_YC:
> +			val = amd_pmc_reg_read(dev, AMD_PMC_SCRATCH_REG_YC);
> +			seq_printf(s, "SMU idlemask: 0x%x\n", val);
> +			break;
> +		default:
> +			seq_puts(s, "SMU idlemask: Unsupported CPU ID\n");
> +			break;
> +		}
> +	} else {
> +		seq_puts(s, "Unsupported SMU version for Idlemask\n");
> +	}
> +
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(amd_pmc_idlemask);
> +
>   static void amd_pmc_dbgfs_unregister(struct amd_pmc_dev *dev)
>   {
>   	debugfs_remove_recursive(dev->dbgfs_dir);
> @@ -213,6 +268,8 @@ static void amd_pmc_dbgfs_register(struct amd_pmc_dev *dev)
>   			    &smu_fw_info_fops);
>   	debugfs_create_file("s0ix_stats", 0644, dev->dbgfs_dir, dev,
>   			    &s0ix_stats_fops);
> +	debugfs_create_file("amd_pmc_idlemask", 0644, dev->dbgfs_dir, dev,
> +			    &amd_pmc_idlemask_fops);
>   }
>   #else
>   static inline void amd_pmc_dbgfs_register(struct amd_pmc_dev *dev)
>