On 17.08.2022 12:57, Leo Yan wrote:
> On Arm64, when boot Dom0 with the Linux kernel, it reports the warning:
>
> [ 0.403737] ------------[ cut here ]------------
> [ 0.403738] WARNING: CPU: 30 PID: 0 at
> drivers/irqchip/irq-gic-v3-its.c:3074 its_cpu_init+0x814/0xae0
> [ 0.403745] Modules linked in:
> [ 0.403748] CPU: 30 PID: 0 Comm: swapper/30 Tainted: G W
> 5.15.23-ampere-lts-standard #1
> [ 0.403752] pstate: 600001c5 (nZCv dAIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [ 0.403755] pc : its_cpu_init+0x814/0xae0
> [ 0.403758] lr : its_cpu_init+0x810/0xae0
> [ 0.403761] sp : ffff800009c03ce0
> [ 0.403762] x29: ffff800009c03ce0 x28: 000000000000001e x27:
> ffff880711f43000
> [ 0.403767] x26: ffff80000a3c0070 x25: fffffc1ffe0a4400 x24:
> ffff80000a3c0000
> [ 0.403770] x23: ffff8000095bc998 x22: ffff8000090a6000 x21:
> ffff800009850cb0
> [ 0.403774] x20: ffff800009701a10 x19: ffff800009701000 x18:
> ffffffffffffffff
> [ 0.403777] x17: 3030303035303031 x16: 3030313030303078 x15:
> 303a30206e6f6967
> [ 0.403780] x14: 6572206530312072 x13: 3030303030353030 x12:
> 3130303130303030
> [ 0.403784] x11: 78303a30206e6f69 x10: 6765722065303120 x9 :
> ffff80000870e710
> [ 0.403788] x8 : 6964657220646e75 x7 : 0000000000000003 x6 :
> 0000000000000000
> [ 0.403791] x5 : 0000000000000000 x4 : fffffc0000000000 x3 :
> 0000000000000010
> [ 0.403794] x2 : 000000000000ffff x1 : 0000000000010000 x0 :
> 00000000ffffffed
> [ 0.403798] Call trace:
> [ 0.403799] its_cpu_init+0x814/0xae0
> [ 0.403802] gic_starting_cpu+0x48/0x90
> [ 0.403805] cpuhp_invoke_callback+0x16c/0x5b0
> [ 0.403808] cpuhp_invoke_callback_range+0x78/0xf0
> [ 0.403811] notify_cpu_starting+0xbc/0xdc
> [ 0.403814] secondary_start_kernel+0xe0/0x170
> [ 0.403817] __secondary_switched+0x94/0x98
> [ 0.403821] ---[ end trace f68728a0d3053b70 ]---
>
> In the Linux kernel, the GIC driver tries to reserve ITS interrupt
> table, and the reserved pages can survive for kexec/kdump and will be
> used for secondary kernel. Linux kernel relies on MEMRESERVE EFI
> configuration table for memory page , but this table is not supported
> by Xen.
>
> This patch adds a MEMRESERVE configuration table into the system table,
> it points to a dummy data structure acpi_table_memreserve, this
> structure has the consistent definition with the Linux kernel.
> Following the method in Xen, it creates a table entry for the structure
> acpi_table_memreserve.
>
> Cc: Ard Biesheuvel <a...@kernel.org>
> Cc: Marc Zyngier <m...@kernel.org>
> Cc: Bertrand Marquis <bertrand.marq...@arm.com>
> Cc: Rahul Singh <rahul.si...@arm.com>
> Cc: Peter Griffin <peter.grif...@linaro.org>
> Signed-off-by: Leo Yan <leo....@linaro.org>
> ---
> xen/arch/arm/acpi/domain_build.c | 24 ++++++++++++++++++++++++
> xen/arch/arm/efi/efi-dom0.c | 19 ++++++++++++++++---
> xen/arch/arm/include/asm/acpi.h | 1 +
> xen/include/acpi/actbl.h | 17 +++++++++++++++++
> xen/include/efi/efiapi.h | 2 ++
> 5 files changed, 60 insertions(+), 3 deletions(-)
Please make sure you Cc all maintainers of all files that you touch.
Albeit, see below, you could indeed have avoided Cc-ing me if you
hadn't misplaced stuff in two of the headers that you fiddle with.
> --- a/xen/arch/arm/efi/efi-dom0.c
> +++ b/xen/arch/arm/efi/efi-dom0.c
> @@ -38,7 +38,7 @@ size_t __init estimate_efi_size(unsigned int mem_nr_banks)
> {
> size_t size;
> size_t est_size = sizeof(EFI_SYSTEM_TABLE);
> - size_t ect_size = sizeof(EFI_CONFIGURATION_TABLE);
> + size_t ect_size = sizeof(EFI_CONFIGURATION_TABLE) * 2;
> size_t emd_size = sizeof(EFI_MEMORY_DESCRIPTOR);
> size_t fw_vendor_size = sizeof(xen_efi_fw_vendor);
> unsigned int acpi_mem_nr_banks = 0;
> @@ -63,7 +63,8 @@ void __init acpi_create_efi_system_table(struct domain *d,
>
> table_addr = d->arch.efi_acpi_gpa
> + acpi_get_table_offset(tbl_add, TBL_EFIT);
> - table_size = sizeof(EFI_SYSTEM_TABLE) + sizeof(EFI_CONFIGURATION_TABLE)
> + table_size = sizeof(EFI_SYSTEM_TABLE)
> + + sizeof(EFI_CONFIGURATION_TABLE) * 2
> + sizeof(xen_efi_fw_vendor);
Nit: Indentation.
> @@ -75,7 +76,7 @@ void __init acpi_create_efi_system_table(struct domain *d,
> efi_sys_tbl->Hdr.HeaderSize = table_size;
>
> efi_sys_tbl->FirmwareRevision = 1;
> - efi_sys_tbl->NumberOfTableEntries = 1;
> + efi_sys_tbl->NumberOfTableEntries = 2;
This is the 3rd magic "2" - I think there wants to be some consolidation,
such that it becomes obvious which "2"-s really are the same (and would
change together if, like you do here, another entry is needed).
> @@ -86,6 +87,18 @@ void __init acpi_create_efi_system_table(struct domain *d,
> efi_conf_tbl->VendorTable = (VOID *)tbl_add[TBL_RSDP].start;
> efi_sys_tbl->ConfigurationTable = (EFI_CONFIGURATION_TABLE *)(table_addr
> + offset);
> +
> + /*
> + * Configuration table for MEMRESERVE is used in Linux kernel for
> + * reserving pages, its main purpose is used for kexec/kdump to
> + * reserve persistent pages (e.g. GIC pending pages) for the secondary
> + * kernel.
> + */
> + offset += sizeof(EFI_CONFIGURATION_TABLE);
> + efi_conf_tbl = (EFI_CONFIGURATION_TABLE *)(base_ptr + offset);
> + efi_conf_tbl->VendorGuid = (EFI_GUID)LINUX_EFI_MEMRESERVE_TABLE_GUID;
> + efi_conf_tbl->VendorTable = (VOID *)tbl_add[TBL_MRSV].start;
> +
> xz_crc32_init();
> efi_sys_tbl->Hdr.CRC32 = xz_crc32((uint8_t *)efi_sys_tbl,
> efi_sys_tbl->Hdr.HeaderSize, 0);
Rather than adjusting offset and calculating efi_conf_table fdrom scratch,
perhaps better simply efi_conf_table++? That way there would be one less
cast, which are always somewhat risky.
> --- a/xen/include/acpi/actbl.h
> +++ b/xen/include/acpi/actbl.h
> @@ -302,6 +302,23 @@ struct acpi_table_fadt {
> #define ACPI_FADT_HW_REDUCED (1<<20) /* 20: [V5] ACPI hardware is
> not implemented (ACPI 5.0) */
> #define ACPI_FADT_LOW_POWER_S0 (1<<21) /* 21: [V5] S0 power savings
> are equal or better than S3 (ACPI 5.0) */
>
> +/*******************************************************************************
> + *
> + * MEMRESERVE - Dummy entry for memory reserve configuration table
> + *
> +
> ******************************************************************************/
> +
> +struct acpi_table_memreserve {
> + int size; /* allocated size of the array */
> + int count; /* number of entries used */
> + u64 next; /* pa of next struct instance */
> + struct {
> + u64 base;
> + u64 size;
> + } entry[];
> +};
This header holds ACPI spec defined data structures. This one looks
to be a Linux one, and hence shouldn't be defined here. You use it
in a single CU only, so I see no reason to define it there.
Furthermore - what if Linux decided to change their structure? Or
is there a guarantee that they won't? Generally such structures
belong in the public interface, guaranteeing forward compatibility
even if Linux decided to change / extend theirs (at which point
consuming code there would need to do translation, but maybe using
a Xen-defined struct [plus translation in Linux] right away would
be best).
Finally, style-wise, please don't use u64 in new code anymore; we
are trying hard to move over to standard uint<N>_t types. Plus,
unless indeed mandated by Linux, please avoid signed types for
fields (or variables) which can never go negative.
> +
> +
> /* Values for preferred_profile (Preferred Power Management Profiles) */
Please don't add double blank lines anywhere.
> --- a/xen/include/efi/efiapi.h
> +++ b/xen/include/efi/efiapi.h
> @@ -882,6 +882,8 @@ typedef struct _EFI_BOOT_SERVICES {
> #define SAL_SYSTEM_TABLE_GUID \
> { 0xeb9d2d32, 0x2d88, 0x11d3, {0x9a, 0x16, 0x0, 0x90, 0x27, 0x3f, 0xc1,
> 0x4d} }
>
> +#define LINUX_EFI_MEMRESERVE_TABLE_GUID \
> + { 0x888eb0c6, 0x8ede, 0x4ff5, {0xa8, 0xf0, 0x9a, 0xee, 0x5c, 0xb9, 0x77,
> 0xc2} }
This header holds EFI spec mandated definitions (generally taken
from the gnu-efi project), when this one again looks to be a Linux
one (and again looks to be used in only a single CU).
Jan
