acpi: Use access_width over register_width for system memory accesses

PATCH RESEND Superseded Jarred White 3 participants 5 messages Dec 16, 2023 – Feb 3, 2024

Subsystems (2)

ACPI ACPI CPPC

lore

Share & tools

lore

Jarred White

author

2023-12-16 00:13 UTC #0 lore raw

PATCH RESEND v2 1/1 acpi: Use access_width over register_width for system memory accesses

To align with ACPI 6.3+, since bit_width can be any 8-bit value, we cannot depend on it being always on a clean 8b boundary. Instead, use access_width to determine the size and use the offset and width to shift and mask the bits we want to read/write out. Make sure to add a check for system memory since pcc redefines the access_width to subspace id.

Signed-off-by Jarred White <[email protected]>

---
Original thread
https://lore.kernel.org/all/[email protected]/
---
changelog:
v1-->v2:
1. Fixed coding style errors
2. Backwards compatibility with ioremapping of address still an open question. Suggestions are welcomed.

 drivers/acpi/cppc_acpi.c | 36 +++++++++++++++++++++++++++++++-----
 1 file changed, 31 insertions(+), 5 deletions(-)

drivers/acpi/cppc_acpi.c +31 -5

index 7ff269a78c20..fb37e1727bf8 100644
@@ -163,6 +163,13 @@ show_cppc_data(cppc_get_perf_caps, cppc_perf_caps, nominal_freq);
 show_cppc_data(cppc_get_perf_ctrs, cppc_perf_fb_ctrs, reference_perf);
 show_cppc_data(cppc_get_perf_ctrs, cppc_perf_fb_ctrs, wraparound_time);
 
+/* Use access_width to determine the total number of bits */
+#define ACCESS_WIDTH_TO_BITS(reg) 8 << ((reg)->access_width - 1)
+
+/* Shift and apply the mask for CPC reads/writes */
+#define MASK_VAL(val) (((val) >> reg->bit_offset) & 			\
+					GENMASK((reg->bit_width), 0))
+
 static ssize_t show_feedback_ctrs(struct kobject *kobj,
 		struct kobj_attribute *attr, char *buf)
 {

@@ -777,6 +784,7 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
 			} else if (gas_t->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
 				if (gas_t->address) {
 					void __iomem *addr;
+					size_t access_width;
 
 					if (!osc_cpc_flexible_adr_space_confirmed) {
 						pr_debug("Flexible address space capability not supported\n");

@@ -784,7 +792,8 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
 							goto out_free;
 					}
 
-					addr = ioremap(gas_t->address, gas_t->bit_width/8);
+					access_width = ACCESS_WIDTH_TO_BITS(gas_t) / 8;
+					addr = ioremap(gas_t->address, access_width);
 					if (!addr)
 						goto out_free;
 					cpc_ptr->cpc_regs[i-2].sys_mem_vaddr = addr;

@@ -980,6 +989,7 @@ int __weak cpc_write_ffh(int cpunum, struct cpc_reg *reg, u64 val)
 static int cpc_read(int cpu, struct cpc_register_resource *reg_res, u64 *val)
 {
 	void __iomem *vaddr = NULL;
+	int size;
 	int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
 	struct cpc_reg *reg = &reg_res->cpc_entry.reg;

@@ -991,7 +1001,7 @@ static int cpc_read(int cpu, struct cpc_register_resource *reg_res, u64 *val)
 	*val = 0;
 
 	if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_IO) {
-		u32 width = 8 << (reg->access_width - 1);
+		u32 width = ACCESS_WIDTH_TO_BITS(reg);
 		u32 val_u32;
 		acpi_status status;

@@ -1015,7 +1025,12 @@ static int cpc_read(int cpu, struct cpc_register_resource *reg_res, u64 *val)
 		return acpi_os_read_memory((acpi_physical_address)reg->address,
 				val, reg->bit_width);
 
-	switch (reg->bit_width) {
+	if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY)
+		size = ACCESS_WIDTH_TO_BITS(reg);
+	else
+		size = reg->bit_width;
+
+	switch (size) {
 	case 8:
 		*val = readb_relaxed(vaddr);
 		break;

@@ -1034,18 +1049,22 @@ static int cpc_read(int cpu, struct cpc_register_resource *reg_res, u64 *val)
 		return -EFAULT;
 	}
 
+	if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY)
+		*val = MASK_VAL(*val);
+
 	return 0;
 }
 
 static int cpc_write(int cpu, struct cpc_register_resource *reg_res, u64 val)
 {
 	int ret_val = 0;
+	int size;
 	void __iomem *vaddr = NULL;
 	int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
 	struct cpc_reg *reg = &reg_res->cpc_entry.reg;
 
 	if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_IO) {
-		u32 width = 8 << (reg->access_width - 1);
+		u32 width = ACCESS_WIDTH_TO_BITS(reg);
 		acpi_status status;
 
 		status = acpi_os_write_port((acpi_io_address)reg->address,

@@ -1067,7 +1086,14 @@ static int cpc_write(int cpu, struct cpc_register_resource *reg_res, u64 val)
 		return acpi_os_write_memory((acpi_physical_address)reg->address,
 				val, reg->bit_width);
 
-	switch (reg->bit_width) {
+	if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
+		size = ACCESS_WIDTH_TO_BITS(reg);
+		val = MASK_VAL(val);
+	} else {
+		size = reg->bit_width;
+	}
+
+	switch (size) {
 	case 8:
 		writeb_relaxed(val, vaddr);
 		break;

Signature · 1 line

2.34.1

Rafael J. Wysocki

maintainer

2023-12-21 14:13 UTC #1 lore raw

Quoted 3 lines · Jarred White

To align with ACPI 6.3+, since bit_width can be any 8-bit value, we cannot depend on it being always on a clean 8b boundary.

So presumably, there are systems out there in which the platform firmware uses bit_width that is not on a clean 8-bit boundary.

It would be good to mention them here.

Quoted 4 lines

Instead, use access_width
to determine the size and use the offset and width to shift and mask the bits we want to read/write out. Make sure to add a check for system memory since pcc redefines the access_width to subspace id.

And it would be good to add a comment to the code for it too.

Quoted 20 lines

Signed-off-by Jarred White <[email protected]>
---
Original thread
https://lore.kernel.org/all/[email protected]/
---
changelog:
v1-->v2:
1. Fixed coding style errors
2. Backwards compatibility with ioremapping of address still an open question. Suggestions are welcomed.
 drivers/acpi/cppc_acpi.c | 36 +++++++++++++++++++++++++++++++-----
 1 file changed, 31 insertions(+), 5 deletions(-)
drivers/acpi/cppc_acpi.c +0 -0
index 7ff269a78c20..fb37e1727bf8 100644

drivers/acpi/cppc_acpi.c show_cppc_data(cppc_get_perf_caps, cppc_perf_caps, nominal_freq); · line 167

Quoted 5 lines

drivers/acpi/cppc_acpi.c +2 -0

@@ -163,6 +163,13 @@ show_cppc_data(cppc_get_perf_caps, cppc_perf_caps, nominal_freq);
 show_cppc_data(cppc_get_perf_ctrs, cppc_perf_fb_ctrs, reference_perf);
 show_cppc_data(cppc_get_perf_ctrs, cppc_perf_fb_ctrs, wraparound_time);
 
+/* Use access_width to determine the total number of bits */
+#define ACCESS_WIDTH_TO_BITS(reg) 8 << ((reg)->access_width - 1)

Consider adding parens around the entire expression, or you may get unexpected results.

drivers/acpi/cppc_acpi.c show_cppc_data(cppc_get_perf_caps, cppc_perf_caps, nominal_freq); · line 171

Quoted 4 lines

drivers/acpi/cppc_acpi.c +4 -0

+
+/* Shift and apply the mask for CPC reads/writes */
+#define MASK_VAL(val) (((val) >> reg->bit_offset) &                    \
+                                       GENMASK((reg->bit_width), 0))

'reg' should be an argument of the macro or it will be prone to subtle mistakes.

Quoted 13 lines

drivers/acpi/cppc_acpi.c +2 -0

+
 static ssize_t show_feedback_ctrs(struct kobject *kobj,
                struct kobj_attribute *attr, char *buf)
 {

@@ -777,6 +784,7 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
                        } else if (gas_t->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
                                if (gas_t->address) {
                                        void __iomem *addr;
+                                       size_t access_width;
 
                                        if (!osc_cpc_flexible_adr_space_confirmed) {
                                                pr_debug("Flexible address space capability not supported\n");

drivers/acpi/cppc_acpi.c int acpi_cppc_processor_probe(struct acpi_processor *pr) · line 795

Quoted 5 lines

drivers/acpi/cppc_acpi.c +1 -1

@@ -784,7 +792,8 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
                                                        goto out_free;
                                        }
 
-                                       addr = ioremap(gas_t->address, gas_t->bit_width/8);
+                                       access_width = ACCESS_WIDTH_TO_BITS(gas_t) / 8;

What would happen if the platform firmware incorrectly provided reg->access_witdh == 0?

Quoted 76 lines

drivers/acpi/cppc_acpi.c +22 -5

+                                       addr = ioremap(gas_t->address, access_width);
                                        if (!addr)
                                                goto out_free;
                                        cpc_ptr->cpc_regs[i-2].sys_mem_vaddr = addr;

@@ -980,6 +989,7 @@ int __weak cpc_write_ffh(int cpunum, struct cpc_reg *reg, u64 val)
 static int cpc_read(int cpu, struct cpc_register_resource *reg_res, u64 *val)
 {
        void __iomem *vaddr = NULL;
+       int size;
        int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
        struct cpc_reg *reg = &reg_res->cpc_entry.reg;

@@ -991,7 +1001,7 @@ static int cpc_read(int cpu, struct cpc_register_resource *reg_res, u64 *val)
        *val = 0;
 
        if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_IO) {
-               u32 width = 8 << (reg->access_width - 1);
+               u32 width = ACCESS_WIDTH_TO_BITS(reg);
                u32 val_u32;
                acpi_status status;

@@ -1015,7 +1025,12 @@ static int cpc_read(int cpu, struct cpc_register_resource *reg_res, u64 *val)
                return acpi_os_read_memory((acpi_physical_address)reg->address,
                                val, reg->bit_width);
 
-       switch (reg->bit_width) {
+       if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY)
+               size = ACCESS_WIDTH_TO_BITS(reg);
+       else
+               size = reg->bit_width;
+
+       switch (size) {
        case 8:
                *val = readb_relaxed(vaddr);
                break;

@@ -1034,18 +1049,22 @@ static int cpc_read(int cpu, struct cpc_register_resource *reg_res, u64 *val)
                return -EFAULT;
        }
 
+       if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY)
+               *val = MASK_VAL(*val);
+
        return 0;
 }
 
 static int cpc_write(int cpu, struct cpc_register_resource *reg_res, u64 val)
 {
        int ret_val = 0;
+       int size;
        void __iomem *vaddr = NULL;
        int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
        struct cpc_reg *reg = &reg_res->cpc_entry.reg;
 
        if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_IO) {
-               u32 width = 8 << (reg->access_width - 1);
+               u32 width = ACCESS_WIDTH_TO_BITS(reg);
                acpi_status status;
 
                status = acpi_os_write_port((acpi_io_address)reg->address,

@@ -1067,7 +1086,14 @@ static int cpc_write(int cpu, struct cpc_register_resource *reg_res, u64 val)
                return acpi_os_write_memory((acpi_physical_address)reg->address,
                                val, reg->bit_width);
 
-       switch (reg->bit_width) {
+       if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
+               size = ACCESS_WIDTH_TO_BITS(reg);
+               val = MASK_VAL(val);
+       } else {
+               size = reg->bit_width;
+       }
+
+       switch (size) {
        case 8:
                writeb_relaxed(val, vaddr);
                break;
--

Easwar Hariharan

2023-12-23 00:00 UTC #2 lore raw

Hi Rafael,

Quoted 10 lines · Rafael J. Wysocki

Quoted 3 lines · Jarred White
To align with ACPI 6.3+, since bit_width can be any 8-bit value, we cannot depend on it being always on a clean 8b boundary.
So presumably, there are systems out there in which the platform firmware uses bit_width that is not on a clean 8-bit boundary.
It would be good to mention them here.

The _CPC package resides in SystemMemory space for the Microsoft Azure Cobalt 100 platform that was recently announced.[1] Without this patch, the following kernel panic is seen:

Starting Securi[ 22.713561] SError Interrupt on CPU26, code 0xbe000011 -- SError

[   22.713565] CPU: 26 PID: 1510 Comm: systemd-udevd Not tainted 5.15.2.1-13.cm2 #1
[   22.713567] Hardware name: MICROSOFT CORPORATION C2141/C2141, BIOS v1.10.1.0000_dda4ae5a  TfaVersion: v1.10.1.0053_3508b407  HspVersion: v1.10.1.164_38ccd
[   22.713569] pstate: 62400009 (nZCv daif +PAN -UAO +TCO -DIT -SSBS BTYPE=--)
[   22.713571] pc : cppc_get_perf_caps+0xec/0x410
[   22.713578] lr : cppc_get_perf_caps+0xe8/0x410
[   22.713579] sp : ffff8000155ab730
[   22.713580] x29: ffff8000155ab730 x28: ffff0080139d0038 x27: ffff0080139d0078
[   22.713583] x26: 0000000000000000 x25: ffff0080139d0058 x24: 00000000ffffffff
[   22.713584] x23: ffff0080139d0298 x22: ffff0080139d0278 x21: 0000000000000000
[   22.713586] x20: ffff00802b251910 x19: ffff0080139d0000 x18: ffffffffffffffff
[   22.713587] x17: 0000000000000000 x16: ffffdc7e111bad04 x15: ffff00802b251008
[   22.713588] x14: ffffffffffffffff x13: ffff013f1fd63300 x12: 0000000000000006
[   22.713590] x11: ffffdc7e128f4420 x10: 0000000000000000 x9 : ffffdc7e111badec
[   22.713591] x8 : ffff00802b251980 x7 : 0000000000000000 x6 : ffff0080139d0028
[   22.713592] x5 : 0000000000000000 x4 : ffff0080139d0018 x3 : 00000000ffffffff
[   22.713594] x2 : 0000000000000008 x1 : ffff8000155ab7a0 x0 : 0000000000000000
[   22.713596] Kernel panic - not syncing: Asynchronous SError Interrupt
[   22.713597] CPU: 26 PID: 1510 Comm: systemd-udevd Not tainted 5.15.2.1-13.cm2 #1
[   22.713599] Hardware name: MICROSOFT CORPORATION C2141/C2141, BIOS v1.10.1.0000_dda4ae5a  TfaVersion: v1.10.1.0053_3508b407  HspVersion: v1.10.1.164_38ccd
[   22.713600] Call trace:
[   22.713600]  dump_backtrace+0x0/0x1e0
[   22.713603]  show_stack+0x24/0x30
[   22.713604]  dump_stack_lvl+0x8c/0xb8
[   22.713607]  dump_stack+0x18/0x34
[   22.713608]  panic+0x16c/0x384
[   22.713612]  add_taint+0x0/0xc0
[   22.713618]  arm64_serror_panic+0x7c/0x90
[   22.713621]  arm64_is_fatal_ras_serror+0x34/0xa4
[   22.713622]  do_serror+0x50/0x6c
[   22.713623]  el1h_64_error_handler+0x40/0x74
[   22.713628]  el1h_64_error+0x7c/0x80
[   22.713630]  cppc_get_perf_caps+0xec/0x410
[   22.713632]  cppc_cpufreq_cpu_init+0x74/0x400 [cppc_cpufreq]
[   22.713635]  cpufreq_online+0x2dc/0xa30
[   22.713640]  cpufreq_add_dev+0xc0/0xd4
[   22.713642]  subsys_interface_register+0x134/0x14c
[   22.713647]  cpufreq_register_driver+0x1b0/0x354
[   22.713649]  cppc_cpufreq_init+0x1a8/0x1000 [cppc_cpufreq]
[   22.713651]  do_one_initcall+0x50/0x250
[   22.713654]  do_init_module+0x60/0x27c
[   22.713659]  load_module+0x2300/0x2570
[   22.713661]  __do_sys_finit_module+0xa8/0x114
[   22.713663]  __arm64_sys_finit_module+0x2c/0x3c
[   22.713665]  invoke_syscall+0x78/0x100
[   22.713669]  el0_svc_common.constprop.0+0x180/0x1a0
[   22.713671]  do_el0_svc+0x84/0xa0
[   22.713672]  el0_svc+0x2c/0xc0
[   22.713674]  el0t_64_sync_handler+0xa4/0x12c
[   22.713675]  el0t_64_sync+0x1a4/0x1a8
[   22.713677] SMP: stopping secondary CPUs
[   22.714457] Kernel Offset: 0x5c7e008d0000 from 0xffff800010000000
[   22.714458] PHYS_OFFSET: 0x80000000
[   22.714459] CPU features: 0x000001c1,fb337fdb
[   22.714460] Memory Limit: none

I apologize that we couldn't mention this before as the platform was not known publicly then.

Quoted 7 lines

Quoted 5 lines
Instead, use access_width
to determine the size and use the offset and width to shift and mask the bits we want to read/write out. Make sure to add a check for system memory since pcc redefines the access_width to subspace id.
And it would be good to add a comment to the code for it too.

Will do. Jarred is currently on vacation for the end of the year, but will respond to this and the comments below once he's back in the new year.

Quoted 30 lines

Quoted 21 lines
Signed-off-by Jarred White <[email protected]>
---
Original thread
https://lore.kernel.org/all/[email protected]/
---
changelog:
v1-->v2:
1. Fixed coding style errors
2. Backwards compatibility with ioremapping of address still an open question. Suggestions are welcomed.
  drivers/acpi/cppc_acpi.c | 36 +++++++++++++++++++++++++++++++-----
  1 file changed, 31 insertions(+), 5 deletions(-)
drivers/acpi/cppc_acpi.c +0 -0
index 7ff269a78c20..fb37e1727bf8 100644
drivers/acpi/cppc_acpi.c show_cppc_data(cppc_get_perf_caps, cppc_perf_caps, nominal_freq); · line 167
Quoted 5 lines
drivers/acpi/cppc_acpi.c +2 -0
@@ -163,6 +163,13 @@ show_cppc_data(cppc_get_perf_caps, cppc_perf_caps, nominal_freq);
  show_cppc_data(cppc_get_perf_ctrs, cppc_perf_fb_ctrs, reference_perf);
  show_cppc_data(cppc_get_perf_ctrs, cppc_perf_fb_ctrs, wraparound_time);
 
+/* Use access_width to determine the total number of bits */
+#define ACCESS_WIDTH_TO_BITS(reg) 8 << ((reg)->access_width - 1)
Consider adding parens around the entire expression, or you may get unexpected results.

Quoted 3 lines

drivers/acpi/cppc_acpi.c show_cppc_data(cppc_get_perf_caps, cppc_perf_caps, nominal_freq);
Quoted 3 lines
drivers/acpi/cppc_acpi.c +3 -0
+
+/* Shift and apply the mask for CPC reads/writes */
+#define MASK_VAL(val) (((val) >> reg->bit_offset) & 

Quoted 20 lines

drivers/acpi/cppc_acpi.c show_cppc_data(cppc_get_perf_caps, cppc_perf_caps, nominal_freq); · line 171

Quoted 1 line

drivers/acpi/cppc_acpi.c +1 -0

+                                       GENMASK((reg->bit_width), 0))

'reg' should be an argument of the macro or it will be prone to subtle mistakes.

Quoted 16 lines

drivers/acpi/cppc_acpi.c +2 -0

+
  static ssize_t show_feedback_ctrs(struct kobject *kobj,
                 struct kobj_attribute *attr, char *buf)
  {

@@ -777,6 +784,7 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
                         } else if (gas_t->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
                                 if (gas_t->address) {
                                         void __iomem *addr;
+                                       size_t access_width;
 
                                         if (!osc_cpc_flexible_adr_space_confirmed) {
                                                 pr_debug("Flexible address space capability not supported\n");

@@ -784,7 +792,8 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
                                                         goto out_free;
                                         }

Quoted 6 lines

drivers/acpi/cppc_acpi.c int acpi_cppc_processor_probe(struct acpi_processor *pr) · line 795
Quoted 2 lines
drivers/acpi/cppc_acpi.c +1 -1
-                                       addr = ioremap(gas_t->address, gas_t->bit_width/8);
+                                       access_width = ACCESS_WIDTH_TO_BITS(gas_t) / 8;
What would happen if the platform firmware incorrectly provided reg->access_witdh == 0?

<snip>

[1] https://azure.microsoft.com/en-us/blog/microsoft-azure-delivers-purpose-built-cloud-infrastructure-in-the-era-of-ai

Signature · 2 lines

Thanks,
Easwar

Jarred White

author

2024-01-12 14:19 UTC #3 lore raw

Quoted 14 lines · Easwar Hariharan

Quoted 1 line
Review int acpi_cppc_processor_probe(struct
Quoted 1 line
(quoted diff) +0 -0
@@ -784,7 +792,8 @@ int acpi_cppc_processor_probe(struct 
acpi_processor *pr)
Quoted 4 lines
drivers/acpi/cppc_acpi.c int acpi_cppc_processor_probe(struct acpi_processor *pr)
Quoted 4 lines
drivers/acpi/cppc_acpi.c +0 -1
                                                           goto out_free;
                                           }
 
-                                       addr = 
ioremap(gas_t->address, gas_t->bit_width/8);
Quoted 1 line
Review int acpi_cppc_processor_probe(struct · line 795
Quoted 1 line
(quoted diff) +1 -0
+                                       access_width = 
ACCESS_WIDTH_TO_BITS(gas_t) / 8;
Quoted 4 lines
What would happen if the platform firmware incorrectly provided reg->access_witdh == 0?

Unfortunately, we have no hardware platform with this configuration to test with. What is the level of concern here? Were there previous changes that raised this similar concerns?

Thanks,
Jarred

Jarred White

author

2024-02-03 00:44 UTC #4 lore raw

Hello,

Quoted 7 lines · Jarred White

Unfortunately, we have no hardware platform with this configuration to test with. What is the level of concern here? Were there previous changes that raised this similar concerns?
Thanks,
Jarred

I just wanted to follow-up on my previous questions.

Thanks,
Jarred