| Message ID | 20231216001312.1160-1-jarredwhite@linux.microsoft.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [RESEND,v2] acpi: Use access_width over register_width for system memory accesses | expand |
On Sat, Dec 16, 2023 at 1:13 AM Jarred White <jarredwhite@linux.microsoft.com> wrote: > > 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. > 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. > > Signed-off-by: Jarred White <jarredwhite@linux.microsoft.com> > --- > Original thread > https://lore.kernel.org/all/20230925180552.76071-1-jarredwhite@linux.microsoft.com/ > --- > 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(-) > > diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c > index 7ff269a78c20..fb37e1727bf8 100644 > --- a/drivers/acpi/cppc_acpi.c > +++ b/drivers/acpi/cppc_acpi.c > @@ -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. > + > +/* 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. > + > 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; What would happen if the platform firmware incorrectly provided reg->access_witdh == 0? > + 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 = ®_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 = ®_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; > --
Hi Rafael, On 12/21/23 06:12, Rafael J. Wysocki wrote: > On Sat, Dec 16, 2023 at 1:13 AM Jarred White > <jarredwhite@linux.microsoft.com> wrote: >> >> 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. > >> 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. > >> >> Signed-off-by: Jarred White <jarredwhite@linux.microsoft.com> >> --- >> Original thread >> https://lore.kernel.org/all/20230925180552.76071-1-jarredwhite@linux.microsoft.com/ >> --- >> 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(-) >> >> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c >> index 7ff269a78c20..fb37e1727bf8 100644 >> --- a/drivers/acpi/cppc_acpi.c >> +++ b/drivers/acpi/cppc_acpi.c >> @@ -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. > >> + >> +/* 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. > >> + >> 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; > > 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
On 12/22/2023 4:00 PM, Easwar Hariharan wrote: > >> @@ -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? > > > 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
Hello, On 1/12/2024 6:19 AM, Jarred White wrote: > > 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
diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c index 7ff269a78c20..fb37e1727bf8 100644 --- a/drivers/acpi/cppc_acpi.c +++ b/drivers/acpi/cppc_acpi.c @@ -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 = ®_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 = ®_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;
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 <jarredwhite@linux.microsoft.com> --- Original thread https://lore.kernel.org/all/20230925180552.76071-1-jarredwhite@linux.microsoft.com/ --- 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(-)