| Message ID | 20241006153611.1165482-1-kobak@nvidia.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2,V6] acpi/prmt: find block with specific type | expand |
On Sun, 2024-10-06 at 23:36 +0800, KobaK wrote: > From: koba ko <kobak@nvidia.com> > > Replace gotos with returns > > Signed-off-by: koba ko <kobak@nvidia.com> I think my previous comment was valid because a different prm_status is returned, say, PRM_HANDLER_ERROR. Given that we return AE_OK directly for PRM_HANDLER_ERROR case in patch 1/2, I think it is okay to drop this patch. thanks, rui > --- > drivers/acpi/prmt.c | 22 ++++++++++++---------- > 1 file changed, 12 insertions(+), 10 deletions(-) > > diff --git a/drivers/acpi/prmt.c b/drivers/acpi/prmt.c > index 970207bc8f4a..b0cf4428f7e3 100644 > --- a/drivers/acpi/prmt.c > +++ b/drivers/acpi/prmt.c > @@ -288,8 +288,10 @@ 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) { > @@ -318,8 +320,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; > @@ -330,8 +334,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; > @@ -346,10 +352,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)
On Sun, 2024-10-06 at 23:36 +0800, KobaK 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: > __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: Koba Ko <kobak@nvidia.com> > Reviewed-by: Matthew R. Ochs <mochs@nvidia.com> Reviewed-by: Zhang Rui <rui.zhang@intel.com> -rui > --- > 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 > --- > drivers/acpi/prmt.c | 22 +++++++++++++++++++++- > 1 file changed, 21 insertions(+), 1 deletion(-) > > diff --git a/drivers/acpi/prmt.c b/drivers/acpi/prmt.c > index 1cfaa5957ac4..970207bc8f4a 100644 > --- a/drivers/acpi/prmt.c > +++ b/drivers/acpi/prmt.c > @@ -79,8 +79,10 @@ static u64 efi_pa_va_lookup(u64 pa) > 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; > + } > } > > return 0; > @@ -149,8 +151,20 @@ acpi_parse_prmt(union acpi_subtable_headers > *header, const unsigned long end) > > guid_copy(&th->guid, (guid_t *)handler_info- > >handler_guid); > 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(GUID: %pUL, 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(GUID: %pUL, PA: %p)", > + cur_handler, &th->guid, (void *)th- > >acpi_param_buffer_addr); > + > } while (++cur_handler < tm->handler_count && (handler_info = > get_next_handler(handler_info))); > > return 0; > @@ -277,6 +291,12 @@ 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;
On Sun, 6 Oct 2024 at 17:36, 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: You can drop the call trace from the commit log. > __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. This is outdated now > If no suitable block is found, a warning message will be prompted > but the procedue continues to manage the next prm handler. procedure > However, if the prm handler is actullay called without proper allocation, actually > 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 ensure please capitalize PRM and PRMT consistently > 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> > --- > 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 > --- > drivers/acpi/prmt.c | 22 +++++++++++++++++++++- > 1 file changed, 21 insertions(+), 1 deletion(-) > > diff --git a/drivers/acpi/prmt.c b/drivers/acpi/prmt.c > index 1cfaa5957ac4..970207bc8f4a 100644 > --- a/drivers/acpi/prmt.c > +++ b/drivers/acpi/prmt.c > @@ -79,8 +79,10 @@ static u64 efi_pa_va_lookup(u64 pa) > 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)) { Please indent with 4 spaces so the ( line up vertically > return pa_offset + md->virt_addr + page - md->phys_addr; > + } > } > > return 0; > @@ -149,8 +151,20 @@ acpi_parse_prmt(union acpi_subtable_headers *header, const unsigned long end) > > guid_copy(&th->guid, (guid_t *)handler_info->handler_guid); > 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(GUID: %pUL, 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(GUID: %pUL, PA: %p)", > + cur_handler, &th->guid, (void *)th->acpi_param_buffer_addr); > + Can we move this warning into efi_pa_va_lookup() so we don't need to duplicate it three times? > } while (++cur_handler < tm->handler_count && (handler_info = get_next_handler(handler_info))); > > return 0; > @@ -277,6 +291,12 @@ 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) { Please split the condition into three lines, and use 4 spaces of indentation on the continuation lines. > + 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 Tue, 8 Oct 2024 at 05:24, Zhang, Rui <rui.zhang@intel.com> wrote: > > On Sun, 2024-10-06 at 23:36 +0800, KobaK wrote: > > From: koba ko <kobak@nvidia.com> > > > > Replace gotos with returns > > > > Signed-off-by: koba ko <kobak@nvidia.com> > > I think my previous comment was valid because a different prm_status is > returned, say, PRM_HANDLER_ERROR. > > Given that we return AE_OK directly for PRM_HANDLER_ERROR case in patch > 1/2, I think it is okay to drop this patch. > Agreed
diff --git a/drivers/acpi/prmt.c b/drivers/acpi/prmt.c index 1cfaa5957ac4..970207bc8f4a 100644 --- a/drivers/acpi/prmt.c +++ b/drivers/acpi/prmt.c @@ -79,8 +79,10 @@ static u64 efi_pa_va_lookup(u64 pa) 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; + } } return 0; @@ -149,8 +151,20 @@ acpi_parse_prmt(union acpi_subtable_headers *header, const unsigned long end) guid_copy(&th->guid, (guid_t *)handler_info->handler_guid); 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(GUID: %pUL, 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(GUID: %pUL, PA: %p)", + cur_handler, &th->guid, (void *)th->acpi_param_buffer_addr); + } while (++cur_handler < tm->handler_count && (handler_info = get_next_handler(handler_info))); return 0; @@ -277,6 +291,12 @@ 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;