| Message ID | 20240911155536.3900579-1-kobak@nvidia.com |
|---|---|
| State | New |
| Headers | show |
| Series | [V5] acpi/prmt: find block with specific type | expand |
On Wed, Sep 11, 2024 at 5:55 PM KobaK <kobak@nvidia.com> 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. > 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 I need input from EFI people on this, so can you please resend the patch with a CC to linux-efi@vger.kernel.org? > 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> > --- > 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 > --- > 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..cd4a7f5491d6 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: %pUL) are missed, handler_addr %p, data_addr %p, param_addr %p", > + cur_handler, &th->guid, th->handler_addr, > + (void *)th->static_data_buffer_addr, (void *)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) > -- > 2.43.0 > >
On Wed, 2 Oct 2024 at 20:06, Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Wed, Sep 11, 2024 at 5:55 PM KobaK <kobak@nvidia.com> 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. > > 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 > > I need input from EFI people on this, so can you please resend the > patch with a CC to linux-efi@vger.kernel.org? > > > Fixes: cefc7ca46235 ("ACPI: PRM: implement OperationRegion handler for the PlatformRtMechanism subtype") > > Signed-off-by: KobaK <kobak@nvidia.com> Please use your full name. > > Reviewed-by: Matthew R. Ochs <mochs@nvidia.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 > > V5: Fix typo and pr_warn warning > > --- > > 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..cd4a7f5491d6 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); Wouldn't it make more sense to test the EFI_MEMORY_RUNTIME attribute rather than expecting/assuming a certain memory type in each case? That attribute is precisely what controls whether or not a region has been remapped into the firmware's page tables. > > + 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: %pUL) are missed, handler_addr %p, data_addr %p, param_addr %p", Please improve this diagnostic: 'are missed' is not very helpful. > > + cur_handler, &th->guid, th->handler_addr, > > + (void *)th->static_data_buffer_addr, (void *)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; > > + } What is the reason for this change, and the ones down below? > > > > 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) > > -- > > 2.43.0 > > > > >
On 10/3/24 05:11, Ard Biesheuvel wrote: > External email: Use caution opening links or attachments > > > On Wed, 2 Oct 2024 at 20:06, Rafael J. Wysocki <rafael@kernel.org> wrote: >> On Wed, Sep 11, 2024 at 5:55 PM KobaK <kobak@nvidia.com> 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. >>> 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 >> I need input from EFI people on this, so can you please resend the >> patch with a CC to linux-efi@vger.kernel.org? >> >>> Fixes: cefc7ca46235 ("ACPI: PRM: implement OperationRegion handler for the PlatformRtMechanism subtype") >>> Signed-off-by: KobaK <kobak@nvidia.com> > Please use your full name. Hi Ardb, Sure, will update. > >>> Reviewed-by: Matthew R. Ochs <mochs@nvidia.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 >>> V5: Fix typo and pr_warn warning >>> --- >>> 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..cd4a7f5491d6 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); > Wouldn't it make more sense to test the EFI_MEMORY_RUNTIME attribute > rather than expecting/assuming a certain memory type in each case? > That attribute is precisely what controls whether or not a region has > been remapped into the firmware's page tables. Please see the below > >>> + 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: %pUL) are missed, handler_addr %p, data_addr %p, param_addr %p", > Please improve this diagnostic: 'are missed' is not very helpful. Are these good for you ``` -static u64 efi_pa_va_lookup(u64 pa, u32 type) +static u64 efi_pa_va_lookup(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->type == type) && + 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; } @@ -150,18 +150,20 @@ 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, 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: %pUL) are missed, handler_addr %p, data_addr %p, param_addr %p", - cur_handler, &th->guid, th->handler_addr, - (void *)th->static_data_buffer_addr, (void *)th->acpi_param_buffer_addr); + th->handler_addr = (void *)efi_pa_va_lookup(handler_info->handler_address); + if (!th->handler_addr) + pr_warn( "Idx: %llu, failed to find VA for handler_addr(GUID: %pUL, PA: %p)", + cur_handler, &th->guid, th->handler_addr); + + th->static_data_buffer_addr = efi_pa_va_lookup(handler_info->static_data_buffer_address); + if (!th->static_data_buffer_addr) + pr_warn( "Idx: %llu, failed to find VA for data_addr(PA: %p)", + cur_handler, &th->guid, (void *)th->static_data_buffer_addr); + + th->acpi_param_buffer_addr = efi_pa_va_lookup(handler_info->acpi_param_buffer_address); + if (!th->acpi_param_buffer_addr) + pr_warn( "Idx: %llu, failed to find VA for param_addr(PA: %p)", + cur_handler, &th->guid, (void *)th->acpi_param_buffer_addr); ``` > > >>> + cur_handler, &th->guid, th->handler_addr, >>> + (void *)th->static_data_buffer_addr, (void *)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; >>> + } > What is the reason for this change, and the ones down below? As per Rui's comment, goto can be replaced with return. So I modified them with return and PRM_HANDLER_GUID_NOT_FOUND. > >>> 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) >>> -- >>> 2.43.0 >>> >>>
On 10/3/24 02:05, Rafael J. Wysocki wrote: > External email: Use caution opening links or attachments > > > On Wed, Sep 11, 2024 at 5:55 PM KobaK <kobak@nvidia.com> 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. >> 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 > I need input from EFI people on this, so can you please resend the > patch with a CC to linux-efi@vger.kernel.org? Ok, i will CC in next push >> 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> >> --- >> 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 >> --- >> 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..cd4a7f5491d6 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: %pUL) are missed, handler_addr %p, data_addr %p, param_addr %p", >> + cur_handler, &th->guid, th->handler_addr, >> + (void *)th->static_data_buffer_addr, (void *)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) >> -- >> 2.43.0 >> >>
On Thu, 3 Oct 2024 at 05:45, Koba Ko <kobak@nvidia.com> wrote: > > > On 10/3/24 05:11, Ard Biesheuvel wrote: > > External email: Use caution opening links or attachments > > > > > > On Wed, 2 Oct 2024 at 20:06, Rafael J. Wysocki <rafael@kernel.org> wrote: > >> On Wed, Sep 11, 2024 at 5:55 PM KobaK <kobak@nvidia.com> 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. > >>> 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 > >> I need input from EFI people on this, so can you please resend the > >> patch with a CC to linux-efi@vger.kernel.org? > >> > >>> Fixes: cefc7ca46235 ("ACPI: PRM: implement OperationRegion handler for the PlatformRtMechanism subtype") > >>> Signed-off-by: KobaK <kobak@nvidia.com> > > Please use your full name. > Hi Ardb, > Sure, will update. > > > >>> Reviewed-by: Matthew R. Ochs <mochs@nvidia.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 > >>> V5: Fix typo and pr_warn warning > >>> --- > >>> 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..cd4a7f5491d6 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); > > Wouldn't it make more sense to test the EFI_MEMORY_RUNTIME attribute > > rather than expecting/assuming a certain memory type in each case? > > That attribute is precisely what controls whether or not a region has > > been remapped into the firmware's page tables. > Please see the below > > > >>> + 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: %pUL) are missed, handler_addr %p, data_addr %p, param_addr %p", > > Please improve this diagnostic: 'are missed' is not very helpful. > > Are these good for you > I /think/ it looks ok but please resend it as a proper patch - the whitespace got mangled and it is difficult to read. > ``` > > -static u64 efi_pa_va_lookup(u64 pa, u32 type) > +static u64 efi_pa_va_lookup(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->type == type) && > + 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; > } > @@ -150,18 +150,20 @@ 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, > 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: %pUL) > are missed, handler_addr %p, data_addr %p, param_addr %p", > - cur_handler, &th->guid, th->handler_addr, > - (void *)th->static_data_buffer_addr, > (void *)th->acpi_param_buffer_addr); > + th->handler_addr = (void > *)efi_pa_va_lookup(handler_info->handler_address); > + if (!th->handler_addr) > + pr_warn( "Idx: %llu, failed to find VA for > handler_addr(GUID: %pUL, PA: %p)", > + cur_handler, &th->guid, th->handler_addr); > + > + th->static_data_buffer_addr = > efi_pa_va_lookup(handler_info->static_data_buffer_address); > + if (!th->static_data_buffer_addr) > + pr_warn( "Idx: %llu, failed to find VA for > data_addr(PA: %p)", > + cur_handler, &th->guid, (void > *)th->static_data_buffer_addr); > + > + th->acpi_param_buffer_addr = > efi_pa_va_lookup(handler_info->acpi_param_buffer_address); > + if (!th->acpi_param_buffer_addr) > + pr_warn( "Idx: %llu, failed to find VA for > param_addr(PA: %p)", > + cur_handler, &th->guid, (void > *)th->acpi_param_buffer_addr); > > ``` > ... > >>> @@ -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; > >>> + } > > What is the reason for this change, and the ones down below? > As per Rui's comment, goto can be replaced with return. > So I modified them with return and PRM_HANDLER_GUID_NOT_FOUND. I don't think this change is necessary, but it should be a separate patch at the very least.
diff --git a/drivers/acpi/prmt.c b/drivers/acpi/prmt.c index c78453c74ef5..cd4a7f5491d6 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: %pUL) are missed, handler_addr %p, data_addr %p, param_addr %p", + cur_handler, &th->guid, th->handler_addr, + (void *)th->static_data_buffer_addr, (void *)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)
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> --- 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 --- drivers/acpi/prmt.c | 49 +++++++++++++++++++++++++++++++-------------- 1 file changed, 34 insertions(+), 15 deletions(-)