| Message ID | 20240801014853.1594699-1-kobak@nvidia.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | acpi/prmt: find block with specific type | expand |
On Thu, 2024-08-01 at 09:48 +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. > Too many characters in one line. https://docs.kernel.org/process/submitting-patches.html#the-canonical-patch-format > [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. > prmt find a block with EFI_RUNTIME_SERVICES_CODE for prm context. > By using the correct memory types for runtime services, > we can ensure that the PRM handler and > its context are properly mapped in the virtual address space during > runtime, > preventing the paging request error. some general rules to follow when writing a changelog https://docs.kernel.org/process/maintainer-tip.html 4.2.3. Changelog > > [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> > --- > drivers/acpi/prmt.c | 46 ++++++++++++++++++++++++++++++------------- > -- > 1 file changed, 31 insertions(+), 15 deletions(-) > > diff --git a/drivers/acpi/prmt.c b/drivers/acpi/prmt.c > index c78453c74ef5..e2f0bdd81013 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(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; > + } > } > > + pr_err("PRM: Failed to find a block for pa: %lx type %u\n", > pa, type); > + If it is a pr_err, why not error out? or what is the proper handling for such failures? > return 0; > } > > @@ -148,9 +152,12 @@ 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); > } while (++cur_handler < tm->handler_count && (handler_info = > get_next_handler(handler_info))); > > return 0; > @@ -250,8 +257,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; > + goto error; I think it is okay to return AE_OK directly, right? thanks, rui > + } > + > + if (!handler->handler_addr || !handler- > >static_data_buffer_addr || > + !handler->acpi_param_buffer_addr) { > + buffer->prm_status = PRM_HANDLER_ERROR; > + goto error; > + } > > ACPI_COPY_NAMESEG(context.signature, "PRMC"); > context.revision = 0x0; > @@ -274,8 +289,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; > + goto error; > + } > > if (module->updatable) > module->updatable = false; > @@ -286,8 +303,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; > + goto error; > + } > > if (module->updatable) > buffer->prm_status = > UPDATE_UNLOCK_WITHOUT_LOCK; > @@ -301,10 +320,7 @@ static acpi_status > acpi_platformrt_space_handler(u32 function, > break; > } > > - return AE_OK; > - > -invalid_guid: > - buffer->prm_status = PRM_HANDLER_GUID_NOT_FOUND; > +error: > return AE_OK; > } >
On 8/21/24 11:20, Zhang, Rui wrote: > External email: Use caution opening links or attachments > > > On Thu, 2024-08-01 at 09:48 +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. >> > Too many characters in one line. > https://docs.kernel.org/process/submitting-patches.html#the-canonical-patch-format will fix this in the description. > > >> [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. >> prmt find a block with EFI_RUNTIME_SERVICES_CODE for prm context. >> By using the correct memory types for runtime services, >> we can ensure that the PRM handler and >> its context are properly mapped in the virtual address space during >> runtime, >> preventing the paging request error. > some general rules to follow when writing a changelog > https://docs.kernel.org/process/maintainer-tip.html 4.2.3. Changelog will decorate this. > >> [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> >> --- >> drivers/acpi/prmt.c | 46 ++++++++++++++++++++++++++++++------------- >> -- >> 1 file changed, 31 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/acpi/prmt.c b/drivers/acpi/prmt.c >> index c78453c74ef5..e2f0bdd81013 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(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; >> + } >> } >> >> + pr_err("PRM: Failed to find a block for pa: %lx type %u\n", >> pa, type); >> + > If it is a pr_err, why not error out? > or what is the proper handling for such failures? > >> return 0; >> } >> >> @@ -148,9 +152,12 @@ 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); >> } while (++cur_handler < tm->handler_count && (handler_info = >> get_next_handler(handler_info))); >> >> return 0; >> @@ -250,8 +257,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; >> + goto error; > I think it is okay to return AE_OK directly, right? > > thanks, > rui I'm also good for this. I followed the convention in this block. If change to "return", i will change all "goto error". How do you think? >> + } >> + >> + if (!handler->handler_addr || !handler- >>> static_data_buffer_addr || >> + !handler->acpi_param_buffer_addr) { >> + buffer->prm_status = PRM_HANDLER_ERROR; >> + goto error; >> + } >> >> ACPI_COPY_NAMESEG(context.signature, "PRMC"); >> context.revision = 0x0; >> @@ -274,8 +289,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; >> + goto error; >> + } >> >> if (module->updatable) >> module->updatable = false; >> @@ -286,8 +303,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; >> + goto error; >> + } >> >> if (module->updatable) >> buffer->prm_status = >> UPDATE_UNLOCK_WITHOUT_LOCK; >> @@ -301,10 +320,7 @@ static acpi_status >> acpi_platformrt_space_handler(u32 function, >> break; >> } >> >> - return AE_OK; >> - >> -invalid_guid: >> - buffer->prm_status = PRM_HANDLER_GUID_NOT_FOUND; >> +error: >> return AE_OK; >> } >>
On Wed, 2024-08-21 at 12:01 +0800, Koba Ko wrote: > On 8/21/24 11:20, Zhang, Rui wrote: > > External email: Use caution opening links or attachments > > > > > > On Thu, 2024-08-01 at 09:48 +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. > > > > > Too many characters in one line. > > https://docs.kernel.org/process/submitting-patches.html#the-canonical-patch-format > will fix this in the description. > > > > > > > [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. > > > prmt find a block with EFI_RUNTIME_SERVICES_CODE for prm context. > > > By using the correct memory types for runtime services, > > > we can ensure that the PRM handler and > > > its context are properly mapped in the virtual address space > > > during > > > runtime, > > > preventing the paging request error. > > some general rules to follow when writing a changelog > > https://docs.kernel.org/process/maintainer-tip.html 4.2.3. > > Changelog > will decorate this. > > > > > [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> > > > --- > > > drivers/acpi/prmt.c | 46 ++++++++++++++++++++++++++++++-------- > > > ----- > > > -- > > > 1 file changed, 31 insertions(+), 15 deletions(-) > > > > > > diff --git a/drivers/acpi/prmt.c b/drivers/acpi/prmt.c > > > index c78453c74ef5..e2f0bdd81013 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(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; > > > + } > > > } > > > > > > + pr_err("PRM: Failed to find a block for pa: %lx type > > > %u\n", > > > pa, type); > > > + > > If it is a pr_err, why not error out? > > or what is the proper handling for such failures? > > Not sure if you missed this one. It is not clear what is the expected behavior in this case. Better to describe this in the changelog as well. > > > return 0; > > > } > > > > > > @@ -148,9 +152,12 @@ 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); > > > } while (++cur_handler < tm->handler_count && > > > (handler_info = > > > get_next_handler(handler_info))); > > > > > > return 0; > > > @@ -250,8 +257,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; > > > + goto error; > > I think it is okay to return AE_OK directly, right? > > > > thanks, > > rui > > I'm also good for this. > I followed the convention in this block. > If change to "return", i will change all "goto error". > How do you think? sounds good to me. -rui > > > > + } > > > + > > > + if (!handler->handler_addr || !handler- > > > > static_data_buffer_addr || > > > + !handler->acpi_param_buffer_addr) { > > > + buffer->prm_status = PRM_HANDLER_ERROR; > > > + goto error; > > > + } > > > > > > ACPI_COPY_NAMESEG(context.signature, "PRMC"); > > > context.revision = 0x0; > > > @@ -274,8 +289,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; > > > + goto error; > > > + } > > > > > > if (module->updatable) > > > module->updatable = false; > > > @@ -286,8 +303,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; > > > + goto error; > > > + } > > > > > > if (module->updatable) > > > buffer->prm_status = > > > UPDATE_UNLOCK_WITHOUT_LOCK; > > > @@ -301,10 +320,7 @@ static acpi_status > > > acpi_platformrt_space_handler(u32 function, > > > break; > > > } > > > > > > - return AE_OK; > > > - > > > -invalid_guid: > > > - buffer->prm_status = PRM_HANDLER_GUID_NOT_FOUND; > > > +error: > > > return AE_OK; > > > } > > > >
On 8/21/24 12:55, Zhang, Rui wrote: > External email: Use caution opening links or attachments > > > On Wed, 2024-08-21 at 12:01 +0800, Koba Ko wrote: >> On 8/21/24 11:20, Zhang, Rui wrote: >>> External email: Use caution opening links or attachments >>> >>> >>> On Thu, 2024-08-01 at 09:48 +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. >>>> >>> Too many characters in one line. >>> https://docs.kernel.org/process/submitting-patches.html#the-canonical-patch-format >> will fix this in the description. >>> >>>> [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. >>>> prmt find a block with EFI_RUNTIME_SERVICES_CODE for prm context. >>>> By using the correct memory types for runtime services, >>>> we can ensure that the PRM handler and >>>> its context are properly mapped in the virtual address space >>>> during >>>> runtime, >>>> preventing the paging request error. >>> some general rules to follow when writing a changelog >>> https://docs.kernel.org/process/maintainer-tip.html 4.2.3. >>> Changelog >> will decorate this. >>>> [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> >>>> --- >>>> drivers/acpi/prmt.c | 46 ++++++++++++++++++++++++++++++-------- >>>> ----- >>>> -- >>>> 1 file changed, 31 insertions(+), 15 deletions(-) >>>> >>>> diff --git a/drivers/acpi/prmt.c b/drivers/acpi/prmt.c >>>> index c78453c74ef5..e2f0bdd81013 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(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; >>>> + } >>>> } >>>> >>>> + pr_err("PRM: Failed to find a block for pa: %lx type >>>> %u\n", >>>> pa, type); >>>> + >>> If it is a pr_err, why not error out? >>> or what is the proper handling for such failures? >>> > Not sure if you missed this one. > It is not clear what is the expected behavior in this case. Better to > describe this in the changelog as well. Sorry, missed. if get failure and return 0. in acpi_platformrt_space_handler, it takes care to handle the null pointers. ``` + if (!handler->handler_addr || !handler->static_data_buffer_addr || + !handler->acpi_param_buffer_addr) { + buffer->prm_status = PRM_HANDLER_ERROR; + goto error; + } ``` will also update in the description. >>>> return 0; >>>> } >>>> >>>> @@ -148,9 +152,12 @@ 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); >>>> } while (++cur_handler < tm->handler_count && >>>> (handler_info = >>>> get_next_handler(handler_info))); >>>> >>>> return 0; >>>> @@ -250,8 +257,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; >>>> + goto error; >>> I think it is okay to return AE_OK directly, right? >>> >>> thanks, >>> rui >> I'm also good for this. >> I followed the convention in this block. >> If change to "return", i will change all "goto error". >> How do you think? > sounds good to me. > > -rui > >>>> + } >>>> + >>>> + if (!handler->handler_addr || !handler- >>>>> static_data_buffer_addr || >>>> + !handler->acpi_param_buffer_addr) { >>>> + buffer->prm_status = PRM_HANDLER_ERROR; >>>> + goto error; >>>> + } >>>> >>>> ACPI_COPY_NAMESEG(context.signature, "PRMC"); >>>> context.revision = 0x0; >>>> @@ -274,8 +289,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; >>>> + goto error; >>>> + } >>>> >>>> if (module->updatable) >>>> module->updatable = false; >>>> @@ -286,8 +303,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; >>>> + goto error; >>>> + } >>>> >>>> if (module->updatable) >>>> buffer->prm_status = >>>> UPDATE_UNLOCK_WITHOUT_LOCK; >>>> @@ -301,10 +320,7 @@ static acpi_status >>>> acpi_platformrt_space_handler(u32 function, >>>> break; >>>> } >>>> >>>> - return AE_OK; >>>> - >>>> -invalid_guid: >>>> - buffer->prm_status = PRM_HANDLER_GUID_NOT_FOUND; >>>> +error: >>>> return AE_OK; >>>> } >>>>
On Wed, 2024-08-21 at 13:48 +0800, Koba Ko wrote: > > On 8/21/24 12:55, Zhang, Rui wrote: > > External email: Use caution opening links or attachments > > > > > > On Wed, 2024-08-21 at 12:01 +0800, Koba Ko wrote: > > > On 8/21/24 11:20, Zhang, Rui wrote: > > > > External email: Use caution opening links or attachments > > > > > > > > > > > > On Thu, 2024-08-01 at 09:48 +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. > > > > > > > > > Too many characters in one line. > > > > https://docs.kernel.org/process/submitting-patches.html#the-canonical-patch-format > > > will fix this in the description. > > > > > > > > > [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. > > > > > prmt find a block with EFI_RUNTIME_SERVICES_CODE for prm > > > > > context. > > > > > By using the correct memory types for runtime services, > > > > > we can ensure that the PRM handler and > > > > > its context are properly mapped in the virtual address space > > > > > during > > > > > runtime, > > > > > preventing the paging request error. > > > > some general rules to follow when writing a changelog > > > > https://docs.kernel.org/process/maintainer-tip.html 4.2.3. > > > > Changelog > > > will decorate this. > > > > > [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> > > > > > --- > > > > > drivers/acpi/prmt.c | 46 ++++++++++++++++++++++++++++++--- > > > > > ----- > > > > > ----- > > > > > -- > > > > > 1 file changed, 31 insertions(+), 15 deletions(-) > > > > > > > > > > diff --git a/drivers/acpi/prmt.c b/drivers/acpi/prmt.c > > > > > index c78453c74ef5..e2f0bdd81013 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(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; > > > > > + } > > > > > } > > > > > > > > > > + pr_err("PRM: Failed to find a block for pa: %lx type > > > > > %u\n", > > > > > pa, type); > > > > > + > > > > If it is a pr_err, why not error out? > > > > or what is the proper handling for such failures? > > > > > > Not sure if you missed this one. > > It is not clear what is the expected behavior in this case. Better > > to > > describe this in the changelog as well. > > Sorry, missed. > if get failure and return 0. > in acpi_platformrt_space_handler, it takes care to handle the null > pointers. > ``` > + if (!handler->handler_addr || > !handler->static_data_buffer_addr || > + !handler->acpi_param_buffer_addr) { > + buffer->prm_status = PRM_HANDLER_ERROR; > + goto error; > + } > ``` > will also update in the description. Yeah, but I mean pr_err() may be overkill if the driver is still functional. thanks, rui > > > > > > return 0; > > > > > } > > > > > > > > > > @@ -148,9 +152,12 @@ 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); > > > > > } while (++cur_handler < tm->handler_count && > > > > > (handler_info = > > > > > get_next_handler(handler_info))); > > > > > > > > > > return 0; > > > > > @@ -250,8 +257,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; > > > > > + goto error; > > > > I think it is okay to return AE_OK directly, right? > > > > > > > > thanks, > > > > rui > > > I'm also good for this. > > > I followed the convention in this block. > > > If change to "return", i will change all "goto error". > > > How do you think? > > sounds good to me. > > > > -rui > > > > > > > + } > > > > > + > > > > > + if (!handler->handler_addr || !handler- > > > > > > static_data_buffer_addr || > > > > > + !handler->acpi_param_buffer_addr) { > > > > > + buffer->prm_status = > > > > > PRM_HANDLER_ERROR; > > > > > + goto error; > > > > > + } > > > > > > > > > > ACPI_COPY_NAMESEG(context.signature, > > > > > "PRMC"); > > > > > context.revision = 0x0; > > > > > @@ -274,8 +289,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; > > > > > + goto error; > > > > > + } > > > > > > > > > > if (module->updatable) > > > > > module->updatable = false; > > > > > @@ -286,8 +303,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; > > > > > + goto error; > > > > > + } > > > > > > > > > > if (module->updatable) > > > > > buffer->prm_status = > > > > > UPDATE_UNLOCK_WITHOUT_LOCK; > > > > > @@ -301,10 +320,7 @@ static acpi_status > > > > > acpi_platformrt_space_handler(u32 function, > > > > > break; > > > > > } > > > > > > > > > > - return AE_OK; > > > > > - > > > > > -invalid_guid: > > > > > - buffer->prm_status = PRM_HANDLER_GUID_NOT_FOUND; > > > > > +error: > > > > > return AE_OK; > > > > > } > > > > >
On 8/21/24 14:33, Zhang, Rui wrote: > Yeah, but I mean pr_err() may be overkill if the driver is still > functional. how about replace with pr_warn?
On Wed, 2024-08-21 at 14:36 +0800, Koba Ko wrote: > > On 8/21/24 14:33, Zhang, Rui wrote: > > Yeah, but I mean pr_err() may be overkill if the driver is still > > functional. > > how about replace with pr_warn? when it fails, 1. the address space handler still returns AE_OK (is it right?) 2. I don't see how PRM_HANDLER_GUID_NOT_FOUND prm_status is handled So, if it is a critical error, we should fail the prmt probe immediately. If it is not, we can let space handler returns AE_OK like you do in this patch, and in this case, even a pr_info() is sufficient IMV. thanks, rui
On 8/21/24 14:48, Zhang, Rui wrote: > External email: Use caution opening links or attachments > > > On Wed, 2024-08-21 at 14:36 +0800, Koba Ko wrote: >> On 8/21/24 14:33, Zhang, Rui wrote: >>> Yeah, but I mean pr_err() may be overkill if the driver is still >>> functional. >> how about replace with pr_warn? > when it fails, > 1. the address space handler still returns AE_OK (is it right?) > 2. I don't see how PRM_HANDLER_GUID_NOT_FOUND prm_status is handled > > So, if it is a critical error, we should fail the prmt probe > immediately. > If it is not, we can let space handler returns AE_OK like you do in > this patch, and in this case, even a pr_info() is sufficient IMV. > > thanks, > rui Agree with you. it's worse to determine the failure on another place. better way like yours, when get failure, just complain and block the procedure in the scene. also will modify in the v2. thanks
On 8/21/24 15:03, Koba Ko wrote: > > On 8/21/24 14:48, Zhang, Rui wrote: >> External email: Use caution opening links or attachments >> >> >> On Wed, 2024-08-21 at 14:36 +0800, Koba Ko wrote: >>> On 8/21/24 14:33, Zhang, Rui wrote: >>>> Yeah, but I mean pr_err() may be overkill if the driver is still >>>> functional. >>> how about replace with pr_warn? >> when it fails, >> 1. the address space handler still returns AE_OK (is it right?) >> 2. I don't see how PRM_HANDLER_GUID_NOT_FOUND prm_status is handled >> >> So, if it is a critical error, we should fail the prmt probe >> immediately. >> If it is not, we can let space handler returns AE_OK like you do in >> this patch, and in this case, even a pr_info() is sufficient IMV. >> >> thanks, >> rui After reviewed again, I think it's a not critical error here. Even the current handler fails to get VA from efi_pa_va_lookup, the next handler still have a chance to get VA successfully. Block the procedure is overkill. I would like to use pr_warn because pr_info may not be enough IMV. How do you think!? Thanks > Agree with you. it's worse to determine the failure on another place. > better way like yours, when get failure, > just complain and block the procedure in the scene. > also will modify in the v2. > thanks >
diff --git a/drivers/acpi/prmt.c b/drivers/acpi/prmt.c index c78453c74ef5..e2f0bdd81013 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(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; + } } + pr_err("PRM: Failed to find a block for pa: %lx type %u\n", pa, type); + return 0; } @@ -148,9 +152,12 @@ 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); } while (++cur_handler < tm->handler_count && (handler_info = get_next_handler(handler_info))); return 0; @@ -250,8 +257,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; + goto error; + } + + if (!handler->handler_addr || !handler->static_data_buffer_addr || + !handler->acpi_param_buffer_addr) { + buffer->prm_status = PRM_HANDLER_ERROR; + goto error; + } ACPI_COPY_NAMESEG(context.signature, "PRMC"); context.revision = 0x0; @@ -274,8 +289,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; + goto error; + } if (module->updatable) module->updatable = false; @@ -286,8 +303,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; + goto error; + } if (module->updatable) buffer->prm_status = UPDATE_UNLOCK_WITHOUT_LOCK; @@ -301,10 +320,7 @@ static acpi_status acpi_platformrt_space_handler(u32 function, break; } - return AE_OK; - -invalid_guid: - buffer->prm_status = PRM_HANDLER_GUID_NOT_FOUND; +error: return AE_OK; }
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. prmt find a block with EFI_RUNTIME_SERVICES_CODE for prm context. By using the correct memory types for runtime services, we can ensure that the PRM handler and its 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> --- drivers/acpi/prmt.c | 46 ++++++++++++++++++++++++++++++--------------- 1 file changed, 31 insertions(+), 15 deletions(-)