| Message ID | 20241009064517.2678456-1-kobak@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [V7] acpi/prmt: find block with specific type | expand |
On Wed, 9 Oct 2024 at 08:45, KobaK <kobak@nvidia.com> wrote: > > From: kobak <kobak@nvidia.com> > > PRMT needs to find the correct type of block to > translate the PA-VA mapping for EFI runtime services. > > The issue arises because the PRMT is finding a block of > type EFI_CONVENTIONAL_MEMORY, which is not appropriate for > runtime services as described in Section 2.2.2 (Runtime > Services) of the UEFI Specification [1]. Since the PRM handler is > a type of runtime service, this causes an exception > when the PRM handler is called. > > [Firmware Bug]: Unable to handle paging request in EFI runtime service > WARNING: CPU: 22 PID: 4330 at drivers/firmware/efi/runtime-wrappers.c:341 > __efi_queue_work+0x11c/0x170 > Call trace: > > Find a block with specific type to fix this. > PRMT find a block with EFI_MEMORY_RUNTIME for PRM handler and PRM context. > If no suitable block is found, a warning message will be prompted > but the procedure continues to manage the next PRM handler. > However, if the PRM handler is actually called without proper allocation, > it would result in a failure during error handling. > > By using the correct memory types for runtime services, > ensure that the PRM handler and the context are > properly mapped in the virtual address space during runtime, > preventing the paging request error. > > The issue is really that only memory that has been remapped for > runtime by the firmware can be used by the PRM handler, and so the > region needs to have the EFI_MEMORY_RUNTIME attribute. > > [1] https://uefi.org/sites/default/files/resources/UEFI_Spec_2_10_Aug29.pdf > Fixes: cefc7ca46235 ("ACPI: PRM: implement OperationRegion handler for the PlatformRtMechanism subtype") > Signed-off-by: Koba Ko <kobak@nvidia.com> > Reviewed-by: Matthew R. Ochs <mochs@nvidia.com> > Reviewed-by: Zhang Rui <rui.zhang@intel.com> Reviewed-by: Ard Biesheuvel <ardb@kernel.org> This needs a cc:stable too > --- > V2: > 1. format the changelog and add more about error handling. > 2. replace goto > V3: Warn if parts of handler are missed during va-pa translating. > V4: Fix the 0day > V5: Fix typo and pr_warn warning > V6: use EFI_MOMOERY_RUNTIME to find block and split goto refactor as a single > patch > V7: > 1. refine the codes and commit description as per comments > 2. drop goto refacotr > --- > > drivers/acpi/prmt.c | 27 ++++++++++++++++++++++----- > 1 file changed, 22 insertions(+), 5 deletions(-) > > diff --git a/drivers/acpi/prmt.c b/drivers/acpi/prmt.c > index 1cfaa5957ac4..51f5ae3d4350 100644 > --- a/drivers/acpi/prmt.c > +++ b/drivers/acpi/prmt.c > @@ -72,17 +72,21 @@ struct prm_module_info { > struct prm_handler_info handlers[] __counted_by(handler_count); > }; > > -static u64 efi_pa_va_lookup(u64 pa) > +static u64 efi_pa_va_lookup(efi_guid_t *guid, u64 pa) > { > efi_memory_desc_t *md; > u64 pa_offset = pa & ~PAGE_MASK; > u64 page = pa & PAGE_MASK; > > for_each_efi_memory_desc(md) { > - if (md->phys_addr < pa && pa < md->phys_addr + PAGE_SIZE * md->num_pages) > + if ((md->attribute & EFI_MEMORY_RUNTIME) && > + (md->phys_addr < pa && pa < md->phys_addr + PAGE_SIZE * md->num_pages)) { > return pa_offset + md->virt_addr + page - md->phys_addr; > + } > } > > + pr_warn("Failed to find VA for GUID: %pUL, PA: %p", guid, pa); > + > return 0; > } > > @@ -148,9 +152,15 @@ acpi_parse_prmt(union acpi_subtable_headers *header, const unsigned long end) > th = &tm->handlers[cur_handler]; > > guid_copy(&th->guid, (guid_t *)handler_info->handler_guid); > - th->handler_addr = (void *)efi_pa_va_lookup(handler_info->handler_address); > - th->static_data_buffer_addr = efi_pa_va_lookup(handler_info->static_data_buffer_address); > - th->acpi_param_buffer_addr = efi_pa_va_lookup(handler_info->acpi_param_buffer_address); > + th->handler_addr = > + (void *)efi_pa_va_lookup(&th->guid, handler_info->handler_address); > + > + th->static_data_buffer_addr = > + efi_pa_va_lookup(&th->guid, handler_info->static_data_buffer_address); > + > + th->acpi_param_buffer_addr = > + efi_pa_va_lookup(&th->guid, handler_info->acpi_param_buffer_address); > + > } while (++cur_handler < tm->handler_count && (handler_info = get_next_handler(handler_info))); > > return 0; > @@ -277,6 +287,13 @@ static acpi_status acpi_platformrt_space_handler(u32 function, > if (!handler || !module) > goto invalid_guid; > > + if (!handler->handler_addr || > + !handler->static_data_buffer_addr || > + !handler->acpi_param_buffer_addr) { > + buffer->prm_status = PRM_HANDLER_ERROR; > + return AE_OK; > + } > + > ACPI_COPY_NAMESEG(context.signature, "PRMC"); > context.revision = 0x0; > context.reserved = 0x0; > -- > 2.43.0 >
On 10/9/24 14:50, Ard Biesheuvel wrote: > External email: Use caution opening links or attachments > > > On Wed, 9 Oct 2024 at 08:45, KobaK <kobak@nvidia.com> wrote: >> From: kobak <kobak@nvidia.com> >> >> PRMT needs to find the correct type of block to >> translate the PA-VA mapping for EFI runtime services. >> >> The issue arises because the PRMT is finding a block of >> type EFI_CONVENTIONAL_MEMORY, which is not appropriate for >> runtime services as described in Section 2.2.2 (Runtime >> Services) of the UEFI Specification [1]. Since the PRM handler is >> a type of runtime service, this causes an exception >> when the PRM handler is called. >> >> [Firmware Bug]: Unable to handle paging request in EFI runtime service >> WARNING: CPU: 22 PID: 4330 at drivers/firmware/efi/runtime-wrappers.c:341 >> __efi_queue_work+0x11c/0x170 >> Call trace: >> >> Find a block with specific type to fix this. >> PRMT find a block with EFI_MEMORY_RUNTIME for PRM handler and PRM context. >> If no suitable block is found, a warning message will be prompted >> but the procedure continues to manage the next PRM handler. >> However, if the PRM handler is actually called without proper allocation, >> it would result in a failure during error handling. >> >> By using the correct memory types for runtime services, >> ensure that the PRM handler and the context are >> properly mapped in the virtual address space during runtime, >> preventing the paging request error. >> >> The issue is really that only memory that has been remapped for >> runtime by the firmware can be used by the PRM handler, and so the >> region needs to have the EFI_MEMORY_RUNTIME attribute. >> >> [1] https://uefi.org/sites/default/files/resources/UEFI_Spec_2_10_Aug29.pdf >> Fixes: cefc7ca46235 ("ACPI: PRM: implement OperationRegion handler for the PlatformRtMechanism subtype") >> Signed-off-by: Koba Ko <kobak@nvidia.com> >> Reviewed-by: Matthew R. Ochs <mochs@nvidia.com> >> Reviewed-by: Zhang Rui <rui.zhang@intel.com> > Reviewed-by: Ard Biesheuvel <ardb@kernel.org> > > This needs a cc:stable too Hi Ard, Rui Thanks for reviewed. > >> --- >> V2: >> 1. format the changelog and add more about error handling. >> 2. replace goto >> V3: Warn if parts of handler are missed during va-pa translating. >> V4: Fix the 0day >> V5: Fix typo and pr_warn warning >> V6: use EFI_MOMOERY_RUNTIME to find block and split goto refactor as a single >> patch >> V7: >> 1. refine the codes and commit description as per comments >> 2. drop goto refacotr >> --- >> >> drivers/acpi/prmt.c | 27 ++++++++++++++++++++++----- >> 1 file changed, 22 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/acpi/prmt.c b/drivers/acpi/prmt.c >> index 1cfaa5957ac4..51f5ae3d4350 100644 >> --- a/drivers/acpi/prmt.c >> +++ b/drivers/acpi/prmt.c >> @@ -72,17 +72,21 @@ struct prm_module_info { >> struct prm_handler_info handlers[] __counted_by(handler_count); >> }; >> >> -static u64 efi_pa_va_lookup(u64 pa) >> +static u64 efi_pa_va_lookup(efi_guid_t *guid, u64 pa) >> { >> efi_memory_desc_t *md; >> u64 pa_offset = pa & ~PAGE_MASK; >> u64 page = pa & PAGE_MASK; >> >> for_each_efi_memory_desc(md) { >> - if (md->phys_addr < pa && pa < md->phys_addr + PAGE_SIZE * md->num_pages) >> + if ((md->attribute & EFI_MEMORY_RUNTIME) && >> + (md->phys_addr < pa && pa < md->phys_addr + PAGE_SIZE * md->num_pages)) { >> return pa_offset + md->virt_addr + page - md->phys_addr; >> + } >> } >> >> + pr_warn("Failed to find VA for GUID: %pUL, PA: %p", guid, pa); >> + >> return 0; >> } >> >> @@ -148,9 +152,15 @@ acpi_parse_prmt(union acpi_subtable_headers *header, const unsigned long end) >> th = &tm->handlers[cur_handler]; >> >> guid_copy(&th->guid, (guid_t *)handler_info->handler_guid); >> - th->handler_addr = (void *)efi_pa_va_lookup(handler_info->handler_address); >> - th->static_data_buffer_addr = efi_pa_va_lookup(handler_info->static_data_buffer_address); >> - th->acpi_param_buffer_addr = efi_pa_va_lookup(handler_info->acpi_param_buffer_address); >> + th->handler_addr = >> + (void *)efi_pa_va_lookup(&th->guid, handler_info->handler_address); >> + >> + th->static_data_buffer_addr = >> + efi_pa_va_lookup(&th->guid, handler_info->static_data_buffer_address); >> + >> + th->acpi_param_buffer_addr = >> + efi_pa_va_lookup(&th->guid, handler_info->acpi_param_buffer_address); >> + >> } while (++cur_handler < tm->handler_count && (handler_info = get_next_handler(handler_info))); >> >> return 0; >> @@ -277,6 +287,13 @@ static acpi_status acpi_platformrt_space_handler(u32 function, >> if (!handler || !module) >> goto invalid_guid; >> >> + if (!handler->handler_addr || >> + !handler->static_data_buffer_addr || >> + !handler->acpi_param_buffer_addr) { >> + buffer->prm_status = PRM_HANDLER_ERROR; >> + return AE_OK; >> + } >> + >> ACPI_COPY_NAMESEG(context.signature, "PRMC"); >> context.revision = 0x0; >> context.reserved = 0x0; >> -- >> 2.43.0 >>
Hi KobaK, 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.12-rc2 next-20241011] [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/KobaK/acpi-prmt-find-block-with-specific-type/20241009-144658 base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next patch link: https://lore.kernel.org/r/20241009064517.2678456-1-kobak%40nvidia.com patch subject: [PATCH V7] acpi/prmt: find block with specific type config: x86_64-kexec (https://download.01.org/0day-ci/archive/20241013/202410130117.PZ2JPuxo-lkp@intel.com/config) compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241013/202410130117.PZ2JPuxo-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/202410130117.PZ2JPuxo-lkp@intel.com/ All warnings (new ones prefixed by >>): >> drivers/acpi/prmt.c:88:60: warning: format specifies type 'void *' but the argument has type 'u64' (aka 'unsigned long long') [-Wformat] 88 | pr_warn("Failed to find VA for GUID: %pUL, PA: %p", guid, pa); | ~~ ^~ | %llu include/linux/printk.h:543:37: note: expanded from macro 'pr_warn' 543 | printk(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__) | ~~~ ^~~~~~~~~~~ include/linux/printk.h:490:60: note: expanded from macro 'printk' 490 | #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__) | ~~~ ^~~~~~~~~~~ include/linux/printk.h:462:19: note: expanded from macro 'printk_index_wrap' 462 | _p_func(_fmt, ##__VA_ARGS__); \ | ~~~~ ^~~~~~~~~~~ >> drivers/acpi/prmt.c:156:29: warning: passing 1-byte aligned argument to 4-byte aligned parameter 1 of 'efi_pa_va_lookup' may result in an unaligned pointer access [-Walign-mismatch] 156 | (void *)efi_pa_va_lookup(&th->guid, handler_info->handler_address); | ^ drivers/acpi/prmt.c:159:21: warning: passing 1-byte aligned argument to 4-byte aligned parameter 1 of 'efi_pa_va_lookup' may result in an unaligned pointer access [-Walign-mismatch] 159 | efi_pa_va_lookup(&th->guid, handler_info->static_data_buffer_address); | ^ drivers/acpi/prmt.c:162:21: warning: passing 1-byte aligned argument to 4-byte aligned parameter 1 of 'efi_pa_va_lookup' may result in an unaligned pointer access [-Walign-mismatch] 162 | efi_pa_va_lookup(&th->guid, handler_info->acpi_param_buffer_address); | ^ 4 warnings generated. vim +88 drivers/acpi/prmt.c 74 75 static u64 efi_pa_va_lookup(efi_guid_t *guid, u64 pa) 76 { 77 efi_memory_desc_t *md; 78 u64 pa_offset = pa & ~PAGE_MASK; 79 u64 page = pa & PAGE_MASK; 80 81 for_each_efi_memory_desc(md) { 82 if ((md->attribute & EFI_MEMORY_RUNTIME) && 83 (md->phys_addr < pa && pa < md->phys_addr + PAGE_SIZE * md->num_pages)) { 84 return pa_offset + md->virt_addr + page - md->phys_addr; 85 } 86 } 87 > 88 pr_warn("Failed to find VA for GUID: %pUL, PA: %p", guid, pa); 89 90 return 0; 91 } 92 93 #define get_first_handler(a) ((struct acpi_prmt_handler_info *) ((char *) (a) + a->handler_info_offset)) 94 #define get_next_handler(a) ((struct acpi_prmt_handler_info *) (sizeof(struct acpi_prmt_handler_info) + (char *) a)) 95 96 static int __init 97 acpi_parse_prmt(union acpi_subtable_headers *header, const unsigned long end) 98 { 99 struct acpi_prmt_module_info *module_info; 100 struct acpi_prmt_handler_info *handler_info; 101 struct prm_handler_info *th; 102 struct prm_module_info *tm; 103 u64 *mmio_count; 104 u64 cur_handler = 0; 105 u32 module_info_size = 0; 106 u64 mmio_range_size = 0; 107 void *temp_mmio; 108 109 module_info = (struct acpi_prmt_module_info *) header; 110 module_info_size = struct_size(tm, handlers, module_info->handler_info_count); 111 tm = kmalloc(module_info_size, GFP_KERNEL); 112 if (!tm) 113 goto parse_prmt_out1; 114 115 guid_copy(&tm->guid, (guid_t *) module_info->module_guid); 116 tm->major_rev = module_info->major_rev; 117 tm->minor_rev = module_info->minor_rev; 118 tm->handler_count = module_info->handler_info_count; 119 tm->updatable = true; 120 121 if (module_info->mmio_list_pointer) { 122 /* 123 * Each module is associated with a list of addr 124 * ranges that it can use during the service 125 */ 126 mmio_count = (u64 *) memremap(module_info->mmio_list_pointer, 8, MEMREMAP_WB); 127 if (!mmio_count) 128 goto parse_prmt_out2; 129 130 mmio_range_size = struct_size(tm->mmio_info, addr_ranges, *mmio_count); 131 tm->mmio_info = kmalloc(mmio_range_size, GFP_KERNEL); 132 if (!tm->mmio_info) 133 goto parse_prmt_out3; 134 135 temp_mmio = memremap(module_info->mmio_list_pointer, mmio_range_size, MEMREMAP_WB); 136 if (!temp_mmio) 137 goto parse_prmt_out4; 138 memmove(tm->mmio_info, temp_mmio, mmio_range_size); 139 } else { 140 tm->mmio_info = kmalloc(sizeof(*tm->mmio_info), GFP_KERNEL); 141 if (!tm->mmio_info) 142 goto parse_prmt_out2; 143 144 tm->mmio_info->mmio_count = 0; 145 } 146 147 INIT_LIST_HEAD(&tm->module_list); 148 list_add(&tm->module_list, &prm_module_list); 149 150 handler_info = get_first_handler(module_info); 151 do { 152 th = &tm->handlers[cur_handler]; 153 154 guid_copy(&th->guid, (guid_t *)handler_info->handler_guid); 155 th->handler_addr = > 156 (void *)efi_pa_va_lookup(&th->guid, handler_info->handler_address); 157 158 th->static_data_buffer_addr = 159 efi_pa_va_lookup(&th->guid, handler_info->static_data_buffer_address); 160 161 th->acpi_param_buffer_addr = 162 efi_pa_va_lookup(&th->guid, handler_info->acpi_param_buffer_address); 163 164 } while (++cur_handler < tm->handler_count && (handler_info = get_next_handler(handler_info))); 165 166 return 0; 167 168 parse_prmt_out4: 169 kfree(tm->mmio_info); 170 parse_prmt_out3: 171 memunmap(mmio_count); 172 parse_prmt_out2: 173 kfree(tm); 174 parse_prmt_out1: 175 return -ENOMEM; 176 } 177
Hi KobaK,
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.12-rc2 next-20241011]
[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/KobaK/acpi-prmt-find-block-with-specific-type/20241009-144658
base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link: https://lore.kernel.org/r/20241009064517.2678456-1-kobak%40nvidia.com
patch subject: [PATCH V7] acpi/prmt: find block with specific type
config: x86_64-rhel-8.3 (https://download.01.org/0day-ci/archive/20241013/202410130153.COiJzh3R-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241013/202410130153.COiJzh3R-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/202410130153.COiJzh3R-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from include/linux/kernel.h:31,
from drivers/acpi/prmt.c:17:
drivers/acpi/prmt.c: In function 'efi_pa_va_lookup':
>> include/linux/kern_levels.h:5:25: warning: format '%p' expects argument of type 'void *', but argument 3 has type 'u64' {aka 'long long unsigned int'} [-Wformat=]
5 | #define KERN_SOH "\001" /* ASCII Start Of Header */
| ^~~~~~
include/linux/printk.h:462:25: note: in definition of macro 'printk_index_wrap'
462 | _p_func(_fmt, ##__VA_ARGS__); \
| ^~~~
include/linux/printk.h:543:9: note: in expansion of macro 'printk'
543 | printk(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~
include/linux/kern_levels.h:12:25: note: in expansion of macro 'KERN_SOH'
12 | #define KERN_WARNING KERN_SOH "4" /* warning conditions */
| ^~~~~~~~
include/linux/printk.h:543:16: note: in expansion of macro 'KERN_WARNING'
543 | printk(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~~~~~~
drivers/acpi/prmt.c:88:9: note: in expansion of macro 'pr_warn'
88 | pr_warn("Failed to find VA for GUID: %pUL, PA: %p", guid, pa);
| ^~~~~~~
vim +5 include/linux/kern_levels.h
314ba3520e513a Joe Perches 2012-07-30 4
04d2c8c83d0e3a Joe Perches 2012-07-30 @5 #define KERN_SOH "\001" /* ASCII Start Of Header */
04d2c8c83d0e3a Joe Perches 2012-07-30 6 #define KERN_SOH_ASCII '\001'
04d2c8c83d0e3a Joe Perches 2012-07-30 7
diff --git a/drivers/acpi/prmt.c b/drivers/acpi/prmt.c index 1cfaa5957ac4..51f5ae3d4350 100644 --- a/drivers/acpi/prmt.c +++ b/drivers/acpi/prmt.c @@ -72,17 +72,21 @@ struct prm_module_info { struct prm_handler_info handlers[] __counted_by(handler_count); }; -static u64 efi_pa_va_lookup(u64 pa) +static u64 efi_pa_va_lookup(efi_guid_t *guid, u64 pa) { efi_memory_desc_t *md; u64 pa_offset = pa & ~PAGE_MASK; u64 page = pa & PAGE_MASK; for_each_efi_memory_desc(md) { - if (md->phys_addr < pa && pa < md->phys_addr + PAGE_SIZE * md->num_pages) + if ((md->attribute & EFI_MEMORY_RUNTIME) && + (md->phys_addr < pa && pa < md->phys_addr + PAGE_SIZE * md->num_pages)) { return pa_offset + md->virt_addr + page - md->phys_addr; + } } + pr_warn("Failed to find VA for GUID: %pUL, PA: %p", guid, pa); + return 0; } @@ -148,9 +152,15 @@ acpi_parse_prmt(union acpi_subtable_headers *header, const unsigned long end) th = &tm->handlers[cur_handler]; guid_copy(&th->guid, (guid_t *)handler_info->handler_guid); - th->handler_addr = (void *)efi_pa_va_lookup(handler_info->handler_address); - th->static_data_buffer_addr = efi_pa_va_lookup(handler_info->static_data_buffer_address); - th->acpi_param_buffer_addr = efi_pa_va_lookup(handler_info->acpi_param_buffer_address); + th->handler_addr = + (void *)efi_pa_va_lookup(&th->guid, handler_info->handler_address); + + th->static_data_buffer_addr = + efi_pa_va_lookup(&th->guid, handler_info->static_data_buffer_address); + + th->acpi_param_buffer_addr = + efi_pa_va_lookup(&th->guid, handler_info->acpi_param_buffer_address); + } while (++cur_handler < tm->handler_count && (handler_info = get_next_handler(handler_info))); return 0; @@ -277,6 +287,13 @@ static acpi_status acpi_platformrt_space_handler(u32 function, if (!handler || !module) goto invalid_guid; + if (!handler->handler_addr || + !handler->static_data_buffer_addr || + !handler->acpi_param_buffer_addr) { + buffer->prm_status = PRM_HANDLER_ERROR; + return AE_OK; + } + ACPI_COPY_NAMESEG(context.signature, "PRMC"); context.revision = 0x0; context.reserved = 0x0;