From: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
To: Igor Mammedov <imammedo@redhat.com>
Cc: "Jonathan Cameron" <Jonathan.Cameron@huawei.com>,
"Shiju Jose" <shiju.jose@huawei.com>,
"Michael S. Tsirkin" <mst@redhat.com>,
"Philippe Mathieu-Daudé" <philmd@linaro.org>,
"Ani Sinha" <anisinha@redhat.com>,
"Dongjiu Geng" <gengdongjiu1@gmail.com>,
"Eduardo Habkost" <eduardo@habkost.net>,
"Marcel Apfelbaum" <marcel.apfelbaum@gmail.com>,
"Yanan Wang" <wangyanan55@huawei.com>,
"Zhao Liu" <zhao1.liu@intel.com>,
linux-kernel@vger.kernel.org, qemu-arm@nongnu.org,
qemu-devel@nongnu.org
Subject: Re: [PATCH RFC 4/5] acpi/generic_event_device: add logic to detect if HEST addr is available
Date: Tue, 12 Nov 2024 16:22:54 +0100 [thread overview]
Message-ID: <20241112162254.65cc3efc@foz.lan> (raw)
In-Reply-To: <20241112155557.728e9d68@foz.lan>
Em Tue, 12 Nov 2024 15:55:57 +0100
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:
> Em Thu, 3 Oct 2024 16:27:28 +0200
> Igor Mammedov <imammedo@redhat.com> escreveu:
>
> > > +++ b/hw/acpi/ghes.c
> > > @@ -513,7 +513,7 @@ void ghes_record_cper_errors(const void *cper, size_t len,
> > > }
> > > ags = &acpi_ged_state->ghes_state;
> > >
> > > - if (!ags->hest_addr_le) {
> > > + if (!ags->hest_lookup) {
> > > get_ghes_offsets(le64_to_cpu(ags->hw_error_le),
> > > &cper_addr, &read_ack_register_addr);
> >
> > just fencing off lookup is not enough,
> > to be compatible with qemu-9.1 (virt-9.1) we also should not publish hest_addr fwcfg.
>
> I tried this:
>
> diff --git a/hw/acpi/ghes.c b/hw/acpi/ghes.c
> index 201e72516608..6bb962d3c449 100644
> --- a/hw/acpi/ghes.c
> +++ b/hw/acpi/ghes.c
> @@ -402,8 +402,10 @@ void acpi_ghes_add_fw_cfg(AcpiGhesState *ags, FWCfgState *s,
> fw_cfg_add_file_callback(s, ACPI_HW_ERROR_ADDR_FW_CFG_FILE, NULL, NULL,
> NULL, &(ags->hw_error_le), sizeof(ags->hw_error_le), false);
>
> - fw_cfg_add_file_callback(s, ACPI_HEST_ADDR_FW_CFG_FILE, NULL, NULL,
> - NULL, &(ags->hest_addr_le), sizeof(ags->hest_addr_le), false);
> + if (ags->hest_lookup) {
> + fw_cfg_add_file_callback(s, ACPI_HEST_ADDR_FW_CFG_FILE, NULL, NULL,
> + NULL, &(ags->hest_addr_le), sizeof(ags->hest_addr_le), false);
> + }
>
> ags->present = true;
> }
>
> But with such change, boot fails:
>
> EFI stub: Booting Linux Kernel...
> UpdateRegionMappingRecursive(0): DF100000 - E1B90000 set 400 clr FF9F000000000B3F
> UpdateRegionMappingRecursive(1): DF100000 - E1B90000 set 400 clr FF9F000000000B3F
> UpdateRegionMappingRecursive(2): C0000000 - 100000000 set 6000000000070C clr 0
> UpdateRegionMappingRecursive(2): DF100000 - E1B90000 set 400 clr FF9F000000000B3F
> UpdateRegionMappingRecursive(3): DF000000 - DF200000 set 6000000000070C clr 0
> UpdateRegionMappingRecursive(3): DF100000 - DF200000 set 400 clr FF9F000000000B3F
> UpdateRegionMappingRecursive(3): E1A00000 - E1C00000 set 6000000000070C clr 0
> UpdateRegionMappingRecursive(3): E1A00000 - E1B90000 set 400 clr FF9F000000000B3F
> EFI stub: Generating empty DTB
> EFI stub: Exiting boot services...
> UpdateRegionMappingRecursive(0): 139AC1000 - 139CD0000 set 400 clr FF9F000000000B3F
> UpdateRegionMappingRecursive(1): 139AC1000 - 139CD0000 set 400 clr FF9F000000000B3F
> UpdateRegionMappingRecursive(2): 139AC1000 - 139CD0000 set 400 clr FF9F000000000B3F
> UpdateRegionMappingRecursive(3): 139A00000 - 139C00000 set 6000000000070C clr 0
> UpdateRegionMappingRecursive(3): 139AC1000 - 139C00000 set 400 clr FF9F000000000B3F
> UpdateRegionMappingRecursive(3): 139C00000 - 139CD0000 set 400 clr FF9F000000000B3F
> UpdateRegionMappingRecursive(0): 139AC1000 - 139AD0000 set 60000000000400 clr FF9F000000000B3F
> UpdateRegionMappingRecursive(1): 139AC1000 - 139AD0000 set 60000000000400 clr FF9F000000000B3F
> UpdateRegionMappingRecursive(2): 139AC1000 - 139AD0000 set 60000000000400 clr FF9F000000000B3F
> UpdateRegionMappingRecursive(3): 139AC1000 - 139AD0000 set 60000000000400 clr FF9F000000000B3F
> SetUefiImageMemoryAttributes - 0x000000013FE60000 - 0x0000000000040000 (0x0000000000000008)
> UpdateRegionMappingRecursive(0): 13FE60000 - 13FEA0000 set 70C clr 0
> UpdateRegionMappingRecursive(1): 13FE60000 - 13FEA0000 set 70C clr 0
> UpdateRegionMappingRecursive(2): 13FE60000 - 13FEA0000 set 70C clr 0
> UpdateRegionMappingRecursive(3): 13FE60000 - 13FEA0000 set 70C clr 0
> SetUefiImageMemoryAttributes - 0x000000013CAF0000 - 0x0000000000040000 (0x0000000000000008)
> UpdateRegionMappingRecursive(0): 13CAF0000 - 13CB30000 set 70C clr 0
> UpdateRegionMappingRecursive(1): 13CAF0000 - 13CB30000 set 70C clr 0
> UpdateRegionMappingRecursive(2): 13CAF0000 - 13CB30000 set 70C clr 0
> UpdateRegionMappingRecursive(3): 13CAF0000 - 13CB30000 set 70C clr 0
> SetUefiImageMemoryAttributes - 0x000000013CAA0000 - 0x0000000000040000 (0x0000000000000008)
> UpdateRegionMappingRecursive(0): 13CAA0000 - 13CAE0000 set 70C clr 0
> UpdateRegionMappingRecursive(1): 13CAA0000 - 13CAE0000 set 70C clr 0
> UpdateRegionMappingRecursive(2): 13CAA0000 - 13CAE0000 set 70C clr 0
> UpdateRegionMappingRecursive(3): 13CAA0000 - 13CAE0000 set 70C clr 0
> SetUefiImageMemoryAttributes - 0x000000013CA50000 - 0x0000000000040000 (0x0000000000000008)
> UpdateRegionMappingRecursive(0): 13CA50000 - 13CA90000 set 70C clr 0
> UpdateRegionMappingRecursive(1): 13CA50000 - 13CA90000 set 70C clr 0
> UpdateRegionMappingRecursive(2): 13CA50000 - 13CA90000 set 70C clr 0
> UpdateRegionMappingRecursive(3): 13CA50000 - 13CA90000 set 70C clr 0
> SetUefiImageMemoryAttributes - 0x000000013C960000 - 0x0000000000040000 (0x0000000000000008)
> UpdateRegionMappingRecursive(0): 13C960000 - 13C9A0000 set 70C clr 0
> UpdateRegionMappingRecursive(1): 13C960000 - 13C9A0000 set 70C clr 0
> UpdateRegionMappingRecursive(2): 13C960000 - 13C9A0000 set 70C clr 0
> UpdateRegionMappingRecursive(3): 13C960000 - 13C9A0000 set 70C clr 0
> SetUefiImageMemoryAttributes - 0x000000013FE20000 - 0x0000000000030000 (0x0000000000000008)
> UpdateRegionMappingRecursive(0): 13FE20000 - 13FE50000 set 70C clr 0
> UpdateRegionMappingRecursive(1): 13FE20000 - 13FE50000 set 70C clr 0
> UpdateRegionMappingRecursive(2): 13FE20000 - 13FE50000 set 70C clr 0
> UpdateRegionMappingRecursive(3): 13FE20000 - 13FE50000 set 70C clr 0
> SetUefiImageMemoryAttributes - 0x000000013C7B0000 - 0x0000000000030000 (0x0000000000000008)
> UpdateRegionMappingRecursive(0): 13C7B0000 - 13C7E0000 set 70C clr 0
> UpdateRegionMappingRecursive(1): 13C7B0000 - 13C7E0000 set 70C clr 0
> UpdateRegionMappingRecursive(2): 13C7B0000 - 13C7E0000 set 70C clr 0
> UpdateRegionMappingRecursive(3): 13C7B0000 - 13C7E0000 set 70C clr 0
> SetUefiImageMemoryAttributes - 0x000000013C770000 - 0x0000000000030000 (0x0000000000000008)
> UpdateRegionMappingRecursive(0): 13C770000 - 13C7A0000 set 70C clr 0
> UpdateRegionMappingRecursive(1): 13C770000 - 13C7A0000 set 70C clr 0
> UpdateRegionMappingRecursive(2): 13C770000 - 13C7A0000 set 70C clr 0
> UpdateRegionMappingRecursive(3): 13C770000 - 13C7A0000 set 70C clr 0
>
> At this point, nothing else appears, and bios doesn't boot OSPM.
>
> (I'm using an arm64 BIOS with debug enabled)
>
> Thanks,
> Mauro
Got it. In order to be able to remove a call to
fw_cfg_add_file_callback(), no calls to bios_linker_loader_write_pointer()
can happen.
That basically explains why we can't do:
if (!ags->hest_lookup) {
fw_cfg_add_file_callback(s, ACPI_HW_ERROR_ADDR_FW_CFG_FILE, NULL, NULL,
NULL, &(ags->hw_error_le), sizeof(ags->hw_error_le), false);
}
We need the BIOS file callback to solve all the pointers that
were created between HEST table and the hardware error table.
This hunk worked:
diff --git a/hw/acpi/ghes.c b/hw/acpi/ghes.c
index 201e72516608..245efde75a8f 100644
--- a/hw/acpi/ghes.c
+++ b/hw/acpi/ghes.c
@@ -385,10 +385,12 @@ void acpi_build_hest(GArray *table_data, GArray *hardware_errors,
* tell firmware to write into GPA the address of HEST via fw_cfg,
* once initialized.
*/
- bios_linker_loader_write_pointer(linker,
- ACPI_HEST_ADDR_FW_CFG_FILE, 0,
- sizeof(uint64_t),
- ACPI_BUILD_TABLE_FILE, hest_offset);
+ if (ags->hest_lookup) {
+ bios_linker_loader_write_pointer(linker,
+ ACPI_HEST_ADDR_FW_CFG_FILE, 0,
+ sizeof(uint64_t),
+ ACPI_BUILD_TABLE_FILE, hest_offset);
+ }
}
void acpi_ghes_add_fw_cfg(AcpiGhesState *ags, FWCfgState *s,
@@ -402,8 +404,10 @@ void acpi_ghes_add_fw_cfg(AcpiGhesState *ags, FWCfgState *s,
fw_cfg_add_file_callback(s, ACPI_HW_ERROR_ADDR_FW_CFG_FILE, NULL, NULL,
NULL, &(ags->hw_error_le), sizeof(ags->hw_error_le), false);
- fw_cfg_add_file_callback(s, ACPI_HEST_ADDR_FW_CFG_FILE, NULL, NULL,
- NULL, &(ags->hest_addr_le), sizeof(ags->hest_addr_le), false);
+ if (ags->hest_lookup) {
+ fw_cfg_add_file_callback(s, ACPI_HEST_ADDR_FW_CFG_FILE, NULL, NULL,
+ NULL, &(ags->hest_addr_le), sizeof(ags->hest_addr_le), false);
+ }
ags->present = true;
}
Thanks,
Mauro
next prev parent reply other threads:[~2024-11-12 15:23 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-10-01 11:42 [PATCH RFC 0/5] Change ghes driver to use HEST-based offsets Mauro Carvalho Chehab
2024-10-01 11:42 ` [PATCH RFC 1/5] acpi/ghes: add a firmware file with HEST address Mauro Carvalho Chehab
2024-10-01 11:42 ` [PATCH RFC 2/5] acpi/ghes: Use HEST table offsets when preparing GHES records Mauro Carvalho Chehab
2024-10-02 14:25 ` Igor Mammedov
2024-10-01 11:42 ` [PATCH RFC 3/5] acpi/generic_event_device: Update GHES migration to cover hest addr Mauro Carvalho Chehab
2024-10-02 15:15 ` Igor Mammedov
2024-10-02 15:46 ` Peter Xu
2024-10-01 11:42 ` [PATCH RFC 4/5] acpi/generic_event_device: add logic to detect if HEST addr is available Mauro Carvalho Chehab
2024-10-03 14:27 ` Igor Mammedov
2024-11-12 14:55 ` Mauro Carvalho Chehab
2024-11-12 15:22 ` Mauro Carvalho Chehab [this message]
2024-10-01 11:42 ` [PATCH RFC 5/5] arm/virt-acpi-build: Properly handle virt-9.1 Mauro Carvalho Chehab
2024-10-03 14:46 ` Igor Mammedov
2024-10-02 13:45 ` [PATCH RFC 0/5] Change ghes driver to use HEST-based offsets Igor Mammedov
2024-11-13 6:54 ` Mauro Carvalho Chehab
2024-11-13 6:57 ` Mauro Carvalho Chehab
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20241112162254.65cc3efc@foz.lan \
--to=mchehab+huawei@kernel.org \
--cc=Jonathan.Cameron@huawei.com \
--cc=anisinha@redhat.com \
--cc=eduardo@habkost.net \
--cc=gengdongjiu1@gmail.com \
--cc=imammedo@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=marcel.apfelbaum@gmail.com \
--cc=mst@redhat.com \
--cc=philmd@linaro.org \
--cc=qemu-arm@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=shiju.jose@huawei.com \
--cc=wangyanan55@huawei.com \
--cc=zhao1.liu@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.