diff mbox series

acpi: Allow ignoring _OSC CPPC v2 bit via kernel parameter

Message ID ZnD22b3Br1ng7alf@kf-XE (mailing list archive)
State Superseded, archived
Headers show
Series acpi: Allow ignoring _OSC CPPC v2 bit via kernel parameter | expand

Commit Message

Aaron Rainbolt June 18, 2024, 2:54 a.m. UTC 
acpi: Allow ignoring _OSC CPPC v2 bit via kernel parameter

The _OSC is supposed to contain a bit indicating whether the hardware
supports CPPC v2 or not. This bit is not always set, causing CPPC v2 to
be considered absent. This results in severe single-core performance
issues with the EEVDF scheduler.

To work around this, provide a new kernel parameter,
"processor.ignore_osc_cppc_bit", which may be used to ignore the _OSC
CPPC v2 bit and act as if the bit was enabled. This allows CPPC to be
properly detected even if not "enabled" by _OSC, allowing users with
problematic hardware to obtain decent single-core performance.

Tested-by: Michael Mikowski <mmikowski@kfocus.org>
Signed-off-by: Aaron Rainbolt <arainbolt@kfocus.org>

---

Comments

Mario Limonciello June 18, 2024, 5:09 p.m. UTC | #1 
On 6/17/2024 21:54, Aaron Rainbolt wrote:
> acpi: Allow ignoring _OSC CPPC v2 bit via kernel parameter
> 
> The _OSC is supposed to contain a bit indicating whether the hardware
> supports CPPC v2 or not. This bit is not always set, causing CPPC v2 to
> be considered absent. This results in severe single-core performance
> issues with the EEVDF scheduler.
> 
> To work around this, provide a new kernel parameter,
> "processor.ignore_osc_cppc_bit", which may be used to ignore the _OSC
> CPPC v2 bit and act as if the bit was enabled. This allows CPPC to be
> properly detected even if not "enabled" by _OSC, allowing users with
> problematic hardware to obtain decent single-core performance.
> 
> Tested-by: Michael Mikowski <mmikowski@kfocus.org>
> Signed-off-by: Aaron Rainbolt <arainbolt@kfocus.org>

This sounds like a platform bug and if we do accept a patch like this I 
think we need a lot more documentation about the situation.

Can you please share more information about your hardware:
1) Manufacturer?
2) CPU?
3) Manufacturer firmware version?
4) If it's AMD what's the AGESA version?

And most importantly do you have the latest system firmware version from 
your manufacturer?  If not; please upgrade that first.

> 
> ---
> 
> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> index 1d857978f5f4..53406dd6cb87 100644
> --- a/drivers/acpi/cppc_acpi.c
> +++ b/drivers/acpi/cppc_acpi.c
> @@ -671,7 +671,7 @@ static inline void arch_init_invariance_cppc(void) { }
>    *
>    *	Return: 0 for success or negative value for err.
>    */
> -int acpi_cppc_processor_probe(struct acpi_processor *pr)
> +int acpi_cppc_processor_probe(struct acpi_processor *pr, bool ignore_osc_cppc_bit)
>   {
>   	struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL};
>   	union acpi_object *out_obj, *cpc_obj;
> @@ -686,7 +686,7 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
>   
>   	if (!osc_sb_cppc2_support_acked) {
>   		pr_debug("CPPC v2 _OSC not acked\n");
> -		if (!cpc_supported_by_cpu()) {
> +		if (!ignore_osc_cppc_bit && !cpc_supported_by_cpu()) {
>   			pr_debug("CPPC is not supported by the CPU\n");
>   			return -ENODEV;
>   		}
> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
> index 67db60eda370..a183bca6c1c5 100644
> --- a/drivers/acpi/processor_driver.c
> +++ b/drivers/acpi/processor_driver.c
> @@ -33,6 +33,11 @@ MODULE_AUTHOR("Paul Diefenbaugh");
>   MODULE_DESCRIPTION("ACPI Processor Driver");
>   MODULE_LICENSE("GPL");
>   
> +static bool ignore_osc_cppc_bit = false;
> +module_param(ignore_osc_cppc_bit, bool, 0);
> +MODULE_PARM_DESC(ignore_osc_cppc_bit,
> +	"Ignore _OSC CPPC bit, assume CPPC v2 is present");
> +
>   static int acpi_processor_start(struct device *dev);
>   static int acpi_processor_stop(struct device *dev);
>   
> @@ -170,7 +175,7 @@ static int __acpi_processor_start(struct acpi_device *device)
>   	if (pr->flags.need_hotplug_init)
>   		return 0;
>   
> -	result = acpi_cppc_processor_probe(pr);
> +	result = acpi_cppc_processor_probe(pr, ignore_osc_cppc_bit);
>   	if (result && !IS_ENABLED(CONFIG_ACPI_CPU_FREQ_PSS))
>   		dev_dbg(&device->dev, "CPPC data invalid or not present\n");
>   
> diff --git a/include/acpi/processor.h b/include/acpi/processor.h
> index 3f34ebb27525..79fd61b3f537 100644
> --- a/include/acpi/processor.h
> +++ b/include/acpi/processor.h
> @@ -358,10 +358,10 @@ int acpi_map_cpuid(phys_cpuid_t phys_id, u32 acpi_id);
>   int acpi_get_cpuid(acpi_handle, int type, u32 acpi_id);
>   
>   #ifdef CONFIG_ACPI_CPPC_LIB
> -extern int acpi_cppc_processor_probe(struct acpi_processor *pr);
> +extern int acpi_cppc_processor_probe(struct acpi_processor *pr, bool ignore_osc_cppc_bit);
>   extern void acpi_cppc_processor_exit(struct acpi_processor *pr);
>   #else
> -static inline int acpi_cppc_processor_probe(struct acpi_processor *pr)
> +static inline int acpi_cppc_processor_probe(struct acpi_processor *pr, bool ignore_osc_cppc_bit)
>   {
>   	return 0;
>   }
Aaron Rainbolt June 18, 2024, 6:30 p.m. UTC | #2 
On Tue, Jun 18, 2024 at 12:09:19PM -0500, Mario Limonciello wrote:
> On 6/17/2024 21:54, Aaron Rainbolt wrote:
> > acpi: Allow ignoring _OSC CPPC v2 bit via kernel parameter
> > 
> > The _OSC is supposed to contain a bit indicating whether the hardware
> > supports CPPC v2 or not. This bit is not always set, causing CPPC v2 to
> > be considered absent. This results in severe single-core performance
> > issues with the EEVDF scheduler.
> > 
> > To work around this, provide a new kernel parameter,
> > "processor.ignore_osc_cppc_bit", which may be used to ignore the _OSC
> > CPPC v2 bit and act as if the bit was enabled. This allows CPPC to be
> > properly detected even if not "enabled" by _OSC, allowing users with
> > problematic hardware to obtain decent single-core performance.
> > 
> > Tested-by: Michael Mikowski <mmikowski@kfocus.org>
> > Signed-off-by: Aaron Rainbolt <arainbolt@kfocus.org>
> 
> This sounds like a platform bug and if we do accept a patch like this I
> think we need a lot more documentation about the situation.

It is a platform bug, yes. See my previous email,                      
https://lore.kernel.org/linux-acpi/d01b0a1f-bd33-47fe-ab41-43843d8a374f@kfocus.org/T/#u
(I meant to send this email as a reply to that one, but failed to do so.)

> Can you please share more information about your hardware:
> 1) Manufacturer?

Carbon Systems, models Iridium 14 and Iridium 16.

> 2) CPU?

Intel Core i5-13500H.

> 3) Manufacturer firmware version?

The systems use an AMI BIOS with version N.1.10CAR01 according to
dmidecode. This is the latest BIOS available from the manufacturer.

> 4) If it's AMD what's the AGESA version?

Both affected systems are Intel-based and use heterogenous cores, not AMD.

> And most importantly do you have the latest system firmware version from
> your manufacturer?  If not; please upgrade that first.

We are using the latest firmware. (We're trying to work with the ODM to
potentially get a firmware update, but since this affects more than just
us and a firmware update may not be possible for everyone, this would
likely be worth providing a kernel-level workaround for.)

I can easily provide more detailed information - would the full output of
'dmidecode' and 'acpidump' be useful?
kernel test robot June 18, 2024, 6:31 p.m. UTC | #3 
Hi Aaron,

kernel test robot noticed the following build warnings:

[auto build test WARNING on rafael-pm/linux-next]
[also build test WARNING on rafael-pm/bleeding-edge linus/master v6.10-rc4 next-20240618]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Aaron-Rainbolt/acpi-Allow-ignoring-_OSC-CPPC-v2-bit-via-kernel-parameter/20240618-105454
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link:    https://lore.kernel.org/r/ZnD22b3Br1ng7alf%40kf-XE
patch subject: [PATCH] acpi: Allow ignoring _OSC CPPC v2 bit via kernel parameter
config: x86_64-defconfig (https://download.01.org/0day-ci/archive/20240619/202406190206.Z56zEzTy-lkp@intel.com/config)
compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240619/202406190206.Z56zEzTy-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202406190206.Z56zEzTy-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/acpi/cppc_acpi.c:677: warning: Function parameter or struct member 'ignore_osc_cppc_bit' not described in 'acpi_cppc_processor_probe'


vim +677 drivers/acpi/cppc_acpi.c

41ea667227bad5 Nathan Fontenot     2020-11-12  669  
337aadff8e4567 Ashwin Chaugule     2015-10-02  670  /**
337aadff8e4567 Ashwin Chaugule     2015-10-02  671   * acpi_cppc_processor_probe - Search for per CPU _CPC objects.
603fadf33604a2 Bjorn Helgaas       2019-03-25  672   * @pr: Ptr to acpi_processor containing this CPU's logical ID.
337aadff8e4567 Ashwin Chaugule     2015-10-02  673   *
337aadff8e4567 Ashwin Chaugule     2015-10-02  674   *	Return: 0 for success or negative value for err.
337aadff8e4567 Ashwin Chaugule     2015-10-02  675   */
e7e2a9d4606e2e Aaron Rainbolt      2024-06-17  676  int acpi_cppc_processor_probe(struct acpi_processor *pr, bool ignore_osc_cppc_bit)
337aadff8e4567 Ashwin Chaugule     2015-10-02 @677  {
337aadff8e4567 Ashwin Chaugule     2015-10-02  678  	struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL};
337aadff8e4567 Ashwin Chaugule     2015-10-02  679  	union acpi_object *out_obj, *cpc_obj;
337aadff8e4567 Ashwin Chaugule     2015-10-02  680  	struct cpc_desc *cpc_ptr;
337aadff8e4567 Ashwin Chaugule     2015-10-02  681  	struct cpc_reg *gas_t;
158c998ea44ba3 Ashwin Chaugule     2016-08-16  682  	struct device *cpu_dev;
337aadff8e4567 Ashwin Chaugule     2015-10-02  683  	acpi_handle handle = pr->handle;
337aadff8e4567 Ashwin Chaugule     2015-10-02  684  	unsigned int num_ent, i, cpc_rev;
85b1407bf6d2f4 George Cherian      2017-10-11  685  	int pcc_subspace_id = -1;
337aadff8e4567 Ashwin Chaugule     2015-10-02  686  	acpi_status status;
f21a3509842294 Rafael J. Wysocki   2022-03-22  687  	int ret = -ENODATA;
337aadff8e4567 Ashwin Chaugule     2015-10-02  688  
7feec7430edddb Mario Limonciello   2022-07-05  689  	if (!osc_sb_cppc2_support_acked) {
7feec7430edddb Mario Limonciello   2022-07-05  690  		pr_debug("CPPC v2 _OSC not acked\n");
e7e2a9d4606e2e Aaron Rainbolt      2024-06-17  691  		if (!ignore_osc_cppc_bit && !cpc_supported_by_cpu()) {
5f8f9bc4d7bc8d Perry Yuan          2024-04-25  692  			pr_debug("CPPC is not supported by the CPU\n");
c42fa24b44751c Rafael J. Wysocki   2022-03-16  693  			return -ENODEV;
7feec7430edddb Mario Limonciello   2022-07-05  694  		}
5f8f9bc4d7bc8d Perry Yuan          2024-04-25  695  	}
c42fa24b44751c Rafael J. Wysocki   2022-03-16  696  
603fadf33604a2 Bjorn Helgaas       2019-03-25  697  	/* Parse the ACPI _CPC table for this CPU. */
337aadff8e4567 Ashwin Chaugule     2015-10-02  698  	status = acpi_evaluate_object_typed(handle, "_CPC", NULL, &output,
337aadff8e4567 Ashwin Chaugule     2015-10-02  699  			ACPI_TYPE_PACKAGE);
337aadff8e4567 Ashwin Chaugule     2015-10-02  700  	if (ACPI_FAILURE(status)) {
337aadff8e4567 Ashwin Chaugule     2015-10-02  701  		ret = -ENODEV;
337aadff8e4567 Ashwin Chaugule     2015-10-02  702  		goto out_buf_free;
337aadff8e4567 Ashwin Chaugule     2015-10-02  703  	}
337aadff8e4567 Ashwin Chaugule     2015-10-02  704  
337aadff8e4567 Ashwin Chaugule     2015-10-02  705  	out_obj = (union acpi_object *) output.pointer;
337aadff8e4567 Ashwin Chaugule     2015-10-02  706  
337aadff8e4567 Ashwin Chaugule     2015-10-02  707  	cpc_ptr = kzalloc(sizeof(struct cpc_desc), GFP_KERNEL);
337aadff8e4567 Ashwin Chaugule     2015-10-02  708  	if (!cpc_ptr) {
337aadff8e4567 Ashwin Chaugule     2015-10-02  709  		ret = -ENOMEM;
337aadff8e4567 Ashwin Chaugule     2015-10-02  710  		goto out_buf_free;
337aadff8e4567 Ashwin Chaugule     2015-10-02  711  	}
337aadff8e4567 Ashwin Chaugule     2015-10-02  712  
337aadff8e4567 Ashwin Chaugule     2015-10-02  713  	/* First entry is NumEntries. */
337aadff8e4567 Ashwin Chaugule     2015-10-02  714  	cpc_obj = &out_obj->package.elements[0];
337aadff8e4567 Ashwin Chaugule     2015-10-02  715  	if (cpc_obj->type == ACPI_TYPE_INTEGER)	{
337aadff8e4567 Ashwin Chaugule     2015-10-02  716  		num_ent = cpc_obj->integer.value;
40d8abf364bcab Rafael J. Wysocki   2022-03-22  717  		if (num_ent <= 1) {
40d8abf364bcab Rafael J. Wysocki   2022-03-22  718  			pr_debug("Unexpected _CPC NumEntries value (%d) for CPU:%d\n",
40d8abf364bcab Rafael J. Wysocki   2022-03-22  719  				 num_ent, pr->id);
40d8abf364bcab Rafael J. Wysocki   2022-03-22  720  			goto out_free;
40d8abf364bcab Rafael J. Wysocki   2022-03-22  721  		}
337aadff8e4567 Ashwin Chaugule     2015-10-02  722  	} else {
f21a3509842294 Rafael J. Wysocki   2022-03-22  723  		pr_debug("Unexpected _CPC NumEntries entry type (%d) for CPU:%d\n",
f21a3509842294 Rafael J. Wysocki   2022-03-22  724  			 cpc_obj->type, pr->id);
337aadff8e4567 Ashwin Chaugule     2015-10-02  725  		goto out_free;
337aadff8e4567 Ashwin Chaugule     2015-10-02  726  	}
5bbb86aa4b8d84 Ashwin Chaugule     2016-08-16  727  
337aadff8e4567 Ashwin Chaugule     2015-10-02  728  	/* Second entry should be revision. */
337aadff8e4567 Ashwin Chaugule     2015-10-02  729  	cpc_obj = &out_obj->package.elements[1];
337aadff8e4567 Ashwin Chaugule     2015-10-02  730  	if (cpc_obj->type == ACPI_TYPE_INTEGER)	{
337aadff8e4567 Ashwin Chaugule     2015-10-02  731  		cpc_rev = cpc_obj->integer.value;
337aadff8e4567 Ashwin Chaugule     2015-10-02  732  	} else {
f21a3509842294 Rafael J. Wysocki   2022-03-22  733  		pr_debug("Unexpected _CPC Revision entry type (%d) for CPU:%d\n",
f21a3509842294 Rafael J. Wysocki   2022-03-22  734  			 cpc_obj->type, pr->id);
337aadff8e4567 Ashwin Chaugule     2015-10-02  735  		goto out_free;
337aadff8e4567 Ashwin Chaugule     2015-10-02  736  	}
337aadff8e4567 Ashwin Chaugule     2015-10-02  737  
4f4179fcf42087 Rafael J. Wysocki   2022-07-21  738  	if (cpc_rev < CPPC_V2_REV) {
4f4179fcf42087 Rafael J. Wysocki   2022-07-21  739  		pr_debug("Unsupported _CPC Revision (%d) for CPU:%d\n", cpc_rev,
4f4179fcf42087 Rafael J. Wysocki   2022-07-21  740  			 pr->id);
337aadff8e4567 Ashwin Chaugule     2015-10-02  741  		goto out_free;
4f4179fcf42087 Rafael J. Wysocki   2022-07-21  742  	}
4f4179fcf42087 Rafael J. Wysocki   2022-07-21  743  
4f4179fcf42087 Rafael J. Wysocki   2022-07-21  744  	/*
4f4179fcf42087 Rafael J. Wysocki   2022-07-21  745  	 * Disregard _CPC if the number of entries in the return pachage is not
4f4179fcf42087 Rafael J. Wysocki   2022-07-21  746  	 * as expected, but support future revisions being proper supersets of
4f4179fcf42087 Rafael J. Wysocki   2022-07-21  747  	 * the v3 and only causing more entries to be returned by _CPC.
4f4179fcf42087 Rafael J. Wysocki   2022-07-21  748  	 */
4f4179fcf42087 Rafael J. Wysocki   2022-07-21  749  	if ((cpc_rev == CPPC_V2_REV && num_ent != CPPC_V2_NUM_ENT) ||
4f4179fcf42087 Rafael J. Wysocki   2022-07-21  750  	    (cpc_rev == CPPC_V3_REV && num_ent != CPPC_V3_NUM_ENT) ||
4f4179fcf42087 Rafael J. Wysocki   2022-07-21  751  	    (cpc_rev > CPPC_V3_REV && num_ent <= CPPC_V3_NUM_ENT)) {
4f4179fcf42087 Rafael J. Wysocki   2022-07-21  752  		pr_debug("Unexpected number of _CPC return package entries (%d) for CPU:%d\n",
4f4179fcf42087 Rafael J. Wysocki   2022-07-21  753  			 num_ent, pr->id);
4f4179fcf42087 Rafael J. Wysocki   2022-07-21  754  		goto out_free;
4f4179fcf42087 Rafael J. Wysocki   2022-07-21  755  	}
4f4179fcf42087 Rafael J. Wysocki   2022-07-21  756  	if (cpc_rev > CPPC_V3_REV) {
4f4179fcf42087 Rafael J. Wysocki   2022-07-21  757  		num_ent = CPPC_V3_NUM_ENT;
4f4179fcf42087 Rafael J. Wysocki   2022-07-21  758  		cpc_rev = CPPC_V3_REV;
4f4179fcf42087 Rafael J. Wysocki   2022-07-21  759  	}
4f4179fcf42087 Rafael J. Wysocki   2022-07-21  760  
4f4179fcf42087 Rafael J. Wysocki   2022-07-21  761  	cpc_ptr->num_entries = num_ent;
4f4179fcf42087 Rafael J. Wysocki   2022-07-21  762  	cpc_ptr->version = cpc_rev;
337aadff8e4567 Ashwin Chaugule     2015-10-02  763  
337aadff8e4567 Ashwin Chaugule     2015-10-02  764  	/* Iterate through remaining entries in _CPC */
337aadff8e4567 Ashwin Chaugule     2015-10-02  765  	for (i = 2; i < num_ent; i++) {
337aadff8e4567 Ashwin Chaugule     2015-10-02  766  		cpc_obj = &out_obj->package.elements[i];
337aadff8e4567 Ashwin Chaugule     2015-10-02  767  
337aadff8e4567 Ashwin Chaugule     2015-10-02  768  		if (cpc_obj->type == ACPI_TYPE_INTEGER)	{
337aadff8e4567 Ashwin Chaugule     2015-10-02  769  			cpc_ptr->cpc_regs[i-2].type = ACPI_TYPE_INTEGER;
337aadff8e4567 Ashwin Chaugule     2015-10-02  770  			cpc_ptr->cpc_regs[i-2].cpc_entry.int_value = cpc_obj->integer.value;
337aadff8e4567 Ashwin Chaugule     2015-10-02  771  		} else if (cpc_obj->type == ACPI_TYPE_BUFFER) {
337aadff8e4567 Ashwin Chaugule     2015-10-02  772  			gas_t = (struct cpc_reg *)
337aadff8e4567 Ashwin Chaugule     2015-10-02  773  				cpc_obj->buffer.pointer;
337aadff8e4567 Ashwin Chaugule     2015-10-02  774  
337aadff8e4567 Ashwin Chaugule     2015-10-02  775  			/*
337aadff8e4567 Ashwin Chaugule     2015-10-02  776  			 * The PCC Subspace index is encoded inside
337aadff8e4567 Ashwin Chaugule     2015-10-02  777  			 * the CPC table entries. The same PCC index
337aadff8e4567 Ashwin Chaugule     2015-10-02  778  			 * will be used for all the PCC entries,
337aadff8e4567 Ashwin Chaugule     2015-10-02  779  			 * so extract it only once.
337aadff8e4567 Ashwin Chaugule     2015-10-02  780  			 */
337aadff8e4567 Ashwin Chaugule     2015-10-02  781  			if (gas_t->space_id == ACPI_ADR_SPACE_PLATFORM_COMM) {
85b1407bf6d2f4 George Cherian      2017-10-11  782  				if (pcc_subspace_id < 0) {
85b1407bf6d2f4 George Cherian      2017-10-11  783  					pcc_subspace_id = gas_t->access_width;
85b1407bf6d2f4 George Cherian      2017-10-11  784  					if (pcc_data_alloc(pcc_subspace_id))
85b1407bf6d2f4 George Cherian      2017-10-11  785  						goto out_free;
85b1407bf6d2f4 George Cherian      2017-10-11  786  				} else if (pcc_subspace_id != gas_t->access_width) {
f21a3509842294 Rafael J. Wysocki   2022-03-22  787  					pr_debug("Mismatched PCC ids in _CPC for CPU:%d\n",
f21a3509842294 Rafael J. Wysocki   2022-03-22  788  						 pr->id);
337aadff8e4567 Ashwin Chaugule     2015-10-02  789  					goto out_free;
337aadff8e4567 Ashwin Chaugule     2015-10-02  790  				}
5bbb86aa4b8d84 Ashwin Chaugule     2016-08-16  791  			} else if (gas_t->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
5bbb86aa4b8d84 Ashwin Chaugule     2016-08-16  792  				if (gas_t->address) {
5bbb86aa4b8d84 Ashwin Chaugule     2016-08-16  793  					void __iomem *addr;
2f4a4d63a193be Jarred White        2024-03-01  794  					size_t access_width;
5bbb86aa4b8d84 Ashwin Chaugule     2016-08-16  795  
0651ab90e4ade1 Pierre Gondois      2022-05-18  796  					if (!osc_cpc_flexible_adr_space_confirmed) {
0651ab90e4ade1 Pierre Gondois      2022-05-18  797  						pr_debug("Flexible address space capability not supported\n");
09073396ea62d0 Mario Limonciello   2022-07-15  798  						if (!cpc_supported_by_cpu())
0651ab90e4ade1 Pierre Gondois      2022-05-18  799  							goto out_free;
0651ab90e4ade1 Pierre Gondois      2022-05-18  800  					}
0651ab90e4ade1 Pierre Gondois      2022-05-18  801  
2f4a4d63a193be Jarred White        2024-03-01  802  					access_width = GET_BIT_WIDTH(gas_t) / 8;
2f4a4d63a193be Jarred White        2024-03-01  803  					addr = ioremap(gas_t->address, access_width);
5bbb86aa4b8d84 Ashwin Chaugule     2016-08-16  804  					if (!addr)
5bbb86aa4b8d84 Ashwin Chaugule     2016-08-16  805  						goto out_free;
5bbb86aa4b8d84 Ashwin Chaugule     2016-08-16  806  					cpc_ptr->cpc_regs[i-2].sys_mem_vaddr = addr;
5bbb86aa4b8d84 Ashwin Chaugule     2016-08-16  807  				}
a2c8f92bea5f8f Steven Noonan       2021-12-24  808  			} else if (gas_t->space_id == ACPI_ADR_SPACE_SYSTEM_IO) {
a2c8f92bea5f8f Steven Noonan       2021-12-24  809  				if (gas_t->access_width < 1 || gas_t->access_width > 3) {
a2c8f92bea5f8f Steven Noonan       2021-12-24  810  					/*
a2c8f92bea5f8f Steven Noonan       2021-12-24  811  					 * 1 = 8-bit, 2 = 16-bit, and 3 = 32-bit.
a2c8f92bea5f8f Steven Noonan       2021-12-24  812  					 * SystemIO doesn't implement 64-bit
a2c8f92bea5f8f Steven Noonan       2021-12-24  813  					 * registers.
a2c8f92bea5f8f Steven Noonan       2021-12-24  814  					 */
f21a3509842294 Rafael J. Wysocki   2022-03-22  815  					pr_debug("Invalid access width %d for SystemIO register in _CPC\n",
a2c8f92bea5f8f Steven Noonan       2021-12-24  816  						 gas_t->access_width);
a2c8f92bea5f8f Steven Noonan       2021-12-24  817  					goto out_free;
a2c8f92bea5f8f Steven Noonan       2021-12-24  818  				}
a2c8f92bea5f8f Steven Noonan       2021-12-24  819  				if (gas_t->address & OVER_16BTS_MASK) {
a2c8f92bea5f8f Steven Noonan       2021-12-24  820  					/* SystemIO registers use 16-bit integer addresses */
f21a3509842294 Rafael J. Wysocki   2022-03-22  821  					pr_debug("Invalid IO port %llu for SystemIO register in _CPC\n",
a2c8f92bea5f8f Steven Noonan       2021-12-24  822  						 gas_t->address);
a2c8f92bea5f8f Steven Noonan       2021-12-24  823  					goto out_free;
a2c8f92bea5f8f Steven Noonan       2021-12-24  824  				}
0651ab90e4ade1 Pierre Gondois      2022-05-18  825  				if (!osc_cpc_flexible_adr_space_confirmed) {
0651ab90e4ade1 Pierre Gondois      2022-05-18  826  					pr_debug("Flexible address space capability not supported\n");
09073396ea62d0 Mario Limonciello   2022-07-15  827  					if (!cpc_supported_by_cpu())
0651ab90e4ade1 Pierre Gondois      2022-05-18  828  						goto out_free;
0651ab90e4ade1 Pierre Gondois      2022-05-18  829  				}
5bbb86aa4b8d84 Ashwin Chaugule     2016-08-16  830  			} else {
a6cbcdd5ab5f24 Srinivas Pandruvada 2016-09-01  831  				if (gas_t->space_id != ACPI_ADR_SPACE_FIXED_HARDWARE || !cpc_ffh_supported()) {
a2c8f92bea5f8f Steven Noonan       2021-12-24  832  					/* Support only PCC, SystemMemory, SystemIO, and FFH type regs. */
f21a3509842294 Rafael J. Wysocki   2022-03-22  833  					pr_debug("Unsupported register type (%d) in _CPC\n",
f21a3509842294 Rafael J. Wysocki   2022-03-22  834  						 gas_t->space_id);
337aadff8e4567 Ashwin Chaugule     2015-10-02  835  					goto out_free;
337aadff8e4567 Ashwin Chaugule     2015-10-02  836  				}
a6cbcdd5ab5f24 Srinivas Pandruvada 2016-09-01  837  			}
337aadff8e4567 Ashwin Chaugule     2015-10-02  838  
337aadff8e4567 Ashwin Chaugule     2015-10-02  839  			cpc_ptr->cpc_regs[i-2].type = ACPI_TYPE_BUFFER;
337aadff8e4567 Ashwin Chaugule     2015-10-02  840  			memcpy(&cpc_ptr->cpc_regs[i-2].cpc_entry.reg, gas_t, sizeof(*gas_t));
337aadff8e4567 Ashwin Chaugule     2015-10-02  841  		} else {
f21a3509842294 Rafael J. Wysocki   2022-03-22  842  			pr_debug("Invalid entry type (%d) in _CPC for CPU:%d\n",
f21a3509842294 Rafael J. Wysocki   2022-03-22  843  				 i, pr->id);
337aadff8e4567 Ashwin Chaugule     2015-10-02  844  			goto out_free;
337aadff8e4567 Ashwin Chaugule     2015-10-02  845  		}
337aadff8e4567 Ashwin Chaugule     2015-10-02  846  	}
85b1407bf6d2f4 George Cherian      2017-10-11  847  	per_cpu(cpu_pcc_subspace_idx, pr->id) = pcc_subspace_id;
4773e77cdc9b3a Prashanth Prakash   2018-04-04  848  
4773e77cdc9b3a Prashanth Prakash   2018-04-04  849  	/*
4773e77cdc9b3a Prashanth Prakash   2018-04-04  850  	 * Initialize the remaining cpc_regs as unsupported.
4773e77cdc9b3a Prashanth Prakash   2018-04-04  851  	 * Example: In case FW exposes CPPC v2, the below loop will initialize
4773e77cdc9b3a Prashanth Prakash   2018-04-04  852  	 * LOWEST_FREQ and NOMINAL_FREQ regs as unsupported
4773e77cdc9b3a Prashanth Prakash   2018-04-04  853  	 */
4773e77cdc9b3a Prashanth Prakash   2018-04-04  854  	for (i = num_ent - 2; i < MAX_CPC_REG_ENT; i++) {
4773e77cdc9b3a Prashanth Prakash   2018-04-04  855  		cpc_ptr->cpc_regs[i].type = ACPI_TYPE_INTEGER;
4773e77cdc9b3a Prashanth Prakash   2018-04-04  856  		cpc_ptr->cpc_regs[i].cpc_entry.int_value = 0;
4773e77cdc9b3a Prashanth Prakash   2018-04-04  857  	}
4773e77cdc9b3a Prashanth Prakash   2018-04-04  858  
4773e77cdc9b3a Prashanth Prakash   2018-04-04  859  
337aadff8e4567 Ashwin Chaugule     2015-10-02  860  	/* Store CPU Logical ID */
337aadff8e4567 Ashwin Chaugule     2015-10-02  861  	cpc_ptr->cpu_id = pr->id;
337aadff8e4567 Ashwin Chaugule     2015-10-02  862  
337aadff8e4567 Ashwin Chaugule     2015-10-02  863  	/* Parse PSD data for this CPU */
337aadff8e4567 Ashwin Chaugule     2015-10-02  864  	ret = acpi_get_psd(cpc_ptr, handle);
337aadff8e4567 Ashwin Chaugule     2015-10-02  865  	if (ret)
337aadff8e4567 Ashwin Chaugule     2015-10-02  866  		goto out_free;
337aadff8e4567 Ashwin Chaugule     2015-10-02  867  
603fadf33604a2 Bjorn Helgaas       2019-03-25  868  	/* Register PCC channel once for all PCC subspace ID. */
85b1407bf6d2f4 George Cherian      2017-10-11  869  	if (pcc_subspace_id >= 0 && !pcc_data[pcc_subspace_id]->pcc_channel_acquired) {
85b1407bf6d2f4 George Cherian      2017-10-11  870  		ret = register_pcc_channel(pcc_subspace_id);
337aadff8e4567 Ashwin Chaugule     2015-10-02  871  		if (ret)
337aadff8e4567 Ashwin Chaugule     2015-10-02  872  			goto out_free;
8482ef8c6e684a Prakash, Prashanth  2016-08-16  873  
85b1407bf6d2f4 George Cherian      2017-10-11  874  		init_rwsem(&pcc_data[pcc_subspace_id]->pcc_lock);
85b1407bf6d2f4 George Cherian      2017-10-11  875  		init_waitqueue_head(&pcc_data[pcc_subspace_id]->pcc_write_wait_q);
337aadff8e4567 Ashwin Chaugule     2015-10-02  876  	}
337aadff8e4567 Ashwin Chaugule     2015-10-02  877  
337aadff8e4567 Ashwin Chaugule     2015-10-02  878  	/* Everything looks okay */
337aadff8e4567 Ashwin Chaugule     2015-10-02  879  	pr_debug("Parsed CPC struct for CPU: %d\n", pr->id);
337aadff8e4567 Ashwin Chaugule     2015-10-02  880  
158c998ea44ba3 Ashwin Chaugule     2016-08-16  881  	/* Add per logical CPU nodes for reading its feedback counters. */
158c998ea44ba3 Ashwin Chaugule     2016-08-16  882  	cpu_dev = get_cpu_device(pr->id);
501634759d55a5 Dan Carpenter       2016-11-30  883  	if (!cpu_dev) {
501634759d55a5 Dan Carpenter       2016-11-30  884  		ret = -EINVAL;
158c998ea44ba3 Ashwin Chaugule     2016-08-16  885  		goto out_free;
501634759d55a5 Dan Carpenter       2016-11-30  886  	}
158c998ea44ba3 Ashwin Chaugule     2016-08-16  887  
603fadf33604a2 Bjorn Helgaas       2019-03-25  888  	/* Plug PSD data into this CPU's CPC descriptor. */
28076483afac9d Rafael J. Wysocki   2016-12-10  889  	per_cpu(cpc_desc_ptr, pr->id) = cpc_ptr;
28076483afac9d Rafael J. Wysocki   2016-12-10  890  
158c998ea44ba3 Ashwin Chaugule     2016-08-16  891  	ret = kobject_init_and_add(&cpc_ptr->kobj, &cppc_ktype, &cpu_dev->kobj,
158c998ea44ba3 Ashwin Chaugule     2016-08-16  892  			"acpi_cppc");
28076483afac9d Rafael J. Wysocki   2016-12-10  893  	if (ret) {
28076483afac9d Rafael J. Wysocki   2016-12-10  894  		per_cpu(cpc_desc_ptr, pr->id) = NULL;
4d8be4bc94f74b Qiushi Wu           2020-05-27  895  		kobject_put(&cpc_ptr->kobj);
158c998ea44ba3 Ashwin Chaugule     2016-08-16  896  		goto out_free;
28076483afac9d Rafael J. Wysocki   2016-12-10  897  	}
158c998ea44ba3 Ashwin Chaugule     2016-08-16  898  
1132e6de11cfc3 Ionela Voinescu     2022-03-10  899  	arch_init_invariance_cppc();
41ea667227bad5 Nathan Fontenot     2020-11-12  900  
337aadff8e4567 Ashwin Chaugule     2015-10-02  901  	kfree(output.pointer);
337aadff8e4567 Ashwin Chaugule     2015-10-02  902  	return 0;
337aadff8e4567 Ashwin Chaugule     2015-10-02  903  
337aadff8e4567 Ashwin Chaugule     2015-10-02  904  out_free:
5bbb86aa4b8d84 Ashwin Chaugule     2016-08-16  905  	/* Free all the mapped sys mem areas for this CPU */
5bbb86aa4b8d84 Ashwin Chaugule     2016-08-16  906  	for (i = 2; i < cpc_ptr->num_entries; i++) {
5bbb86aa4b8d84 Ashwin Chaugule     2016-08-16  907  		void __iomem *addr = cpc_ptr->cpc_regs[i-2].sys_mem_vaddr;
5bbb86aa4b8d84 Ashwin Chaugule     2016-08-16  908  
5bbb86aa4b8d84 Ashwin Chaugule     2016-08-16  909  		if (addr)
5bbb86aa4b8d84 Ashwin Chaugule     2016-08-16  910  			iounmap(addr);
5bbb86aa4b8d84 Ashwin Chaugule     2016-08-16  911  	}
337aadff8e4567 Ashwin Chaugule     2015-10-02  912  	kfree(cpc_ptr);
337aadff8e4567 Ashwin Chaugule     2015-10-02  913  
337aadff8e4567 Ashwin Chaugule     2015-10-02  914  out_buf_free:
337aadff8e4567 Ashwin Chaugule     2015-10-02  915  	kfree(output.pointer);
337aadff8e4567 Ashwin Chaugule     2015-10-02  916  	return ret;
337aadff8e4567 Ashwin Chaugule     2015-10-02  917  }
337aadff8e4567 Ashwin Chaugule     2015-10-02  918  EXPORT_SYMBOL_GPL(acpi_cppc_processor_probe);
337aadff8e4567 Ashwin Chaugule     2015-10-02  919
Mario Limonciello June 18, 2024, 6:35 p.m. UTC | #4 
On 6/18/2024 13:30, Aaron Rainbolt wrote:
> On Tue, Jun 18, 2024 at 12:09:19PM -0500, Mario Limonciello wrote:
>> On 6/17/2024 21:54, Aaron Rainbolt wrote:
>>> acpi: Allow ignoring _OSC CPPC v2 bit via kernel parameter
>>>
>>> The _OSC is supposed to contain a bit indicating whether the hardware
>>> supports CPPC v2 or not. This bit is not always set, causing CPPC v2 to
>>> be considered absent. This results in severe single-core performance
>>> issues with the EEVDF scheduler.
>>>
>>> To work around this, provide a new kernel parameter,
>>> "processor.ignore_osc_cppc_bit", which may be used to ignore the _OSC
>>> CPPC v2 bit and act as if the bit was enabled. This allows CPPC to be
>>> properly detected even if not "enabled" by _OSC, allowing users with
>>> problematic hardware to obtain decent single-core performance.
>>>
>>> Tested-by: Michael Mikowski <mmikowski@kfocus.org>
>>> Signed-off-by: Aaron Rainbolt <arainbolt@kfocus.org>
>>
>> This sounds like a platform bug and if we do accept a patch like this I
>> think we need a lot more documentation about the situation.
> 
> It is a platform bug, yes. See my previous email,
> https://lore.kernel.org/linux-acpi/d01b0a1f-bd33-47fe-ab41-43843d8a374f@kfocus.org/T/#u
> (I meant to send this email as a reply to that one, but failed to do so.)
> 
>> Can you please share more information about your hardware:
>> 1) Manufacturer?
> 
> Carbon Systems, models Iridium 14 and Iridium 16.
> 
>> 2) CPU?
> 
> Intel Core i5-13500H.
> 
>> 3) Manufacturer firmware version?
> 
> The systems use an AMI BIOS with version N.1.10CAR01 according to
> dmidecode. This is the latest BIOS available from the manufacturer.
> 
>> 4) If it's AMD what's the AGESA version?
> 
> Both affected systems are Intel-based and use heterogenous cores, not AMD.
> 
>> And most importantly do you have the latest system firmware version from
>> your manufacturer?  If not; please upgrade that first.
> 
> We are using the latest firmware. (We're trying to work with the ODM to
> potentially get a firmware update, but since this affects more than just
> us and a firmware update may not be possible for everyone, this would
> likely be worth providing a kernel-level workaround for.)
> 
> I can easily provide more detailed information - would the full output of
> 'dmidecode' and 'acpidump' be useful?

Does your BIOS offer any options for these?

Intel(R) SpeedStep(TM)
Intel Speed Shift Technology(TM)

I believe you need those enabled for this to work properly.
Aaron Rainbolt June 18, 2024, 6:52 p.m. UTC | #5 
On Tue, Jun 18, 2024 at 01:35:57PM -0500, Mario Limonciello wrote:
> On 6/18/2024 13:30, Aaron Rainbolt wrote:
> > On Tue, Jun 18, 2024 at 12:09:19PM -0500, Mario Limonciello wrote:
> > > On 6/17/2024 21:54, Aaron Rainbolt wrote:
> > > > acpi: Allow ignoring _OSC CPPC v2 bit via kernel parameter
> > > > 
> > > > The _OSC is supposed to contain a bit indicating whether the hardware
> > > > supports CPPC v2 or not. This bit is not always set, causing CPPC v2 to
> > > > be considered absent. This results in severe single-core performance
> > > > issues with the EEVDF scheduler.
> > > > 
> > > > To work around this, provide a new kernel parameter,
> > > > "processor.ignore_osc_cppc_bit", which may be used to ignore the _OSC
> > > > CPPC v2 bit and act as if the bit was enabled. This allows CPPC to be
> > > > properly detected even if not "enabled" by _OSC, allowing users with
> > > > problematic hardware to obtain decent single-core performance.
> > > > 
> > > > Tested-by: Michael Mikowski <mmikowski@kfocus.org>
> > > > Signed-off-by: Aaron Rainbolt <arainbolt@kfocus.org>
> > > 
> > > This sounds like a platform bug and if we do accept a patch like this I
> > > think we need a lot more documentation about the situation.
> > 
> > It is a platform bug, yes. See my previous email,
> > https://lore.kernel.org/linux-acpi/d01b0a1f-bd33-47fe-ab41-43843d8a374f@kfocus.org/T/#u
> > (I meant to send this email as a reply to that one, but failed to do so.)
> > 
> > > Can you please share more information about your hardware:
> > > 1) Manufacturer?
> > 
> > Carbon Systems, models Iridium 14 and Iridium 16.
> > 
> > > 2) CPU?
> > 
> > Intel Core i5-13500H.
> > 
> > > 3) Manufacturer firmware version?
> > 
> > The systems use an AMI BIOS with version N.1.10CAR01 according to
> > dmidecode. This is the latest BIOS available from the manufacturer.
> > 
> > > 4) If it's AMD what's the AGESA version?
> > 
> > Both affected systems are Intel-based and use heterogenous cores, not AMD.
> > 
> > > And most importantly do you have the latest system firmware version from
> > > your manufacturer?  If not; please upgrade that first.
> > 
> > We are using the latest firmware. (We're trying to work with the ODM to
> > potentially get a firmware update, but since this affects more than just
> > us and a firmware update may not be possible for everyone, this would
> > likely be worth providing a kernel-level workaround for.)
> > 
> > I can easily provide more detailed information - would the full output of
> > 'dmidecode' and 'acpidump' be useful?
> 
> Does your BIOS offer any options for these?
> 
> Intel(R) SpeedStep(TM)
> Intel Speed Shift Technology(TM)
> 
> I believe you need those enabled for this to work properly.

Neither option is available in the BIOS settings UI, however our ODM
confirmed that both Intel Speed Shift Technology and Intel Turbo Boost Max
Technology 3.0 are enabled by default. They did not mention SpeedStep,
but I assume SpeedStep is working since frequency scaling in general
works and the kernel patch fixes the issue.

In case it's helpful to know, when booting without the kernel patch or
without 'processor.ignore_osc_cppc_bit=1' set, neither of
'/proc/sys/kernel/sched_itmt_enabled' or
'/sys/devices/system/cpu/cpu*/acpi_cppc/' exist. When booting with the
patched kernel and with the kernel parameter set, the 'sched_itmt_enabled'
file appears and the 'acpi_cppc" directory for each CPU appears.
Mario Limonciello June 18, 2024, 6:58 p.m. UTC | #6 
On 6/18/2024 13:52, Aaron Rainbolt wrote:
> On Tue, Jun 18, 2024 at 01:35:57PM -0500, Mario Limonciello wrote:
>> On 6/18/2024 13:30, Aaron Rainbolt wrote:
>>> On Tue, Jun 18, 2024 at 12:09:19PM -0500, Mario Limonciello wrote:
>>>> On 6/17/2024 21:54, Aaron Rainbolt wrote:
>>>>> acpi: Allow ignoring _OSC CPPC v2 bit via kernel parameter
>>>>>
>>>>> The _OSC is supposed to contain a bit indicating whether the hardware
>>>>> supports CPPC v2 or not. This bit is not always set, causing CPPC v2 to
>>>>> be considered absent. This results in severe single-core performance
>>>>> issues with the EEVDF scheduler.
>>>>>
>>>>> To work around this, provide a new kernel parameter,
>>>>> "processor.ignore_osc_cppc_bit", which may be used to ignore the _OSC
>>>>> CPPC v2 bit and act as if the bit was enabled. This allows CPPC to be
>>>>> properly detected even if not "enabled" by _OSC, allowing users with
>>>>> problematic hardware to obtain decent single-core performance.
>>>>>
>>>>> Tested-by: Michael Mikowski <mmikowski@kfocus.org>
>>>>> Signed-off-by: Aaron Rainbolt <arainbolt@kfocus.org>
>>>>
>>>> This sounds like a platform bug and if we do accept a patch like this I
>>>> think we need a lot more documentation about the situation.
>>>
>>> It is a platform bug, yes. See my previous email,
>>> https://lore.kernel.org/linux-acpi/d01b0a1f-bd33-47fe-ab41-43843d8a374f@kfocus.org/T/#u
>>> (I meant to send this email as a reply to that one, but failed to do so.)
>>>
>>>> Can you please share more information about your hardware:
>>>> 1) Manufacturer?
>>>
>>> Carbon Systems, models Iridium 14 and Iridium 16.
>>>
>>>> 2) CPU?
>>>
>>> Intel Core i5-13500H.
>>>
>>>> 3) Manufacturer firmware version?
>>>
>>> The systems use an AMI BIOS with version N.1.10CAR01 according to
>>> dmidecode. This is the latest BIOS available from the manufacturer.
>>>
>>>> 4) If it's AMD what's the AGESA version?
>>>
>>> Both affected systems are Intel-based and use heterogenous cores, not AMD.
>>>
>>>> And most importantly do you have the latest system firmware version from
>>>> your manufacturer?  If not; please upgrade that first.
>>>
>>> We are using the latest firmware. (We're trying to work with the ODM to
>>> potentially get a firmware update, but since this affects more than just
>>> us and a firmware update may not be possible for everyone, this would
>>> likely be worth providing a kernel-level workaround for.)
>>>
>>> I can easily provide more detailed information - would the full output of
>>> 'dmidecode' and 'acpidump' be useful?
>>
>> Does your BIOS offer any options for these?
>>
>> Intel(R) SpeedStep(TM)
>> Intel Speed Shift Technology(TM)
>>
>> I believe you need those enabled for this to work properly.
> 
> Neither option is available in the BIOS settings UI, however our ODM
> confirmed that both Intel Speed Shift Technology and Intel Turbo Boost Max
> Technology 3.0 are enabled by default. They did not mention SpeedStep,
> but I assume SpeedStep is working since frequency scaling in general
> works and the kernel patch fixes the issue.

Got it.  If those are enabled I think it would be good to get comments 
from Rafael and Srinivas about your specific situation then.

But regarding the patch, if they are agreeable to this "kind" of knob 
for debugging I personally think it's better to have 
cpc_supported_by_cpu() look at the kernel command line than plumb 
arguments from the module down through every function.

> 
> In case it's helpful to know, when booting without the kernel patch or
> without 'processor.ignore_osc_cppc_bit=1' set, neither of
> '/proc/sys/kernel/sched_itmt_enabled' or
> '/sys/devices/system/cpu/cpu*/acpi_cppc/' exist. When booting with the
> patched kernel and with the kernel parameter set, the 'sched_itmt_enabled'
> file appears and the 'acpi_cppc" directory for each CPU appears.
Aaron Rainbolt June 18, 2024, 7:25 p.m. UTC | #7 
On Tue, Jun 18, 2024 at 01:58:07PM -0500, Mario Limonciello wrote:
> On 6/18/2024 13:52, Aaron Rainbolt wrote:
> > On Tue, Jun 18, 2024 at 01:35:57PM -0500, Mario Limonciello wrote:
> > > On 6/18/2024 13:30, Aaron Rainbolt wrote:
> > > > On Tue, Jun 18, 2024 at 12:09:19PM -0500, Mario Limonciello wrote:
> > > > > On 6/17/2024 21:54, Aaron Rainbolt wrote:
> > > > > > acpi: Allow ignoring _OSC CPPC v2 bit via kernel parameter
> > > > > > 
> > > > > > The _OSC is supposed to contain a bit indicating whether the hardware
> > > > > > supports CPPC v2 or not. This bit is not always set, causing CPPC v2 to
> > > > > > be considered absent. This results in severe single-core performance
> > > > > > issues with the EEVDF scheduler.
> > > > > > 
> > > > > > To work around this, provide a new kernel parameter,
> > > > > > "processor.ignore_osc_cppc_bit", which may be used to ignore the _OSC
> > > > > > CPPC v2 bit and act as if the bit was enabled. This allows CPPC to be
> > > > > > properly detected even if not "enabled" by _OSC, allowing users with
> > > > > > problematic hardware to obtain decent single-core performance.
> > > > > > 
> > > > > > Tested-by: Michael Mikowski <mmikowski@kfocus.org>
> > > > > > Signed-off-by: Aaron Rainbolt <arainbolt@kfocus.org>
> > > > > 
> > > > > This sounds like a platform bug and if we do accept a patch like this I
> > > > > think we need a lot more documentation about the situation.
> > > > 
> > > > It is a platform bug, yes. See my previous email,
> > > > https://lore.kernel.org/linux-acpi/d01b0a1f-bd33-47fe-ab41-43843d8a374f@kfocus.org/T/#u
> > > > (I meant to send this email as a reply to that one, but failed to do so.)
> > > > 
> > > > > Can you please share more information about your hardware:
> > > > > 1) Manufacturer?
> > > > 
> > > > Carbon Systems, models Iridium 14 and Iridium 16.
> > > > 
> > > > > 2) CPU?
> > > > 
> > > > Intel Core i5-13500H.
> > > > 
> > > > > 3) Manufacturer firmware version?
> > > > 
> > > > The systems use an AMI BIOS with version N.1.10CAR01 according to
> > > > dmidecode. This is the latest BIOS available from the manufacturer.
> > > > 
> > > > > 4) If it's AMD what's the AGESA version?
> > > > 
> > > > Both affected systems are Intel-based and use heterogenous cores, not AMD.
> > > > 
> > > > > And most importantly do you have the latest system firmware version from
> > > > > your manufacturer?  If not; please upgrade that first.
> > > > 
> > > > We are using the latest firmware. (We're trying to work with the ODM to
> > > > potentially get a firmware update, but since this affects more than just
> > > > us and a firmware update may not be possible for everyone, this would
> > > > likely be worth providing a kernel-level workaround for.)
> > > > 
> > > > I can easily provide more detailed information - would the full output of
> > > > 'dmidecode' and 'acpidump' be useful?
> > > 
> > > Does your BIOS offer any options for these?
> > > 
> > > Intel(R) SpeedStep(TM)
> > > Intel Speed Shift Technology(TM)
> > > 
> > > I believe you need those enabled for this to work properly.
> > 
> > Neither option is available in the BIOS settings UI, however our ODM
> > confirmed that both Intel Speed Shift Technology and Intel Turbo Boost Max
> > Technology 3.0 are enabled by default. They did not mention SpeedStep,
> > but I assume SpeedStep is working since frequency scaling in general
> > works and the kernel patch fixes the issue.
> 
> Got it.  If those are enabled I think it would be good to get comments from
> Rafael and Srinivas about your specific situation then.
> 
> But regarding the patch, if they are agreeable to this "kind" of knob for
> debugging I personally think it's better to have cpc_supported_by_cpu() look
> at the kernel command line than plumb arguments from the module down through
> every function.

Just to be clear since I'm not all too familiar with how kernel params work,
should core_param be used here? Or is there a variable that allows
accessing the entire command line to look through it? I don't think I can
use module_param in 'arch/x86/kernel/acpi/cppc.c', core_param has a
comment over it describing it as "historical" so I don't think I should
use it, and early_param looks like something one is only supposed to use
in code that runs very early at kernel startup. I can probably figure it
out on my own, but a quick pointer would be helpful.
Mario Limonciello June 18, 2024, 7:27 p.m. UTC | #8 
On 6/18/2024 14:25, Aaron Rainbolt wrote:
> On Tue, Jun 18, 2024 at 01:58:07PM -0500, Mario Limonciello wrote:
>> On 6/18/2024 13:52, Aaron Rainbolt wrote:
>>> On Tue, Jun 18, 2024 at 01:35:57PM -0500, Mario Limonciello wrote:
>>>> On 6/18/2024 13:30, Aaron Rainbolt wrote:
>>>>> On Tue, Jun 18, 2024 at 12:09:19PM -0500, Mario Limonciello wrote:
>>>>>> On 6/17/2024 21:54, Aaron Rainbolt wrote:
>>>>>>> acpi: Allow ignoring _OSC CPPC v2 bit via kernel parameter
>>>>>>>
>>>>>>> The _OSC is supposed to contain a bit indicating whether the hardware
>>>>>>> supports CPPC v2 or not. This bit is not always set, causing CPPC v2 to
>>>>>>> be considered absent. This results in severe single-core performance
>>>>>>> issues with the EEVDF scheduler.
>>>>>>>
>>>>>>> To work around this, provide a new kernel parameter,
>>>>>>> "processor.ignore_osc_cppc_bit", which may be used to ignore the _OSC
>>>>>>> CPPC v2 bit and act as if the bit was enabled. This allows CPPC to be
>>>>>>> properly detected even if not "enabled" by _OSC, allowing users with
>>>>>>> problematic hardware to obtain decent single-core performance.
>>>>>>>
>>>>>>> Tested-by: Michael Mikowski <mmikowski@kfocus.org>
>>>>>>> Signed-off-by: Aaron Rainbolt <arainbolt@kfocus.org>
>>>>>>
>>>>>> This sounds like a platform bug and if we do accept a patch like this I
>>>>>> think we need a lot more documentation about the situation.
>>>>>
>>>>> It is a platform bug, yes. See my previous email,
>>>>> https://lore.kernel.org/linux-acpi/d01b0a1f-bd33-47fe-ab41-43843d8a374f@kfocus.org/T/#u
>>>>> (I meant to send this email as a reply to that one, but failed to do so.)
>>>>>
>>>>>> Can you please share more information about your hardware:
>>>>>> 1) Manufacturer?
>>>>>
>>>>> Carbon Systems, models Iridium 14 and Iridium 16.
>>>>>
>>>>>> 2) CPU?
>>>>>
>>>>> Intel Core i5-13500H.
>>>>>
>>>>>> 3) Manufacturer firmware version?
>>>>>
>>>>> The systems use an AMI BIOS with version N.1.10CAR01 according to
>>>>> dmidecode. This is the latest BIOS available from the manufacturer.
>>>>>
>>>>>> 4) If it's AMD what's the AGESA version?
>>>>>
>>>>> Both affected systems are Intel-based and use heterogenous cores, not AMD.
>>>>>
>>>>>> And most importantly do you have the latest system firmware version from
>>>>>> your manufacturer?  If not; please upgrade that first.
>>>>>
>>>>> We are using the latest firmware. (We're trying to work with the ODM to
>>>>> potentially get a firmware update, but since this affects more than just
>>>>> us and a firmware update may not be possible for everyone, this would
>>>>> likely be worth providing a kernel-level workaround for.)
>>>>>
>>>>> I can easily provide more detailed information - would the full output of
>>>>> 'dmidecode' and 'acpidump' be useful?
>>>>
>>>> Does your BIOS offer any options for these?
>>>>
>>>> Intel(R) SpeedStep(TM)
>>>> Intel Speed Shift Technology(TM)
>>>>
>>>> I believe you need those enabled for this to work properly.
>>>
>>> Neither option is available in the BIOS settings UI, however our ODM
>>> confirmed that both Intel Speed Shift Technology and Intel Turbo Boost Max
>>> Technology 3.0 are enabled by default. They did not mention SpeedStep,
>>> but I assume SpeedStep is working since frequency scaling in general
>>> works and the kernel patch fixes the issue.
>>
>> Got it.  If those are enabled I think it would be good to get comments from
>> Rafael and Srinivas about your specific situation then.
>>
>> But regarding the patch, if they are agreeable to this "kind" of knob for
>> debugging I personally think it's better to have cpc_supported_by_cpu() look
>> at the kernel command line than plumb arguments from the module down through
>> every function.
> 
> Just to be clear since I'm not all too familiar with how kernel params work,
> should core_param be used here? Or is there a variable that allows
> accessing the entire command line to look through it? I don't think I can
> use module_param in 'arch/x86/kernel/acpi/cppc.c', core_param has a
> comment over it describing it as "historical" so I don't think I should
> use it, and early_param looks like something one is only supposed to use
> in code that runs very early at kernel startup. I can probably figure it
> out on my own, but a quick pointer would be helpful.

early_param is how I would do it.

You can save it to a static boolean global variable in that file.  Make 
sure that you update documentation about the new early_param though too.
Aaron Rainbolt June 18, 2024, 8:25 p.m. UTC | #9 
acpi: Allow ignoring _OSC CPPC v2 bit via kernel parameter

The _OSC is supposed to contain a bit indicating whether the hardware
supports CPPC v2 or not. This bit is not always set, causing CPPC v2 to
be considered absent. This results in severe single-core performance
issues with the EEVDF scheduler on heterogenous-core Intel processors.

To work around this, provide a new kernel parameter, "ignore_osc_cppc_bit",
which may be used to ignore the _OSC CPPC v2 bit and act as if the bit was
enabled. This allows CPPC to be properly detected even if not "enabled" by
_OSC, allowing users with problematic hardware to obtain decent single-core
performance.

Signed-off-by: Aaron Rainbolt <arainbolt@kfocus.org>

---

V1 -> V2: Rewrite to work in cpc_supported_by_cpu.

RFC: I have not yet tested this patch to ensure it functions properly,
 nor have I attempted to compile it against mainline. My system takes
 a couple of hours or so to build a kernel, and I'd like to submit this
 for feedback now and test once it's sent.

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index b600df82669d..af2d8973ba3a 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -2063,6 +2063,12 @@
 			could change it dynamically, usually by
 			/sys/module/printk/parameters/ignore_loglevel.
 
+	ignore_osc_cppc_bit
+			Assume CPPC is present and ignore the CPPC v2 bit from
+			the ACPI _OSC method. This is useful for working
+			around buggy firmware where CPPC is supported, but
+			_OSC incorrectly reports it as being absent.
+
 	ignore_rlimit_data
 			Ignore RLIMIT_DATA setting for data mappings,
 			print warning at first misuse.  Can be changed via
diff --git a/arch/x86/kernel/acpi/cppc.c b/arch/x86/kernel/acpi/cppc.c
index ff8f25faca3d..7346a25e68ce 100644
--- a/arch/x86/kernel/acpi/cppc.c
+++ b/arch/x86/kernel/acpi/cppc.c
@@ -11,6 +11,14 @@
 
 /* Refer to drivers/acpi/cppc_acpi.c for the description of functions */
 
+static bool ignore_osc_cppc_bit;
+static int __init parse_ignore_osc_cppc_bit(char *arg)
+{
+	ignore_osc_cppc_bit = true;
+	return 0;
+}
+early_param("ignore_osc_cppc_bit", parse_ignore_osc_cppc_bit);
+
 bool cpc_supported_by_cpu(void)
 {
 	switch (boot_cpu_data.x86_vendor) {
@@ -24,6 +32,10 @@ bool cpc_supported_by_cpu(void)
 			return true;
 		return boot_cpu_has(X86_FEATURE_CPPC);
 	}
+
+	if (ignore_osc_cppc_bit) {
+		return true;
+	}
 	return false;
 }
Aaron Rainbolt June 18, 2024, 8:58 p.m. UTC | #10 
On Tue, Jun 18, 2024 at 03:25:36PM -0500, Aaron Rainbolt wrote:
> acpi: Allow ignoring _OSC CPPC v2 bit via kernel parameter
> 
> The _OSC is supposed to contain a bit indicating whether the hardware
> supports CPPC v2 or not. This bit is not always set, causing CPPC v2 to
> be considered absent. This results in severe single-core performance
> issues with the EEVDF scheduler on heterogenous-core Intel processors.
> 
> To work around this, provide a new kernel parameter, "ignore_osc_cppc_bit",
> which may be used to ignore the _OSC CPPC v2 bit and act as if the bit was
> enabled. This allows CPPC to be properly detected even if not "enabled" by
> _OSC, allowing users with problematic hardware to obtain decent single-core
> performance.
> 
> Signed-off-by: Aaron Rainbolt <arainbolt@kfocus.org>
> 
> ---
> 
> V1 -> V2: Rewrite to work in cpc_supported_by_cpu.
> 
> RFC: I have not yet tested this patch to ensure it functions properly,
>  nor have I attempted to compile it against mainline. My system takes
>  a couple of hours or so to build a kernel, and I'd like to submit this
>  for feedback now and test once it's sent.

I sped up my build by using a defconfig rather than a default menuconfig
(still learning how these things work), and confirmed that the patch does
build against 6.10~rc4. Will report back test results hopefully soon.
Mario Limonciello June 18, 2024, 9:24 p.m. UTC | #11 
On 6/18/2024 15:25, Aaron Rainbolt wrote:
> acpi: Allow ignoring _OSC CPPC v2 bit via kernel parameter
> 
> The _OSC is supposed to contain a bit indicating whether the hardware
> supports CPPC v2 or not. This bit is not always set, causing CPPC v2 to
> be considered absent. This results in severe single-core performance
> issues with the EEVDF scheduler on heterogenous-core Intel processors.
> 
> To work around this, provide a new kernel parameter, "ignore_osc_cppc_bit",
> which may be used to ignore the _OSC CPPC v2 bit and act as if the bit was
> enabled. This allows CPPC to be properly detected even if not "enabled" by
> _OSC, allowing users with problematic hardware to obtain decent single-core
> performance.
> 
> Signed-off-by: Aaron Rainbolt <arainbolt@kfocus.org>
> 
> ---
> 
> V1 -> V2: Rewrite to work in cpc_supported_by_cpu.
> 
> RFC: I have not yet tested this patch to ensure it functions properly,
>   nor have I attempted to compile it against mainline. My system takes
>   a couple of hours or so to build a kernel, and I'd like to submit this
>   for feedback now and test once it's sent.

Thanks, this matches what I suggested, hopefully it works when you test it.

One comment below though.

> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index b600df82669d..af2d8973ba3a 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -2063,6 +2063,12 @@
>   			could change it dynamically, usually by
>   			/sys/module/printk/parameters/ignore_loglevel.
>   
> +	ignore_osc_cppc_bit
> +			Assume CPPC is present and ignore the CPPC v2 bit from
> +			the ACPI _OSC method. This is useful for working
> +			around buggy firmware where CPPC is supported, but
> +			_OSC incorrectly reports it as being absent.
> +
>   	ignore_rlimit_data
>   			Ignore RLIMIT_DATA setting for data mappings,
>   			print warning at first misuse.  Can be changed via
> diff --git a/arch/x86/kernel/acpi/cppc.c b/arch/x86/kernel/acpi/cppc.c
> index ff8f25faca3d..7346a25e68ce 100644
> --- a/arch/x86/kernel/acpi/cppc.c
> +++ b/arch/x86/kernel/acpi/cppc.c
> @@ -11,6 +11,14 @@
>   
>   /* Refer to drivers/acpi/cppc_acpi.c for the description of functions */
>   
> +static bool ignore_osc_cppc_bit;
> +static int __init parse_ignore_osc_cppc_bit(char *arg)
> +{
> +	ignore_osc_cppc_bit = true;
> +	return 0;
> +}
> +early_param("ignore_osc_cppc_bit", parse_ignore_osc_cppc_bit);
> +
>   bool cpc_supported_by_cpu(void)
>   {
>   	switch (boot_cpu_data.x86_vendor) {
> @@ -24,6 +32,10 @@ bool cpc_supported_by_cpu(void)
>   			return true;
>   		return boot_cpu_has(X86_FEATURE_CPPC);
>   	}
> +
> +	if (ignore_osc_cppc_bit) {
> +		return true;
> +	}

I think you should move this check before the switch statement.
The reason is that such a workaround could then apply to any CPU
vendors and models that are AMD or Hygon too.

>   	return false;
>   }
>
Aaron Rainbolt June 18, 2024, 9:47 p.m. UTC | #12 
On Tue, Jun 18, 2024 at 04:24:22PM -0500, Mario Limonciello wrote:
> On 6/18/2024 15:25, Aaron Rainbolt wrote:
> > acpi: Allow ignoring _OSC CPPC v2 bit via kernel parameter
> > 
> > The _OSC is supposed to contain a bit indicating whether the hardware
> > supports CPPC v2 or not. This bit is not always set, causing CPPC v2 to
> > be considered absent. This results in severe single-core performance
> > issues with the EEVDF scheduler on heterogenous-core Intel processors.
> > 
> > To work around this, provide a new kernel parameter, "ignore_osc_cppc_bit",
> > which may be used to ignore the _OSC CPPC v2 bit and act as if the bit was
> > enabled. This allows CPPC to be properly detected even if not "enabled" by
> > _OSC, allowing users with problematic hardware to obtain decent single-core
> > performance.
> > 
> > Signed-off-by: Aaron Rainbolt <arainbolt@kfocus.org>
> > 
> > ---
> > 
> > V1 -> V2: Rewrite to work in cpc_supported_by_cpu.
> > 
> > RFC: I have not yet tested this patch to ensure it functions properly,
> >   nor have I attempted to compile it against mainline. My system takes
> >   a couple of hours or so to build a kernel, and I'd like to submit this
> >   for feedback now and test once it's sent.
> 
> Thanks, this matches what I suggested, hopefully it works when you test it.
> 
> One comment below though.
> 
> > 
> > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > index b600df82669d..af2d8973ba3a 100644
> > --- a/Documentation/admin-guide/kernel-parameters.txt
> > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > @@ -2063,6 +2063,12 @@
> >   			could change it dynamically, usually by
> >   			/sys/module/printk/parameters/ignore_loglevel.
> > +	ignore_osc_cppc_bit
> > +			Assume CPPC is present and ignore the CPPC v2 bit from
> > +			the ACPI _OSC method. This is useful for working
> > +			around buggy firmware where CPPC is supported, but
> > +			_OSC incorrectly reports it as being absent.
> > +
> >   	ignore_rlimit_data
> >   			Ignore RLIMIT_DATA setting for data mappings,
> >   			print warning at first misuse.  Can be changed via
> > diff --git a/arch/x86/kernel/acpi/cppc.c b/arch/x86/kernel/acpi/cppc.c
> > index ff8f25faca3d..7346a25e68ce 100644
> > --- a/arch/x86/kernel/acpi/cppc.c
> > +++ b/arch/x86/kernel/acpi/cppc.c
> > @@ -11,6 +11,14 @@
> >   /* Refer to drivers/acpi/cppc_acpi.c for the description of functions */
> > +static bool ignore_osc_cppc_bit;
> > +static int __init parse_ignore_osc_cppc_bit(char *arg)
> > +{
> > +	ignore_osc_cppc_bit = true;
> > +	return 0;
> > +}
> > +early_param("ignore_osc_cppc_bit", parse_ignore_osc_cppc_bit);
> > +
> >   bool cpc_supported_by_cpu(void)
> >   {
> >   	switch (boot_cpu_data.x86_vendor) {
> > @@ -24,6 +32,10 @@ bool cpc_supported_by_cpu(void)
> >   			return true;
> >   		return boot_cpu_has(X86_FEATURE_CPPC);
> >   	}
> > +
> > +	if (ignore_osc_cppc_bit) {
> > +		return true;
> > +	}
> 
> I think you should move this check before the switch statement.
> The reason is that such a workaround could then apply to any CPU
> vendors and models that are AMD or Hygon too.

Oh good catch, I thought it would apply to everyone but missed an extra
'return' in the switch statement. I'll make sure to fix that in v3.

> >   	return false;
> >   }
>
Aaron Rainbolt June 19, 2024, 4:33 a.m. UTC | #13 
acpi: Allow ignoring _OSC CPPC v2 bit via kernel parameter

The _OSC is supposed to contain a bit indicating whether the hardware
supports CPPC v2 or not. This bit is not always set, causing CPPC v2 to
be considered absent. This results in severe single-core performance
issues with the EEVDF scheduler on heterogenous-core Intel processors.

To work around this, provide a new kernel parameter, "ignore_osc_cppc_bit",
which may be used to ignore the _OSC CPPC v2 bit and act as if the bit was
enabled. This allows CPPC to be properly detected even if not "enabled" by
_OSC, allowing users with problematic hardware to obtain decent single-core
performance.

Tested-by: Michael Mikowski <mmikowski@kfocus.org>
Signed-off-by: Aaron Rainbolt <arainbolt@kfocus.org>

---

V2 -> V3: Move bit ignore to before switch.
V1 -> V2: Rewrite to work in cpc_supported_by_cpu.

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index b600df82669d..af2d8973ba3a 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -2063,6 +2063,12 @@
                        could change it dynamically, usually by
                        /sys/module/printk/parameters/ignore_loglevel.

+       ignore_osc_cppc_bit
+                       Assume CPPC is present and ignore the CPPC v2 bit from
+                       the ACPI _OSC method. This is useful for working
+                       around buggy firmware where CPPC is supported, but
+                       _OSC incorrectly reports it as being absent.
+
        ignore_rlimit_data
                        Ignore RLIMIT_DATA setting for data mappings,
                        print warning at first misuse.  Can be changed via
diff --git a/arch/x86/kernel/acpi/cppc.c b/arch/x86/kernel/acpi/cppc.c
index ff8f25faca3d..0ca1eac826af 100644
--- a/arch/x86/kernel/acpi/cppc.c
+++ b/arch/x86/kernel/acpi/cppc.c
@@ -11,8 +11,20 @@

 /* Refer to drivers/acpi/cppc_acpi.c for the description of functions */

+static bool ignore_osc_cppc_bit;
+static int __init parse_ignore_osc_cppc_bit(char *arg)
+{
+       ignore_osc_cppc_bit = true;
+       return 0;
+}
+early_param("ignore_osc_cppc_bit", parse_ignore_osc_cppc_bit);
+
 bool cpc_supported_by_cpu(void)
 {
+       if (ignore_osc_cppc_bit) {
+               return true;
+       }
+
        switch (boot_cpu_data.x86_vendor) {
        case X86_VENDOR_AMD:
        case X86_VENDOR_HYGON:
Mario Limonciello June 19, 2024, 5:08 a.m. UTC | #14 
On 6/18/2024 23:33, Aaron Rainbolt wrote:
> acpi: Allow ignoring _OSC CPPC v2 bit via kernel parameter
> 
> The _OSC is supposed to contain a bit indicating whether the hardware
> supports CPPC v2 or not. This bit is not always set, causing CPPC v2 to
> be considered absent. This results in severe single-core performance
> issues with the EEVDF scheduler on heterogenous-core Intel processors.

heterogeneous

> 
> To work around this, provide a new kernel parameter, "ignore_osc_cppc_bit",
> which may be used to ignore the _OSC CPPC v2 bit and act as if the bit was
> enabled. This allows CPPC to be properly detected even if not "enabled" by
> _OSC, allowing users with problematic hardware to obtain decent single-core
> performance.
> 
> Tested-by: Michael Mikowski <mmikowski@kfocus.org>
> Signed-off-by: Aaron Rainbolt <arainbolt@kfocus.org>

One minor typo above, but it LGTM.

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>

Up to Rafael if he wants something like this in place though.

> 
> ---
> 
> V2 -> V3: Move bit ignore to before switch.
> V1 -> V2: Rewrite to work in cpc_supported_by_cpu.
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index b600df82669d..af2d8973ba3a 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -2063,6 +2063,12 @@
>                          could change it dynamically, usually by
>                          /sys/module/printk/parameters/ignore_loglevel.
> 
> +       ignore_osc_cppc_bit
> +                       Assume CPPC is present and ignore the CPPC v2 bit from
> +                       the ACPI _OSC method. This is useful for working
> +                       around buggy firmware where CPPC is supported, but
> +                       _OSC incorrectly reports it as being absent.
> +
>          ignore_rlimit_data
>                          Ignore RLIMIT_DATA setting for data mappings,
>                          print warning at first misuse.  Can be changed via
> diff --git a/arch/x86/kernel/acpi/cppc.c b/arch/x86/kernel/acpi/cppc.c
> index ff8f25faca3d..0ca1eac826af 100644
> --- a/arch/x86/kernel/acpi/cppc.c
> +++ b/arch/x86/kernel/acpi/cppc.c
> @@ -11,8 +11,20 @@
> 
>   /* Refer to drivers/acpi/cppc_acpi.c for the description of functions */
> 
> +static bool ignore_osc_cppc_bit;
> +static int __init parse_ignore_osc_cppc_bit(char *arg)
> +{
> +       ignore_osc_cppc_bit = true;
> +       return 0;
> +}
> +early_param("ignore_osc_cppc_bit", parse_ignore_osc_cppc_bit);
> +
>   bool cpc_supported_by_cpu(void)
>   {
> +       if (ignore_osc_cppc_bit) {
> +               return true;
> +       }
> +
>          switch (boot_cpu_data.x86_vendor) {
>          case X86_VENDOR_AMD:
>          case X86_VENDOR_HYGON:
Rafael J. Wysocki June 19, 2024, 5:09 p.m. UTC | #15 
On Wed, Jun 19, 2024 at 6:33 AM Aaron Rainbolt <arainbolt@kfocus.org> wrote:
>
> acpi: Allow ignoring _OSC CPPC v2 bit via kernel parameter
>
> The _OSC is supposed to contain a bit indicating whether the hardware
> supports CPPC v2 or not. This bit is not always set, causing CPPC v2 to
> be considered absent. This results in severe single-core performance
> issues with the EEVDF scheduler on heterogenous-core Intel processors.

While some things can be affected by this, I don't immediately see a
connection between CPPC v2, Intel hybrid processors and EEVDF.

In particular, why would EEVDF alone be affected?

Care to explain this?

> To work around this, provide a new kernel parameter, "ignore_osc_cppc_bit",
> which may be used to ignore the _OSC CPPC v2 bit and act as if the bit was
> enabled. This allows CPPC to be properly detected even if not "enabled" by
> _OSC, allowing users with problematic hardware to obtain decent single-core
> performance.
>
> Tested-by: Michael Mikowski <mmikowski@kfocus.org>
> Signed-off-by: Aaron Rainbolt <arainbolt@kfocus.org>
>
> ---
>
> V2 -> V3: Move bit ignore to before switch.
> V1 -> V2: Rewrite to work in cpc_supported_by_cpu.
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index b600df82669d..af2d8973ba3a 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -2063,6 +2063,12 @@
>                         could change it dynamically, usually by
>                         /sys/module/printk/parameters/ignore_loglevel.
>
> +       ignore_osc_cppc_bit
> +                       Assume CPPC is present and ignore the CPPC v2 bit from
> +                       the ACPI _OSC method. This is useful for working
> +                       around buggy firmware where CPPC is supported, but
> +                       _OSC incorrectly reports it as being absent.
> +
>         ignore_rlimit_data
>                         Ignore RLIMIT_DATA setting for data mappings,
>                         print warning at first misuse.  Can be changed via
> diff --git a/arch/x86/kernel/acpi/cppc.c b/arch/x86/kernel/acpi/cppc.c
> index ff8f25faca3d..0ca1eac826af 100644
> --- a/arch/x86/kernel/acpi/cppc.c
> +++ b/arch/x86/kernel/acpi/cppc.c
> @@ -11,8 +11,20 @@
>
>  /* Refer to drivers/acpi/cppc_acpi.c for the description of functions */
>
> +static bool ignore_osc_cppc_bit;
> +static int __init parse_ignore_osc_cppc_bit(char *arg)
> +{
> +       ignore_osc_cppc_bit = true;
> +       return 0;
> +}
> +early_param("ignore_osc_cppc_bit", parse_ignore_osc_cppc_bit);
> +
>  bool cpc_supported_by_cpu(void)
>  {
> +       if (ignore_osc_cppc_bit) {
> +               return true;
> +       }
> +
>         switch (boot_cpu_data.x86_vendor) {
>         case X86_VENDOR_AMD:
>         case X86_VENDOR_HYGON:
>
Rafael J. Wysocki June 19, 2024, 5:30 p.m. UTC | #16 
On Wednesday, June 19, 2024 7:09:35 PM CEST Rafael J. Wysocki wrote:
> On Wed, Jun 19, 2024 at 6:33 AM Aaron Rainbolt <arainbolt@kfocus.org> wrote:
> >
> > acpi: Allow ignoring _OSC CPPC v2 bit via kernel parameter
> >
> > The _OSC is supposed to contain a bit indicating whether the hardware
> > supports CPPC v2 or not. This bit is not always set, causing CPPC v2 to
> > be considered absent. This results in severe single-core performance
> > issues with the EEVDF scheduler on heterogenous-core Intel processors.
> 
> While some things can be affected by this, I don't immediately see a
> connection between CPPC v2, Intel hybrid processors and EEVDF.
> 
> In particular, why would EEVDF alone be affected?
> 
> Care to explain this?

And the reason why I am asking is because I think that you really need
something like this (untested beyond compilation):

---
 drivers/cpufreq/intel_pstate.c |   16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

Index: linux-pm/drivers/cpufreq/intel_pstate.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/intel_pstate.c
+++ linux-pm/drivers/cpufreq/intel_pstate.c
@@ -355,16 +355,16 @@ static void intel_pstate_set_itmt_prio(i
 	int ret;
 
 	ret = cppc_get_perf_caps(cpu, &cppc_perf);
-	if (ret)
-		return;
-
 	/*
-	 * On some systems with overclocking enabled, CPPC.highest_perf is hardcoded to 0xff.
-	 * In this case we can't use CPPC.highest_perf to enable ITMT.
-	 * In this case we can look at MSR_HWP_CAPABILITIES bits [8:0] to decide.
+	 * If CPPC is not available, fall back to MSR_HWP_CAPABILITIES bits [8:0].
+	 *
+	 * Also, on some systems with overclocking enabled, CPPC.highest_perf is
+	 * hardcoded to 0xff, so CPPC.highest_perf cannot be used to enable ITMT.
+	 * Fall back to MSR_HWP_CAPABILITIES then too.
 	 */
-	if (cppc_perf.highest_perf == CPPC_MAX_PERF)
-		cppc_perf.highest_perf = HWP_HIGHEST_PERF(READ_ONCE(all_cpu_data[cpu]->hwp_cap_cached));
+	if (ret || cppc_perf.highest_perf == CPPC_MAX_PERF)
+		cppc_perf.highest_perf =
+			HWP_HIGHEST_PERF(READ_ONCE(all_cpu_data[cpu]->hwp_cap_cached));
 
 	/*
 	 * The priorities can be set regardless of whether or not
Aaron Rainbolt June 19, 2024, 5:34 p.m. UTC | #17 
On Wed, Jun 19, 2024 at 07:09:35PM +0200, Rafael J. Wysocki wrote:
> On Wed, Jun 19, 2024 at 6:33 AM Aaron Rainbolt <arainbolt@kfocus.org> wrote:
> >
> > acpi: Allow ignoring _OSC CPPC v2 bit via kernel parameter
> >
> > The _OSC is supposed to contain a bit indicating whether the hardware
> > supports CPPC v2 or not. This bit is not always set, causing CPPC v2 to
> > be considered absent. This results in severe single-core performance
> > issues with the EEVDF scheduler on heterogenous-core Intel processors.
> 
> While some things can be affected by this, I don't immediately see a
> connection between CPPC v2, Intel hybrid processors and EEVDF.
> 
> In particular, why would EEVDF alone be affected?
> 
> Care to explain this?

From what I understand, the EEVDF scheduler requires ITMT (which in turn
requires CPPC v2) in order to determine which cores are performance cores
and which cores are efficiency cores. When CPPC v2 is missing, ITMT is
also missing, and the scheduler no longer can figure out which cores are
which. Thus on a system with many efficiency cores and a few performance
cores, there's a pretty decent chance the scheduler will put an intensive
single-threaded load on an efficiency core rather than on a performance
core, which has obvious performance implications since efficiency cores
are slower than performance cores by design.

A good example of someone else hitting this issue can be seen here:
https://bugzilla.kernel.org/show_bug.cgi?id=218195 This issue was brought
onto the LKML here:
https://lore.kernel.org/lkml/a106fb4733d0a3f0d6d5792705cdb5cee13731f8.camel@linux.intel.com/T/
Srinivas would have more info here, but I do not have his email so I can't
CC him on this.
 
> > To work around this, provide a new kernel parameter, "ignore_osc_cppc_bit",
> > which may be used to ignore the _OSC CPPC v2 bit and act as if the bit was
> > enabled. This allows CPPC to be properly detected even if not "enabled" by
> > _OSC, allowing users with problematic hardware to obtain decent single-core
> > performance.
Rafael J. Wysocki June 19, 2024, 5:37 p.m. UTC | #18 
On Wed, Jun 19, 2024 at 7:34 PM Aaron Rainbolt <arainbolt@kfocus.org> wrote:
>
> On Wed, Jun 19, 2024 at 07:09:35PM +0200, Rafael J. Wysocki wrote:
> > On Wed, Jun 19, 2024 at 6:33 AM Aaron Rainbolt <arainbolt@kfocus.org> wrote:
> > >
> > > acpi: Allow ignoring _OSC CPPC v2 bit via kernel parameter
> > >
> > > The _OSC is supposed to contain a bit indicating whether the hardware
> > > supports CPPC v2 or not. This bit is not always set, causing CPPC v2 to
> > > be considered absent. This results in severe single-core performance
> > > issues with the EEVDF scheduler on heterogenous-core Intel processors.
> >
> > While some things can be affected by this, I don't immediately see a
> > connection between CPPC v2, Intel hybrid processors and EEVDF.
> >
> > In particular, why would EEVDF alone be affected?
> >
> > Care to explain this?
>
> From what I understand, the EEVDF scheduler requires ITMT (which in turn
> requires CPPC v2) in order to determine which cores are performance cores
> and which cores are efficiency cores. When CPPC v2 is missing, ITMT is
> also missing, and the scheduler no longer can figure out which cores are
> which. Thus on a system with many efficiency cores and a few performance
> cores, there's a pretty decent chance the scheduler will put an intensive
> single-threaded load on an efficiency core rather than on a performance
> core, which has obvious performance implications since efficiency cores
> are slower than performance cores by design.

So the above information should go into the changelog of your patch.

> A good example of someone else hitting this issue can be seen here:
> https://bugzilla.kernel.org/show_bug.cgi?id=218195 This issue was brought
> onto the LKML here:
> https://lore.kernel.org/lkml/a106fb4733d0a3f0d6d5792705cdb5cee13731f8.camel@linux.intel.com/T/
> Srinivas would have more info here, but I do not have his email so I can't
> CC him on this.

OK, CCed.

Now, please check the patch I've just posted in this thread:

https://lore.kernel.org/linux-acpi/12457165.O9o76ZdvQC@rjwysocki.net/
Aaron Rainbolt June 19, 2024, 5:44 p.m. UTC | #19 
On Wed, Jun 19, 2024 at 07:30:55PM +0200, Rafael J. Wysocki wrote:
> On Wednesday, June 19, 2024 7:09:35 PM CEST Rafael J. Wysocki wrote:
> > On Wed, Jun 19, 2024 at 6:33 AM Aaron Rainbolt <arainbolt@kfocus.org> wrote:
> > >
> > > acpi: Allow ignoring _OSC CPPC v2 bit via kernel parameter
> > >
> > > The _OSC is supposed to contain a bit indicating whether the hardware
> > > supports CPPC v2 or not. This bit is not always set, causing CPPC v2 to
> > > be considered absent. This results in severe single-core performance
> > > issues with the EEVDF scheduler on heterogenous-core Intel processors.
> > 
> > While some things can be affected by this, I don't immediately see a
> > connection between CPPC v2, Intel hybrid processors and EEVDF.
> > 
> > In particular, why would EEVDF alone be affected?
> > 
> > Care to explain this?
> 
> And the reason why I am asking is because I think that you really need
> something like this (untested beyond compilation):
> 
> ---
>  drivers/cpufreq/intel_pstate.c |   16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> Index: linux-pm/drivers/cpufreq/intel_pstate.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/intel_pstate.c
> +++ linux-pm/drivers/cpufreq/intel_pstate.c
> @@ -355,16 +355,16 @@ static void intel_pstate_set_itmt_prio(i
>  	int ret;
>  
>  	ret = cppc_get_perf_caps(cpu, &cppc_perf);
> -	if (ret)
> -		return;
> -
>  	/*
> -	 * On some systems with overclocking enabled, CPPC.highest_perf is hardcoded to 0xff.
> -	 * In this case we can't use CPPC.highest_perf to enable ITMT.
> -	 * In this case we can look at MSR_HWP_CAPABILITIES bits [8:0] to decide.
> +	 * If CPPC is not available, fall back to MSR_HWP_CAPABILITIES bits [8:0].
> +	 *
> +	 * Also, on some systems with overclocking enabled, CPPC.highest_perf is
> +	 * hardcoded to 0xff, so CPPC.highest_perf cannot be used to enable ITMT.
> +	 * Fall back to MSR_HWP_CAPABILITIES then too.
>  	 */
> -	if (cppc_perf.highest_perf == CPPC_MAX_PERF)
> -		cppc_perf.highest_perf = HWP_HIGHEST_PERF(READ_ONCE(all_cpu_data[cpu]->hwp_cap_cached));
> +	if (ret || cppc_perf.highest_perf == CPPC_MAX_PERF)
> +		cppc_perf.highest_perf =
> +			HWP_HIGHEST_PERF(READ_ONCE(all_cpu_data[cpu]->hwp_cap_cached));
>  
>  	/*
>  	 * The priorities can be set regardless of whether or not
> 
> 
> 

That could work, maybe? It modifies the code path that is being hit, but
our systems are not overclocked, and when the patch suggested earlier is
in place things "just work" when the right parameter is set, so I don't
*think* cppc_perf.highest_perf will be equal to CPPC_MAX_PERF. (I know
from https://github.com/StarLabsLtd/firmware/issues/143 that 'ret' will be
0, since cppc_get_perf_caps will error out with "ACPI CPPC: No CPC
descriptor for CPU:X", so that code path cannot be triggered in our case
unless cppc_perf.highest_pert == CPPC_MAX_PERF.)
Aaron Rainbolt June 19, 2024, 5:56 p.m. UTC | #20 
On Wed, Jun 19, 2024 at 07:30:55PM +0200, Rafael J. Wysocki wrote:
> On Wednesday, June 19, 2024 7:09:35 PM CEST Rafael J. Wysocki wrote:
> > On Wed, Jun 19, 2024 at 6:33 AM Aaron Rainbolt <arainbolt@kfocus.org> wrote:
> > >
> > > acpi: Allow ignoring _OSC CPPC v2 bit via kernel parameter
> > >
> > > The _OSC is supposed to contain a bit indicating whether the hardware
> > > supports CPPC v2 or not. This bit is not always set, causing CPPC v2 to
> > > be considered absent. This results in severe single-core performance
> > > issues with the EEVDF scheduler on heterogenous-core Intel processors.
> > 
> > While some things can be affected by this, I don't immediately see a
> > connection between CPPC v2, Intel hybrid processors and EEVDF.
> > 
> > In particular, why would EEVDF alone be affected?
> > 
> > Care to explain this?
> 
> And the reason why I am asking is because I think that you really need
> something like this (untested beyond compilation):
> 
> ---
>  drivers/cpufreq/intel_pstate.c |   16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> Index: linux-pm/drivers/cpufreq/intel_pstate.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/intel_pstate.c
> +++ linux-pm/drivers/cpufreq/intel_pstate.c
> @@ -355,16 +355,16 @@ static void intel_pstate_set_itmt_prio(i
>  	int ret;
>  
>  	ret = cppc_get_perf_caps(cpu, &cppc_perf);
> -	if (ret)
> -		return;
> -
>  	/*
> -	 * On some systems with overclocking enabled, CPPC.highest_perf is hardcoded to 0xff.
> -	 * In this case we can't use CPPC.highest_perf to enable ITMT.
> -	 * In this case we can look at MSR_HWP_CAPABILITIES bits [8:0] to decide.
> +	 * If CPPC is not available, fall back to MSR_HWP_CAPABILITIES bits [8:0].
> +	 *
> +	 * Also, on some systems with overclocking enabled, CPPC.highest_perf is
> +	 * hardcoded to 0xff, so CPPC.highest_perf cannot be used to enable ITMT.
> +	 * Fall back to MSR_HWP_CAPABILITIES then too.
>  	 */
> -	if (cppc_perf.highest_perf == CPPC_MAX_PERF)
> -		cppc_perf.highest_perf = HWP_HIGHEST_PERF(READ_ONCE(all_cpu_data[cpu]->hwp_cap_cached));
> +	if (ret || cppc_perf.highest_perf == CPPC_MAX_PERF)
> +		cppc_perf.highest_perf =
> +			HWP_HIGHEST_PERF(READ_ONCE(all_cpu_data[cpu]->hwp_cap_cached));
>  
>  	/*
>  	 * The priorities can be set regardless of whether or not
> 
> 
> 

Gah. I can't read apparently. That patch may very well work because I
just realized the "if (ret) return;" means to return if ret is NOT 0. I
had it confused with "return if ret is 0".

That patch looks like it may very well work, and better than what I had
because it doesn't require manually setting a kernel parameter. I'll apply
it and test it. (That may take me a bit, I don't have access to the
hardware with the problem, only my boss does, but I should be able to get
it done before the end of today.)
Aaron Rainbolt June 19, 2024, 9:39 p.m. UTC | #21 
On Wed, Jun 19, 2024 at 07:30:55PM +0200, Rafael J. Wysocki wrote:
> On Wednesday, June 19, 2024 7:09:35 PM CEST Rafael J. Wysocki wrote:
> > On Wed, Jun 19, 2024 at 6:33 AM Aaron Rainbolt <arainbolt@kfocus.org> wrote:
> > >
> > > acpi: Allow ignoring _OSC CPPC v2 bit via kernel parameter
> > >
> > > The _OSC is supposed to contain a bit indicating whether the hardware
> > > supports CPPC v2 or not. This bit is not always set, causing CPPC v2 to
> > > be considered absent. This results in severe single-core performance
> > > issues with the EEVDF scheduler on heterogenous-core Intel processors.
> > 
> > While some things can be affected by this, I don't immediately see a
> > connection between CPPC v2, Intel hybrid processors and EEVDF.
> > 
> > In particular, why would EEVDF alone be affected?
> > 
> > Care to explain this?
> 
> And the reason why I am asking is because I think that you really need
> something like this (untested beyond compilation):

Alright, our team has compiled and tested the patch.

Results were mixed - with my patch, both CPPC and ITMT were enabled. This
patch appears to enable only ITMT. (To be specific, running
"find /proc /sys | grep '(cppc|itmt)'" shows only ITMT enabled under
/proc, and no CPPC directories under /sys.) This causes a performance hit
in benchmarking - using my patch with 'ignore_osc_cppc_bit', we were
getting Geekbench 5 scores of at least 1907 single-core, and 10500
multi-core. With this patch, we only are getting approximately 1700
single-core, and less than 9000 multi-core. (With an entirely unpatched
kernel, we were getting less than 1000 single-core, and about 10000
multi-core.)

Ultimately this is an upgrade over unpatched performance, but a
downgrade from the previous patch. It seems having CPPC and ITMT
available at the same time made things work noticeably faster.

Is there some way that can get both CPPC and ITMT to work with an approach
like this? Our hardware does support both, it just has an incorrectly set
bit in _OSC.

> ---
>  drivers/cpufreq/intel_pstate.c |   16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> Index: linux-pm/drivers/cpufreq/intel_pstate.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/intel_pstate.c
> +++ linux-pm/drivers/cpufreq/intel_pstate.c
> @@ -355,16 +355,16 @@ static void intel_pstate_set_itmt_prio(i
>  	int ret;
>  
>  	ret = cppc_get_perf_caps(cpu, &cppc_perf);
> -	if (ret)
> -		return;
> -
>  	/*
> -	 * On some systems with overclocking enabled, CPPC.highest_perf is hardcoded to 0xff.
> -	 * In this case we can't use CPPC.highest_perf to enable ITMT.
> -	 * In this case we can look at MSR_HWP_CAPABILITIES bits [8:0] to decide.
> +	 * If CPPC is not available, fall back to MSR_HWP_CAPABILITIES bits [8:0].
> +	 *
> +	 * Also, on some systems with overclocking enabled, CPPC.highest_perf is
> +	 * hardcoded to 0xff, so CPPC.highest_perf cannot be used to enable ITMT.
> +	 * Fall back to MSR_HWP_CAPABILITIES then too.
>  	 */
> -	if (cppc_perf.highest_perf == CPPC_MAX_PERF)
> -		cppc_perf.highest_perf = HWP_HIGHEST_PERF(READ_ONCE(all_cpu_data[cpu]->hwp_cap_cached));
> +	if (ret || cppc_perf.highest_perf == CPPC_MAX_PERF)
> +		cppc_perf.highest_perf =
> +			HWP_HIGHEST_PERF(READ_ONCE(all_cpu_data[cpu]->hwp_cap_cached));
>  
>  	/*
>  	 * The priorities can be set regardless of whether or not
> 
> 
>
Aaron Rainbolt June 19, 2024, 10:19 p.m. UTC | #22 
On Wed, Jun 19, 2024 at 04:39:39PM -0500, Aaron Rainbolt wrote:
> On Wed, Jun 19, 2024 at 07:30:55PM +0200, Rafael J. Wysocki wrote:
> > On Wednesday, June 19, 2024 7:09:35 PM CEST Rafael J. Wysocki wrote:
> > > On Wed, Jun 19, 2024 at 6:33 AM Aaron Rainbolt <arainbolt@kfocus.org> wrote:
> > > >
> > > > acpi: Allow ignoring _OSC CPPC v2 bit via kernel parameter
> > > >
> > > > The _OSC is supposed to contain a bit indicating whether the hardware
> > > > supports CPPC v2 or not. This bit is not always set, causing CPPC v2 to
> > > > be considered absent. This results in severe single-core performance
> > > > issues with the EEVDF scheduler on heterogenous-core Intel processors.
> > > 
> > > While some things can be affected by this, I don't immediately see a
> > > connection between CPPC v2, Intel hybrid processors and EEVDF.
> > > 
> > > In particular, why would EEVDF alone be affected?
> > > 
> > > Care to explain this?
> > 
> > And the reason why I am asking is because I think that you really need
> > something like this (untested beyond compilation):
> 
> Alright, our team has compiled and tested the patch.
> 
> Results were mixed - with my patch, both CPPC and ITMT were enabled. This
> patch appears to enable only ITMT. (To be specific, running
> "find /proc /sys | grep '(cppc|itmt)'" shows only ITMT enabled under
> /proc, and no CPPC directories under /sys.) This causes a performance hit
> in benchmarking - using my patch with 'ignore_osc_cppc_bit', we were
> getting Geekbench 5 scores of at least 1907 single-core, and 10500
> multi-core. With this patch, we only are getting approximately 1700
> single-core, and less than 9000 multi-core. (With an entirely unpatched
> kernel, we were getting less than 1000 single-core, and about 10000
> multi-core.)
> 
> Ultimately this is an upgrade over unpatched performance, but a
> downgrade from the previous patch. It seems having CPPC and ITMT
> available at the same time made things work noticeably faster.
> 
> Is there some way that can get both CPPC and ITMT to work with an approach
> like this? Our hardware does support both, it just has an incorrectly set
> bit in _OSC.

I was premature in sending this - we reverted back to the previous patch
and are now getting similar scores to the new patch, meaning the benchmark
results may have had nothing to do with the new patch. We'll test further
and report back once we have conclusive results.
 
> > ---
> >  drivers/cpufreq/intel_pstate.c |   16 ++++++++--------
> >  1 file changed, 8 insertions(+), 8 deletions(-)
> > 
> > Index: linux-pm/drivers/cpufreq/intel_pstate.c
> > ===================================================================
> > --- linux-pm.orig/drivers/cpufreq/intel_pstate.c
> > +++ linux-pm/drivers/cpufreq/intel_pstate.c
> > @@ -355,16 +355,16 @@ static void intel_pstate_set_itmt_prio(i
> >  	int ret;
> >  
> >  	ret = cppc_get_perf_caps(cpu, &cppc_perf);
> > -	if (ret)
> > -		return;
> > -
> >  	/*
> > -	 * On some systems with overclocking enabled, CPPC.highest_perf is hardcoded to 0xff.
> > -	 * In this case we can't use CPPC.highest_perf to enable ITMT.
> > -	 * In this case we can look at MSR_HWP_CAPABILITIES bits [8:0] to decide.
> > +	 * If CPPC is not available, fall back to MSR_HWP_CAPABILITIES bits [8:0].
> > +	 *
> > +	 * Also, on some systems with overclocking enabled, CPPC.highest_perf is
> > +	 * hardcoded to 0xff, so CPPC.highest_perf cannot be used to enable ITMT.
> > +	 * Fall back to MSR_HWP_CAPABILITIES then too.
> >  	 */
> > -	if (cppc_perf.highest_perf == CPPC_MAX_PERF)
> > -		cppc_perf.highest_perf = HWP_HIGHEST_PERF(READ_ONCE(all_cpu_data[cpu]->hwp_cap_cached));
> > +	if (ret || cppc_perf.highest_perf == CPPC_MAX_PERF)
> > +		cppc_perf.highest_perf =
> > +			HWP_HIGHEST_PERF(READ_ONCE(all_cpu_data[cpu]->hwp_cap_cached));
> >  
> >  	/*
> >  	 * The priorities can be set regardless of whether or not
> > 
> > 
> >
Aaron Rainbolt June 20, 2024, 1:05 a.m. UTC | #23 
OK, we have done thorough benchmarking of the two patches. In summary,
they both seem to provide exactly the same performance improvements.
My initial worry that Rafael's patch didn't deliver the same performance
improvements was unfounded.

The following are the single-core and multi-core scores from running
Geekbench 5 multiple times on a Carbon Systems Iridium 16 system. The
first batch of tests was done with an Ubuntu kernel built with with my V3
proposed patch, while the second batch was done with a kernel build with
Rafael's proposed patch.

Links to the Geekbench 5 reports can be shared if needed.

_OSC CPPC bit ignore patch (written by Aaron Rainbolt):
Kernel parameter 'ignore_osc_cppc_bit' set in
'/etc/default/grub.d/kfocus.cfg'.
'/sys/devices/system/cpu/cpu*/acpi_cppc' and
'/proc/sys/kernel/sched_itmt_enabled' both present

| Run | Single | Multi  |
| --- | ------ | ------ |
|  01 |   1874 |  10475 |
|  02 |   1831 |  10132 |
|  03 |   1858 |  10348 |
|  04 |   1848 |  10370 |
|  05 |   1831 |  10413 |
| --- | ------ | ------ |
| AVG |   1848 |  10348 |


intel_pstate CPPC override patch (written by Rafael Wysocki):
No special kernel parameters set.
'/sys/devices/system/cpu/cpu*/acpi_cppc' ABSENT,
'/proc/sys/kernel/sched_itmt_enabled' present

| Run | Single | Multi  |
| --- | ------ | ------ |
|  01 |   1820 |  10310 |
|  02 |   1870 |  10303 |
|  03 |   1867 |  10420 |
|  04 |   1844 |  10283 |
|  05 |   1835 |  10451 |
| --- | ------ | ------ |
| AVG |   1847 |  10353 |
Rafael J. Wysocki June 20, 2024, 3:40 p.m. UTC | #24 
On Thu, Jun 20, 2024 at 3:05 AM Aaron Rainbolt <arainbolt@kfocus.org> wrote:
>
> OK, we have done thorough benchmarking of the two patches. In summary,
> they both seem to provide exactly the same performance improvements.
> My initial worry that Rafael's patch didn't deliver the same performance
> improvements was unfounded.

Good to know, thanks!

> The following are the single-core and multi-core scores from running
> Geekbench 5 multiple times on a Carbon Systems Iridium 16 system. The
> first batch of tests was done with an Ubuntu kernel built with with my V3
> proposed patch, while the second batch was done with a kernel build with
> Rafael's proposed patch.
>
> Links to the Geekbench 5 reports can be shared if needed.
>
> _OSC CPPC bit ignore patch (written by Aaron Rainbolt):
> Kernel parameter 'ignore_osc_cppc_bit' set in
> '/etc/default/grub.d/kfocus.cfg'.
> '/sys/devices/system/cpu/cpu*/acpi_cppc' and
> '/proc/sys/kernel/sched_itmt_enabled' both present
>
> | Run | Single | Multi  |
> | --- | ------ | ------ |
> |  01 |   1874 |  10475 |
> |  02 |   1831 |  10132 |
> |  03 |   1858 |  10348 |
> |  04 |   1848 |  10370 |
> |  05 |   1831 |  10413 |
> | --- | ------ | ------ |
> | AVG |   1848 |  10348 |
>
>
> intel_pstate CPPC override patch (written by Rafael Wysocki):
> No special kernel parameters set.
> '/sys/devices/system/cpu/cpu*/acpi_cppc' ABSENT,
> '/proc/sys/kernel/sched_itmt_enabled' present
>
> | Run | Single | Multi  |
> | --- | ------ | ------ |
> |  01 |   1820 |  10310 |
> |  02 |   1870 |  10303 |
> |  03 |   1867 |  10420 |
> |  04 |   1844 |  10283 |
> |  05 |   1835 |  10451 |
> | --- | ------ | ------ |
> | AVG |   1847 |  10353 |

The problem with ignoring what the platform firmware is telling (or
not telling) the OS through ACPI is that only it knows the reason why
it is doing that.

It may be by mistake, but it also may be on purpose and it is hard to say.

However, intel_pstate already knows that HWP is enabled on the
processor, so it can be used directly regardless of whether or not
CPPC is enabled.  That is more appropriate and does not require users
to modify their kernel command line.

I'll add a changelog to the patch and submit it properly.

Thanks!
diff mbox series

Patch

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index 1d857978f5f4..53406dd6cb87 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -671,7 +671,7 @@  static inline void arch_init_invariance_cppc(void) { }
  *
  *	Return: 0 for success or negative value for err.
  */
-int acpi_cppc_processor_probe(struct acpi_processor *pr)
+int acpi_cppc_processor_probe(struct acpi_processor *pr, bool ignore_osc_cppc_bit)
 {
 	struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL};
 	union acpi_object *out_obj, *cpc_obj;
@@ -686,7 +686,7 @@  int acpi_cppc_processor_probe(struct acpi_processor *pr)
 
 	if (!osc_sb_cppc2_support_acked) {
 		pr_debug("CPPC v2 _OSC not acked\n");
-		if (!cpc_supported_by_cpu()) {
+		if (!ignore_osc_cppc_bit && !cpc_supported_by_cpu()) {
 			pr_debug("CPPC is not supported by the CPU\n");
 			return -ENODEV;
 		}
diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
index 67db60eda370..a183bca6c1c5 100644
--- a/drivers/acpi/processor_driver.c
+++ b/drivers/acpi/processor_driver.c
@@ -33,6 +33,11 @@  MODULE_AUTHOR("Paul Diefenbaugh");
 MODULE_DESCRIPTION("ACPI Processor Driver");
 MODULE_LICENSE("GPL");
 
+static bool ignore_osc_cppc_bit = false;
+module_param(ignore_osc_cppc_bit, bool, 0);
+MODULE_PARM_DESC(ignore_osc_cppc_bit,
+	"Ignore _OSC CPPC bit, assume CPPC v2 is present");
+
 static int acpi_processor_start(struct device *dev);
 static int acpi_processor_stop(struct device *dev);
 
@@ -170,7 +175,7 @@  static int __acpi_processor_start(struct acpi_device *device)
 	if (pr->flags.need_hotplug_init)
 		return 0;
 
-	result = acpi_cppc_processor_probe(pr);
+	result = acpi_cppc_processor_probe(pr, ignore_osc_cppc_bit);
 	if (result && !IS_ENABLED(CONFIG_ACPI_CPU_FREQ_PSS))
 		dev_dbg(&device->dev, "CPPC data invalid or not present\n");
 
diff --git a/include/acpi/processor.h b/include/acpi/processor.h
index 3f34ebb27525..79fd61b3f537 100644
--- a/include/acpi/processor.h
+++ b/include/acpi/processor.h
@@ -358,10 +358,10 @@  int acpi_map_cpuid(phys_cpuid_t phys_id, u32 acpi_id);
 int acpi_get_cpuid(acpi_handle, int type, u32 acpi_id);
 
 #ifdef CONFIG_ACPI_CPPC_LIB
-extern int acpi_cppc_processor_probe(struct acpi_processor *pr);
+extern int acpi_cppc_processor_probe(struct acpi_processor *pr, bool ignore_osc_cppc_bit);
 extern void acpi_cppc_processor_exit(struct acpi_processor *pr);
 #else
-static inline int acpi_cppc_processor_probe(struct acpi_processor *pr)
+static inline int acpi_cppc_processor_probe(struct acpi_processor *pr, bool ignore_osc_cppc_bit)
 {
 	return 0;
 }