diff mbox series

[V4] acpi/prmt: find block with specific type

Message ID 20240911045214.930141-1-kobak@nvidia.com
State Superseded
Headers show
Series [V4] acpi/prmt: find block with specific type | expand

Commit Message

KobaK Sept. 11, 2024, 4:52 a.m. UTC 
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:
      __efi_queue_work+0x11c/0x170
      efi_call_acpi_prm_handler+0x68/0xd0
      acpi_platformrt_space_handler+0x198/0x258
      acpi_ev_address_space_dispatch+0x144/0x388
      acpi_ex_access_region+0x9c/0x118
      acpi_ex_write_serial_bus+0xc4/0x218
      acpi_ex_write_data_to_field+0x168/0x218
      acpi_ex_store_object_to_node+0x1a8/0x258
      acpi_ex_store+0xec/0x330
      acpi_ex_opcode_1A_1T_1R+0x15c/0x618
      acpi_ds_exec_end_op+0x274/0x548
      acpi_ps_parse_loop+0x10c/0x6b8
      acpi_ps_parse_aml+0x140/0x3b0
      acpi_ps_execute_method+0x12c/0x2a0
      acpi_ns_evaluate+0x210/0x310
      acpi_evaluate_object+0x178/0x358
      acpi_proc_write+0x1a8/0x8a0 [acpi_call]
      proc_reg_write+0xcc/0x150
      vfs_write+0xd8/0x380
      ksys_write+0x70/0x120
      __arm64_sys_write+0x24/0x48
      invoke_syscall.constprop.0+0x80/0xf8
      do_el0_svc+0x50/0x110
      el0_svc+0x48/0x1d0
      el0t_64_sync_handler+0x15c/0x178
      el0t_64_sync+0x1a8/0x1b0

Find a block with specific type to fix this.
prmt find a block with EFI_RUNTIME_SERVICES_DATA for prm handler and
find a block with EFI_RUNTIME_SERVICES_CODE for prm context.
If no suitable block is found, a warning message will be prompted
but the procedue continues to manage the next prm handler.
However, if the prm handler is actullay 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.

[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: KobaK <kobak@nvidia.com>
Reviewed-by: Matthew R. Ochs <mochs@nvidia.com>
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202409071245.bkGWWtfQ-lkp@intel.com/
---
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 
---
 drivers/acpi/prmt.c | 49 +++++++++++++++++++++++++++++++--------------
 1 file changed, 34 insertions(+), 15 deletions(-)

Comments

Zhang, Rui Sept. 11, 2024, 6:07 a.m. UTC | #1 
On Wed, 2024-09-11 at 12:52 +0800, KobaK wrote:
> 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:
>       __efi_queue_work+0x11c/0x170
>       efi_call_acpi_prm_handler+0x68/0xd0
>       acpi_platformrt_space_handler+0x198/0x258
>       acpi_ev_address_space_dispatch+0x144/0x388
>       acpi_ex_access_region+0x9c/0x118
>       acpi_ex_write_serial_bus+0xc4/0x218
>       acpi_ex_write_data_to_field+0x168/0x218
>       acpi_ex_store_object_to_node+0x1a8/0x258
>       acpi_ex_store+0xec/0x330
>       acpi_ex_opcode_1A_1T_1R+0x15c/0x618
>       acpi_ds_exec_end_op+0x274/0x548
>       acpi_ps_parse_loop+0x10c/0x6b8
>       acpi_ps_parse_aml+0x140/0x3b0
>       acpi_ps_execute_method+0x12c/0x2a0
>       acpi_ns_evaluate+0x210/0x310
>       acpi_evaluate_object+0x178/0x358
>       acpi_proc_write+0x1a8/0x8a0 [acpi_call]
>       proc_reg_write+0xcc/0x150
>       vfs_write+0xd8/0x380
>       ksys_write+0x70/0x120
>       __arm64_sys_write+0x24/0x48
>       invoke_syscall.constprop.0+0x80/0xf8
>       do_el0_svc+0x50/0x110
>       el0_svc+0x48/0x1d0
>       el0t_64_sync_handler+0x15c/0x178
>       el0t_64_sync+0x1a8/0x1b0
> 
> Find a block with specific type to fix this.
> prmt find a block with EFI_RUNTIME_SERVICES_DATA for prm handler and
> find a block with EFI_RUNTIME_SERVICES_CODE for prm context.
> If no suitable block is found, a warning message will be prompted
> but the procedue continues to manage the next prm handler.

s/procedue/procedure ?

> However, if the prm handler is actullay 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.
> 
> [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: KobaK <kobak@nvidia.com>
> Reviewed-by: Matthew R. Ochs <mochs@nvidia.com>
> 

> Reported-by: kernel test robot <lkp@intel.com>
> Closes:
> https://lore.kernel.org/oe-kbuild-all/202409071245.bkGWWtfQ-lkp@intel.com/

I don't think you need these two lines. This applies when you address
the bug in a separate patch.

> ---
> 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 
> ---
>  drivers/acpi/prmt.c | 49 +++++++++++++++++++++++++++++++------------
> --
>  1 file changed, 34 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/acpi/prmt.c b/drivers/acpi/prmt.c
> index c78453c74ef5..029e8f9db53a 100644
> --- a/drivers/acpi/prmt.c
> +++ b/drivers/acpi/prmt.c
> @@ -72,15 +72,17 @@ 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(u64 pa, u32 type)
>  {
>         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->type == type) &&
> +                       (md->phys_addr < pa && pa < md->phys_addr +
> PAGE_SIZE * md->num_pages)) {
>                         return pa_offset + md->virt_addr + page - md-
> >phys_addr;
> +               }
>         }
>  
>         return 0;
> @@ -148,9 +150,18 @@ 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(handler_info-
> >handler_address, EFI_RUNTIME_SERVICES_CODE);
> +               th->static_data_buffer_addr =
> +                       efi_pa_va_lookup(handler_info-
> >static_data_buffer_address, EFI_RUNTIME_SERVICES_DATA);
> +               th->acpi_param_buffer_addr =
> +                       efi_pa_va_lookup(handler_info-
> >acpi_param_buffer_address, EFI_RUNTIME_SERVICES_DATA);
> +
> +               if (!th->handler_addr || !th->static_data_buffer_addr
> || !th->acpi_param_buffer_addr)
> +                       pr_warn(
> +                               "Idx: %llu, Parts of handler(GUID:
> %pUB) are missed, handler_addr %llx, data_addr %llx, param_addr
> %llx",
> +                               cur_handler, th->handler_addr,
> +                               th->static_data_buffer_addr, th-
> >acpi_param_buffer_addr);

have you run build test for this patch?

drivers/acpi/prmt.c: In function ‘acpi_parse_prmt’:
./include/linux/kern_levels.h:5:25: warning: format ‘%llx’ expects a
matching ‘long long unsigned int’ argument [-Wformat=]
    5 | #define KERN_SOH        "\001"          /* ASCII Start Of
Header */
      |                         ^~~~~~
./include/linux/printk.h:437:25: note: in definition of macro
‘printk_index_wrap’
  437 |                 _p_func(_fmt, ##__VA_ARGS__);                 
\
      |                         ^~~~
./include/linux/printk.h:518:9: note: in expansion of macro ‘printk’
  518 |         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:518:16: note: in expansion of macro
‘KERN_WARNING’
  518 |         printk(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
      |                ^~~~~~~~~~~~
drivers/acpi/prmt.c:161:25: note: in expansion of macro ‘pr_warn’
  161 |                         pr_warn(
      |                         ^~~~~~~


-rui

>         } while (++cur_handler < tm->handler_count && (handler_info =
> get_next_handler(handler_info)));
>  
>         return 0;
> @@ -250,8 +261,16 @@ static acpi_status
> acpi_platformrt_space_handler(u32 function,
>  
>                 handler = find_prm_handler(&buffer->handler_guid);
>                 module = find_prm_module(&buffer->handler_guid);
> -               if (!handler || !module)
> -                       goto invalid_guid;
> +               if (!handler || !module) {
> +                       buffer->prm_status =
> PRM_HANDLER_GUID_NOT_FOUND;
> +                       return AE_OK;
> +               }
> +
> +               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;
> @@ -274,8 +293,10 @@ static acpi_status
> acpi_platformrt_space_handler(u32 function,
>         case PRM_CMD_START_TRANSACTION:
>  
>                 module = find_prm_module(&buffer->handler_guid);
> -               if (!module)
> -                       goto invalid_guid;
> +               if (!module) {
> +                       buffer->prm_status =
> PRM_HANDLER_GUID_NOT_FOUND;
> +                       return AE_OK;
> +               }
>  
>                 if (module->updatable)
>                         module->updatable = false;
> @@ -286,8 +307,10 @@ static acpi_status
> acpi_platformrt_space_handler(u32 function,
>         case PRM_CMD_END_TRANSACTION:
>  
>                 module = find_prm_module(&buffer->handler_guid);
> -               if (!module)
> -                       goto invalid_guid;
> +               if (!module) {
> +                       buffer->prm_status =
> PRM_HANDLER_GUID_NOT_FOUND;
> +                       return AE_OK;
> +               }
>  
>                 if (module->updatable)
>                         buffer->prm_status =
> UPDATE_UNLOCK_WITHOUT_LOCK;
> @@ -302,10 +325,6 @@ static acpi_status
> acpi_platformrt_space_handler(u32 function,
>         }
>  
>         return AE_OK;
> -
> -invalid_guid:
> -       buffer->prm_status = PRM_HANDLER_GUID_NOT_FOUND;
> -       return AE_OK;
>  }
>  
>  void __init init_prmt(void)
diff mbox series

Patch

diff --git a/drivers/acpi/prmt.c b/drivers/acpi/prmt.c
index c78453c74ef5..029e8f9db53a 100644
--- a/drivers/acpi/prmt.c
+++ b/drivers/acpi/prmt.c
@@ -72,15 +72,17 @@  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(u64 pa, u32 type)
 {
 	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->type == type) &&
+			(md->phys_addr < pa && pa < md->phys_addr + PAGE_SIZE * md->num_pages)) {
 			return pa_offset + md->virt_addr + page - md->phys_addr;
+		}
 	}
 
 	return 0;
@@ -148,9 +150,18 @@  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(handler_info->handler_address, EFI_RUNTIME_SERVICES_CODE);
+		th->static_data_buffer_addr =
+			efi_pa_va_lookup(handler_info->static_data_buffer_address, EFI_RUNTIME_SERVICES_DATA);
+		th->acpi_param_buffer_addr =
+			efi_pa_va_lookup(handler_info->acpi_param_buffer_address, EFI_RUNTIME_SERVICES_DATA);
+
+		if (!th->handler_addr || !th->static_data_buffer_addr || !th->acpi_param_buffer_addr)
+			pr_warn(
+				"Idx: %llu, Parts of handler(GUID: %pUB) are missed, handler_addr %llx, data_addr %llx, param_addr %llx",
+				cur_handler, th->handler_addr,
+				th->static_data_buffer_addr, th->acpi_param_buffer_addr);
 	} while (++cur_handler < tm->handler_count && (handler_info = get_next_handler(handler_info)));
 
 	return 0;
@@ -250,8 +261,16 @@  static acpi_status acpi_platformrt_space_handler(u32 function,
 
 		handler = find_prm_handler(&buffer->handler_guid);
 		module = find_prm_module(&buffer->handler_guid);
-		if (!handler || !module)
-			goto invalid_guid;
+		if (!handler || !module) {
+			buffer->prm_status = PRM_HANDLER_GUID_NOT_FOUND;
+			return AE_OK;
+		}
+
+		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;
@@ -274,8 +293,10 @@  static acpi_status acpi_platformrt_space_handler(u32 function,
 	case PRM_CMD_START_TRANSACTION:
 
 		module = find_prm_module(&buffer->handler_guid);
-		if (!module)
-			goto invalid_guid;
+		if (!module) {
+			buffer->prm_status = PRM_HANDLER_GUID_NOT_FOUND;
+			return AE_OK;
+		}
 
 		if (module->updatable)
 			module->updatable = false;
@@ -286,8 +307,10 @@  static acpi_status acpi_platformrt_space_handler(u32 function,
 	case PRM_CMD_END_TRANSACTION:
 
 		module = find_prm_module(&buffer->handler_guid);
-		if (!module)
-			goto invalid_guid;
+		if (!module) {
+			buffer->prm_status = PRM_HANDLER_GUID_NOT_FOUND;
+			return AE_OK;
+		}
 
 		if (module->updatable)
 			buffer->prm_status = UPDATE_UNLOCK_WITHOUT_LOCK;
@@ -302,10 +325,6 @@  static acpi_status acpi_platformrt_space_handler(u32 function,
 	}
 
 	return AE_OK;
-
-invalid_guid:
-	buffer->prm_status = PRM_HANDLER_GUID_NOT_FOUND;
-	return AE_OK;
 }
 
 void __init init_prmt(void)